[ewg] Re: IPoIB panics on ipoib_cm_handle_rx_wc()

[Or Gerlitz]

> https://bugs.openfabrics.org/show_bug.cgi?id=989
> ------- Comment #13 from Kris.Corwin at hp.com  2008-04-23 13:23 -------
> I think I found the problem and have a fix.
>
> In ipoib_cm_handle_rx_wc(), if the byte_len is < SKB_TSHOLD, a new sk_buff
> is allocated.  The sk_buff's mac.raw is not initialized.  It sends the packet
> up the stack to netif_receive_skb().  In netif_receive_skb(), skb->mac_len
> is computed by subtracting mac.raw from nh.raw.  Since mac.raw was not
> initialized, we get a very large number.  It eventually leads to a panic in
> skb_under_panic.
>
> The diff of the fix:
> drivers/infiniband/ulp/ipoib/ipoib_cm.c.pre_kris
> drivers/infiniband/ulp/ipoib/ipoib_cm.c
> *** drivers/infiniband/ulp/ipoib/ipoib_cm.c.pre_kris    2008-04-23 16:05:26.000000000 -0400
> --- drivers/infiniband/ulp/ipoib/ipoib_cm.c     2008-04-23 15:16:23.000000000-0400
> ***************
> *** 622,627 ****
> --- 622,628 ----
>                         skb_copy_from_linear_data_offset(skb, IPOIB_ENCAP_LEN,
>                                                          small_skb->data, dlen);
>                         skb_put(small_skb, dlen);
> +                       skb_reset_mac_header(small_skb);
>                         skb = small_skb;
>                         goto copied;
>                 }
Hi Kris,

Good catch. This code does not exist in the mainline kernel and was 
added through the ofed 1.3 (non) process, see the patch below. Does this 
bug hits you for --every-- small packet received with connected mode? if 
not, can you explain why?

The patch for itself as provided by the ofed sources (kernel_patches/fixes/ipoib_0320_small_skb_copy.patch) is event not documented, I took the change log from the git used to store it. This not reviewed and not documented patch which has a bug who could have been found if reviewed is yet another good example why code should not be added through ofed but rather through the mainline cycle, etc, oh well.

Or.
> commit 92557c139fd8329daf1a1bf8beeaa6ae940b055a
> Author: Eli Cohen <eli at mtls03.(none)>
> Date:   Mon Feb 18 21:56:03 2008 +0200
>
>     IB/ipoib: copy small SKBs in CM mode
>
>     CM mode handling of received packets involves iterating trough the fragments.
>     This is time consuming and in case of small packets it is better to allocate
>     a new small skb and copy the data and pass this smaller SKB up to the IP stack.
>
>     Signed-off-by: Eli Cohen <eli at mellanox.co.il>
>
> Index: ofa_1_3_dev_kernel/drivers/infiniband/ulp/ipoib/ipoib.h
> ===================================================================
> --- ofa_1_3_dev_kernel.orig/drivers/infiniband/ulp/ipoib/ipoib.h	2008-02-18 19:23:23.000000000 +0200
> +++ ofa_1_3_dev_kernel/drivers/infiniband/ulp/ipoib/ipoib.h	2008-02-18 22:20:48.000000000 +0200
> @@ -99,6 +99,8 @@ enum {
>  	MAX_SEND_CQE              = 16,
>  	UD_POST_RCV_COUNT         = 16,
>  	CM_POST_SRQ_COUNT         = 16,
> +
> +	SKB_TSHOLD		  = 256,
>  };
>
>  #define	IPOIB_OP_RECV   (1ul << 31)
> Index: ofa_1_3_dev_kernel/drivers/infiniband/ulp/ipoib/ipoib_cm.c
> ===================================================================
> --- ofa_1_3_dev_kernel.orig/drivers/infiniband/ulp/ipoib/ipoib_cm.c	2008-02-18 19:23:23.000000000 +0200
> +++ ofa_1_3_dev_kernel/drivers/infiniband/ulp/ipoib/ipoib_cm.c	2008-02-18 22:21:01.000000000 +0200
> @@ -554,6 +554,7 @@ void ipoib_cm_handle_rx_wc(struct net_de
>  	u64 mapping[IPOIB_CM_RX_SG];
>  	int frags;
>  	int has_srq;
> +	struct sk_buff *small_skb;
>
>  	ipoib_dbg_data(priv, "cm recv completion: id %d, status: %d\n",
>  		       wr_id, wc->status);
> @@ -608,6 +609,20 @@ void ipoib_cm_handle_rx_wc(struct net_de
>  		}
>  	}
>
> +	if (wc->byte_len < SKB_TSHOLD) {
> +		int dlen = wc->byte_len - IPOIB_ENCAP_LEN;
> +
> +		small_skb = dev_alloc_skb(dlen);
> +		if (small_skb) {
> +			small_skb->protocol = ((struct ipoib_header *)skb->data)->proto;
> +			skb_copy_from_linear_data_offset(skb, IPOIB_ENCAP_LEN,
> +							 small_skb->data, dlen);
> +			skb_put(small_skb, dlen);
> +			skb = small_skb;
> +			goto copied;
> +		}
> +	}
> +
>  	frags = PAGE_ALIGN(wc->byte_len - min(wc->byte_len,
>  					      (unsigned)IPOIB_CM_HEAD_SIZE)) / PAGE_SIZE;
>
> @@ -634,6 +649,7 @@ void ipoib_cm_handle_rx_wc(struct net_de
>  	skb_reset_mac_header(skb);
>  	skb_pull(skb, IPOIB_ENCAP_LEN);
>
> +copied:
>  	dev->last_rx = jiffies;
>  	++dev->stats.rx_packets;
>  	dev->stats.rx_bytes += skb->len;
>