On Monday 27 January 2014 10:48:36 Linus Lüssing wrote:
/**
- batadv_mcast_want_all_count - number of nodes with unspecific mcast
interest + * @bat_priv: the bat priv with all the soft interface information + * @ethhdr: ethernet header of a packet
- @want_all_list: pointer to a mcast want list in our bat_priv
- Return the number of nodes which want all IPv4 multicast traffic if
- the given ethhdr is from an IPv4 packet or the number of nodes which
want + * all IPv6 traffic if it matches an IPv6 packet and set the want_list to the + * according one in our bat_priv. For other frame types leave the want_list + * untouched and return zero.
- */
+static int batadv_mcast_want_all_count(struct batadv_priv *bat_priv,
struct ethhdr *ethhdr,struct hlist_head **want_all_list)+{
- int ret;
- switch (ntohs(ethhdr->h_proto)) {
- case ETH_P_IP:
ret = atomic_read(&bat_priv->mcast.num_want_all_ipv4);if (ret)*want_all_list = &bat_priv->mcast.want_all_ipv4_list;break;- case ETH_P_IPV6:
ret = atomic_read(&bat_priv->mcast.num_want_all_ipv6);if (ret)*want_all_list = &bat_priv->mcast.want_all_ipv6_list;break;- default:
/* we shouldn't be here... */ret = 0;- }
- return ret;
+}
As far as I can tell the want_all list is returned through 3 different functions to end up in batadv_mcast_want_all_node_get() where the code checks again for IPv4 vs IPv6. Wouldn't be much easier to make a simple IPv4/IPv6 in that function to retrieve the list ? Or better, batadv_mcast_want_all_ipv4_node_get() / batadv_mcast_want_all_ipv6_node_get() get bat_priv passed and use the correct list ? I see no need to pass the list around.
/**
- batadv_mcast_want_all_ipv4_node_get - get an orig_node with
want_all_ipv4 + * @head: list of originators that want all IPv4 multicast traffic + *
- Return the first orig_node from the given want_all_ipv4 list. Increases
- the refcount of the returned orig_node.
- */
+static struct batadv_orig_node * +batadv_mcast_want_all_ipv4_node_get(struct hlist_head *head) +{
- struct batadv_orig_node *orig_node = NULL;
- rcu_read_lock();
- hlist_for_each_entry_rcu(orig_node, head,
mcast_want_all_ipv4_node) {if (atomic_inc_not_zero(&orig_node->refcount))break;- }
- rcu_read_unlock();
- return orig_node;
+}
+/**
- batadv_mcast_want_all_ipv6_node_get - get an orig_node with
want_all_ipv6 + * @head: list of originators that want all IPv6 multicast traffic + *
- Return the first orig_node from the given want_all_ipv6 list. Increases
- the refcount of the returned orig_node.
- */
+static struct batadv_orig_node * +batadv_mcast_want_all_ipv6_node_get(struct hlist_head *head) +{
- struct batadv_orig_node *orig_node = NULL;
- rcu_read_lock();
- hlist_for_each_entry_rcu(orig_node, head,
mcast_want_all_ipv6_node) {if (atomic_inc_not_zero(&orig_node->refcount))break;- }
- rcu_read_unlock();
- return orig_node;
+}
Both functions have the same crucial bug. What will the function return if we have on entry in the list but are unable to increment the refcount ?
+/**
- batadv_mcast_list_add - grab a lock and add a node to a head
- @node: the node to add
- @head: the head to add the node to
- @lock: the lock to grab while adding the node to the head
- */
+static void batadv_mcast_list_add(struct hlist_node *node,
struct hlist_head *head,spinlock_t *lock)+{
- spin_lock_bh(lock);
- hlist_add_head_rcu(node, head);
- spin_unlock_bh(lock);
+}
+/**
- batadv_mcast_list_del - grab a lock and delete a node from its list
- @node: the node to delete from its list
- @lock: the lock to grab while deleting the node from its list
- */
+static void batadv_mcast_list_del(struct hlist_node *node, spinlock_t *lock) +{
- spin_lock_bh(lock);
- hlist_del_rcu(node);
- spin_unlock_bh(lock);
+}
+/**
- batadv_mcast_list_update - update the list of a flag
- @flag: the flag we want to update the list for
- @node: a list node of an originator
- @head: the list head the node might be added to
- @lock: the lock that synchronizes list modifications
- @new_flags: the new capability bitset of a node
- @old_flags: the current, to be updated bitset of a node
- Update the list of the given node/head with the help of the new flag
- information of an originator to contain the nodes which have the given
- flag set.
- */
+static void batadv_mcast_list_update(uint8_t flag,
struct hlist_node *node,struct hlist_head *head,spinlock_t *lock,uint8_t new_flags, int old_flags)+{
- if (new_flags & flag && !(old_flags & flag))
batadv_mcast_list_add(node, head, lock);- else if (!(new_flags & flag) && old_flags & flag)
batadv_mcast_list_del(node, lock);+}
Didn't we agree on banishing batadv_mcast_list_update() a while ago ?
+/**
- batadv_mcast_want_all_node_get - get an orig_node with an mcast want
list
- @want_all_list: list of originators that want all IPv4 or IPv6 mcast
traffic + * @bat_priv: the bat priv with all the soft interface information
- Return the first orig_node from the given want_all list. Increases the
- refcount of the returned orig_node.
- */
+struct batadv_orig_node * +batadv_mcast_want_all_node_get(struct hlist_head *want_all_list,
struct batadv_priv *bat_priv)+{
if (want_all_list == &bat_priv->mcast.want_all_ipv4_list)return batadv_mcast_want_all_ipv4_node_get(want_all_list);else if (want_all_list == &bat_priv->mcast.want_all_ipv6_list)return batadv_mcast_want_all_ipv6_node_get(want_all_list);elsereturn NULL;+}
In case there is a good reason to keep this function: bat_priv should be the first argument.
+/**
- batadv_send_skb_via_mcast - send an skb to a node with a WANT_ALL flag
- @bat_priv: the bat priv with all the soft interface information
- @skb: payload to send
- @vid: the vid to be used to search the translation table
- @want_all_list: a list of originators with a WANT_ALL flag
- Get an originator node from the want_all_list. Wrap the given skb into a
- batman-adv unicast header and send this frame to this node.
- */
+int batadv_send_skb_via_mcast(struct batadv_priv *bat_priv,
struct sk_buff *skb, unsigned short vid,struct hlist_head *want_all_list)+{
- struct batadv_orig_node *orig_node;
- orig_node = batadv_mcast_want_all_node_get(want_all_list, bat_priv);
- return batadv_send_skb_unicast(bat_priv, skb, BATADV_UNICAST, 0,
orig_node, vid);+}
Maybe I am missing the whole point of WANT_ALL but why do we maintain a list of WANT_ALL nodes to only send the packet to the first valid entry in the list?
Cheers, Marek