[PATCH RFC] ipoib: good references make good neighbors

[PATCH RFC] ipoib: good references make good neighbors

[Chris Mason]

Hi everyone,

We're having a problem where a kernel tree based on 2.6.32 + OFED
1.5.1 is seeing random memory corruption, always in the form of zeros
where good data is supposed to live.

CONFIG_PAGE_DEBUG_ALLOC showed a use after free here:

RIP: 0010:[<ffffffffa02b0bf0>]  [<ffffffffa02b0bf0>] ipoib_neigh_free+0x16/0x59 [ib_ipoib]
Call Trace:
 [<ffffffffa02b4c9a>] ipoib_mcast_free+0x7a/0xfe [ib_ipoib]
 [<ffffffffa02b60bb>] ipoib_mcast_restart_task+0x388/0x419 [ib_ipoib]
 [<ffffffff810425b7>] ? need_resched+0x23/0x2d
 [<ffffffffa02b5d33>] ? ipoib_mcast_restart_task+0x0/0x419 [ib_ipoib]
 [<ffffffff81071765>] worker_thread+0x149/0x1e5
 [<ffffffff81075a0f>] ? autoremove_wake_function+0x0/0x3d
 [<ffffffff8107161c>] ? worker_thread+0x0/0x1e5
 [<ffffffff810757bb>] kthread+0x6e/0x76
 [<ffffffff81012daa>] child_rip+0xa/0x20
 [<ffffffff8107574d>] ? kthread+0x0/0x76
 [<ffffffff81012da0>] ? child_rip+0x0/0x20

The crashes usually pop up while rebooting (which rmmods ipoib), but we
were able to hit it consistently by reseting IB switches, or flipping
ports on and off.

Tina Yang noticed that when ipoib_neigh_alloc() takes a pointer to the
neighbour struct, it doesn't take any references.  I cooked up the patch
below and haven't been able to trigger our corruption since.

Signed-off-by: Chris Mason <chris.mason-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>

