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

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