Re: [B.A.T.M.A.N.] [PATCH 6/6] batman-adv: Send multicast packets to nodes with a WANT_ALL flag

[Marek Lindner <mareklindner@neomailbox.ch>]

[-- Attachment #1: Type: text/plain, Size: 7365 bytes --]

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);
> +       else
> +               return 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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]