--- ofa_kernel-1.5.1/drivers/infiniband/ulp/ipoib/ipoib_main.c	2010-08-23 05:16:57.000000000 -0700
+++ ofa_kernel-1.5.1-refs/drivers/infiniband/ulp/ipoib/ipoib_main.c	2010-08-22 13:35:43.000000000 -0700
@@ -919,6 +919,7 @@ struct ipoib_neigh *ipoib_neigh_alloc(st
 	if (!neigh)
 		return NULL;
 
+	neigh_hold(neighbour);
 	neigh->neighbour = neighbour;
 	neigh->dev = dev;
 	memset(&neigh->dgid.raw, 0, sizeof (union ib_gid));
@@ -932,6 +933,7 @@ struct ipoib_neigh *ipoib_neigh_alloc(st
 void ipoib_neigh_free(struct net_device *dev, struct ipoib_neigh *neigh)
 {
 	struct sk_buff *skb;
+	struct neighbour *neighbour = neigh->neighbour;
 	*to_ipoib_neigh(neigh->neighbour) = NULL;
 	while ((skb = __skb_dequeue(&neigh->queue))) {
 		++dev->stats.tx_dropped;
@@ -940,6 +942,7 @@ void ipoib_neigh_free(struct net_device 
 	if (ipoib_cm_get(neigh))
 		ipoib_cm_destroy_tx(ipoib_cm_get(neigh));
 	kfree(neigh);
+	neigh_release(neighbour);
 }
 
 static int ipoib_neigh_setup_dev(struct net_device *dev, struct neigh_parms *parms)
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC] ipoib: good references make good neighbors

[Ralph Campbell]

The problem with this solution is that it creates
a reference counting "loop" so that the reference
count never goes to zero.
struct neighbour in the kernel points to struct ipoib_neigh
which points back to struct neighbor. If the "back pointer"
holds a reference, then something besides ipoib_neigh_free()
has to do the neigh_release(neighbour).

I think the real fix is the patch I sent to linux-rdma:
https://patchwork.kernel.org/patch/120013/


On Mon, 2010-08-23 at 12:53 -0700, Chris Mason wrote:
> Hi everyone,
> 
> We're having a problem where a kernel tree based on 2.6.32 + OFED
> 1.5.1 is seeing random memory corruption, always in the form of zeros
> where good data is supposed to live.
> 
> CONFIG_PAGE_DEBUG_ALLOC showed a use after free here:
> 
> RIP: 0010:[<ffffffffa02b0bf0>]  [<ffffffffa02b0bf0>] ipoib_neigh_free+0x16/0x59 [ib_ipoib]
> Call Trace:
>  [<ffffffffa02b4c9a>] ipoib_mcast_free+0x7a/0xfe [ib_ipoib]
>  [<ffffffffa02b60bb>] ipoib_mcast_restart_task+0x388/0x419 [ib_ipoib]
>  [<ffffffff810425b7>] ? need_resched+0x23/0x2d
>  [<ffffffffa02b5d33>] ? ipoib_mcast_restart_task+0x0/0x419 [ib_ipoib]
>  [<ffffffff81071765>] worker_thread+0x149/0x1e5
>  [<ffffffff81075a0f>] ? autoremove_wake_function+0x0/0x3d
>  [<ffffffff8107161c>] ? worker_thread+0x0/0x1e5
>  [<ffffffff810757bb>] kthread+0x6e/0x76
>  [<ffffffff81012daa>] child_rip+0xa/0x20
>  [<ffffffff8107574d>] ? kthread+0x0/0x76
>  [<ffffffff81012da0>] ? child_rip+0x0/0x20
> 
> The crashes usually pop up while rebooting (which rmmods ipoib), but we
> were able to hit it consistently by reseting IB switches, or flipping
> ports on and off.
> 
> Tina Yang noticed that when ipoib_neigh_alloc() takes a pointer to the
> neighbour struct, it doesn't take any references.  I cooked up the patch
> below and haven't been able to trigger our corruption since.
> 
> Signed-off-by: Chris Mason <chris.mason-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> 
> --- ofa_kernel-1.5.1/drivers/infiniband/ulp/ipoib/ipoib_main.c	2010-08-23 05:16:57.000000000 -0700
> +++ ofa_kernel-1.5.1-refs/drivers/infiniband/ulp/ipoib/ipoib_main.c	2010-08-22 13:35:43.000000000 -0700
> @@ -919,6 +919,7 @@ struct ipoib_neigh *ipoib_neigh_alloc(st
>  	if (!neigh)
>  		return NULL;
>  
> +	neigh_hold(neighbour);
>  	neigh->neighbour = neighbour;
>  	neigh->dev = dev;
>  	memset(&neigh->dgid.raw, 0, sizeof (union ib_gid));
> @@ -932,6 +933,7 @@ struct ipoib_neigh *ipoib_neigh_alloc(st
>  void ipoib_neigh_free(struct net_device *dev, struct ipoib_neigh *neigh)
>  {
>  	struct sk_buff *skb;
> +	struct neighbour *neighbour = neigh->neighbour;
>  	*to_ipoib_neigh(neigh->neighbour) = NULL;
>  	while ((skb = __skb_dequeue(&neigh->queue))) {
>  		++dev->stats.tx_dropped;
> @@ -940,6 +942,7 @@ void ipoib_neigh_free(struct net_device 
>  	if (ipoib_cm_get(neigh))
>  		ipoib_cm_destroy_tx(ipoib_cm_get(neigh));
>  	kfree(neigh);
> +	neigh_release(neighbour);
>  }
>  
>  static int ipoib_neigh_setup_dev(struct net_device *dev, struct neigh_parms *parms)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC] ipoib: good references make good neighbors

[Chris Mason]

On Mon, Aug 30, 2010 at 05:11:01PM -0700, Ralph Campbell wrote:

[ add a reference to neighbours inside ipoib ]

> The problem with this solution is that it creates
> a reference counting "loop" so that the reference
> count never goes to zero.
> struct neighbour in the kernel points to struct ipoib_neigh
> which points back to struct neighbor. If the "back pointer"
> holds a reference, then something besides ipoib_neigh_free()
> has to do the neigh_release(neighbour).
> 
> I think the real fix is the patch I sent to linux-rdma:
> https://patchwork.kernel.org/patch/120013/

patchwork is keeping this one secret for now:

OperationalError at /patch/120013/

(1040, 'Too many connections')

But I'll check back later today.  We'd hit this corruption at least once
a day under load, and we couldn't reboot a rack of machines without half
of them exploding as others went down (without CONFIG_PAGE_DEBUG_ALLOC)

So I'm a bit relieved that someone else has seen it too ;)

-chris
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC] ipoib: good references make good neighbors

[Roland Dreier]

 > I think the real fix is the patch I sent to linux-rdma:
 > https://patchwork.kernel.org/patch/120013/

Patchwork seems to be up now.  Chris, if you could try this patch out
and see if it fixes your issue that would be great, since you seem to be
able to reproduce the crashes pretty reliably.

 - R.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC] ipoib: good references make good neighbors

[Chris Mason]

On Thu, Sep 02, 2010 at 02:16:07PM -0700, Roland Dreier wrote:
>  > I think the real fix is the patch I sent to linux-rdma:
>  > https://patchwork.kernel.org/patch/120013/
> 
> Patchwork seems to be up now.  Chris, if you could try this patch out
> and see if it fixes your issue that would be great, since you seem to be
> able to reproduce the crashes pretty reliably.

Yeah, my test box is getting reimaged but I'll give it a shot when it
comes back.

-chris

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html