[openib-general] Re: ipoib error handling question

[Roland Dreier]

 > Roland, The following code in neigh_add_path looks strange to me:
 > 
 >                 neigh->ah  = NULL;
 >                 if (skb_queue_len(&neigh->queue) < IPOIB_MAX_PATH_REC_QUEUE) {
 >                         __skb_queue_tail(&neigh->queue, skb);
 >                 } else {
 >                         ++priv->stats.tx_dropped;
 >                         dev_kfree_skb_any(skb);
 >                 }
 > 
 >                 if (!path->query && path_rec_start(dev, path))
 >                         goto err_list;
 > 
 > It seems that if the queue is full, and path_rec_start fails, we will
 > double-free the skb. Would it make sense to simply goto err_list if the queue
 > is full?

Yes, that looks like an obvious mistake.  I checked this in:

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 1633aad..00342d5 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -519,12 +519,10 @@ static void neigh_add_path(struct sk_buf
 			   be32_to_cpup((__be32 *) skb->dst->neighbour->ha));
 	} else {
 		neigh->ah  = NULL;
-		if (skb_queue_len(&neigh->queue) < IPOIB_MAX_PATH_REC_QUEUE) {
+		if (skb_queue_len(&neigh->queue) < IPOIB_MAX_PATH_REC_QUEUE)
 			__skb_queue_tail(&neigh->queue, skb);
-		} else {
-			++priv->stats.tx_dropped;
-			dev_kfree_skb_any(skb);
-		}
+		else
+			goto err_list;
 
 		if (!path->query && path_rec_start(dev, path))
 			goto err;