Re: [B.A.T.M.A.N.] [PATCH 1/2] batman-adv: softif lan loop avoidance

[Sven Eckelmann <sven.eckelmann@gmx.de>]

[-- Attachment #1: Type: Text/Plain, Size: 4167 bytes --]

Marek Lindner wrote:
> +static void softif_neigh_hold(struct softif_neigh *softif_neigh)
> +{
> +       if (!softif_neigh)
> +               return;
> +
> +       atomic_inc(&softif_neigh->refcnt);
> +}
> +
> +static void softif_neigh_put(struct softif_neigh *softif_neigh)
> +{
> +       if (!softif_neigh)
> +               return;
> +
> +       if (atomic_dec_and_test(&softif_neigh->refcnt))
> +               kfree(softif_neigh);
> +}

Do you want to change the other _hold and _put functions to make it consistent 
in batman-adv regarding the NULL pointer checks?

> +void softif_neigh_purge(struct bat_priv *bat_priv)
> +{
> +	struct softif_neigh *softif_neigh;
> +	struct hlist_node *node, *node_tmp;
> +
> +	hlist_for_each_entry_safe(softif_neigh, node, node_tmp,
> +				  &bat_priv->softif_neigh_list, list) {
> +
> +		if (!time_after(jiffies, softif_neigh->last_seen +
> +				msecs_to_jiffies(SOFTIF_NEIGH_TIMEOUT)))
> +			continue;
> +
> +		spin_lock(&bat_priv->softif_neigh_lock);
> +		hlist_del_rcu(&softif_neigh->list);
> +		spin_unlock(&bat_priv->softif_neigh_lock);
> +
> +		if (bat_priv->softif_neigh == softif_neigh) {
> +			bat_priv->softif_neigh = NULL;
> +			bat_dbg(DBG_ROUTES, bat_priv,
> +				 "Current mesh exit point '%pM' vanished "
> +				 "(vid: %d).\n",
> +				 softif_neigh->addr, softif_neigh->vid);
> +			softif_neigh_put(bat_priv->softif_neigh);
> +		}
> +
> +		synchronize_rcu();
> +		softif_neigh_put(softif_neigh);
> +	}

You must protect the whole loop and not only the hlist_del_rcu with the 
bat_priv->softif_neigh_lock, And it must be with spin_lock_irqsave because you 
use it in context were irq is disabled and in some with enabled irq 
contexts... or am I wrong?

> +void softif_batman_recv(struct sk_buff *skb, struct net_device *dev, short
> vid) +{
> +	struct bat_priv *bat_priv = netdev_priv(dev);
> +	struct ethhdr *ethhdr = (struct ethhdr *)skb->data;
> +	struct batman_packet *batman_packet;
> +	struct softif_neigh *softif_neigh;
> +
> +	if (ntohs(ethhdr->h_proto) == ETH_P_8021Q)
> +		batman_packet = (struct batman_packet *)
> +					(skb->data + ETH_HLEN + VLAN_HLEN);
> +	else
> +		batman_packet = (struct batman_packet *)(skb->data + ETH_HLEN);
> +
> +	if (batman_packet->version != COMPAT_VERSION)
> +		goto err;
> +
> +	if (batman_packet->packet_type != BAT_PACKET)
> +		goto err;
> +
> +	if (!(batman_packet->flags & PRIMARIES_FIRST_HOP))
> +		goto err;
> +
> +	if (is_my_mac(batman_packet->orig))
> +		goto err;
> +
> +	softif_neigh = softif_neigh_get(bat_priv, batman_packet->orig, vid);
> +
> +	if (!softif_neigh)
> +		goto err;
> +
> +	if (bat_priv->softif_neigh == softif_neigh)
> +		goto out;
> +
> +	/* we got a neighbor but its mac is 'bigger' than ours  */
> +	if (memcmp(bat_priv->primary_if->net_dev->dev_addr,
> +		   softif_neigh->addr, ETH_ALEN) < 0)
> +		goto out;
> +
> +	/* switch to new 'smallest neighbor' */
> +	if ((bat_priv->softif_neigh) &&
> +	    (memcmp(softif_neigh->addr, bat_priv->softif_neigh->addr,
> +							ETH_ALEN) < 0)) {
> +		bat_dbg(DBG_ROUTES, bat_priv,
> +			"Changing mesh exit point from %pM (vid: %d) "
> +			"to %pM (vid: %d).\n",
> +			 bat_priv->softif_neigh->addr,
> +			 bat_priv->softif_neigh->vid,
> +			 softif_neigh->addr, softif_neigh->vid);
> +		softif_neigh_put(bat_priv->softif_neigh);
> +		bat_priv->softif_neigh = softif_neigh;
> +		/* we need to hold the additional reference */
> +		goto err;
> +	}
> +
> +	/* close own batX device and use softif_neigh as exit node */
> +	if ((!bat_priv->softif_neigh) &&
> +	    (memcmp(softif_neigh->addr,
> +		    bat_priv->primary_if->net_dev->dev_addr, ETH_ALEN) < 0)) {
> +		bat_dbg(DBG_ROUTES, bat_priv,
> +			"Setting mesh exit point to %pM (vid: %d).\n",
> +			softif_neigh->addr, softif_neigh->vid);
> +		bat_priv->softif_neigh = softif_neigh;
> +		/* we need to hold the additional reference */
> +		goto err;
> +	}
> +
> +out:
> +	softif_neigh_put(softif_neigh);
> +err:
> +	kfree_skb(skb);
> +	return;
> +}

Can you mark that function as static?

Best regards,
	Sven

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