[PATCH] IPoIB: fix faulty list maintenance in path and neigh list

[PATCH] IPoIB: fix faulty list maintenance in path and neigh list

[Mike Marciniszyn]

From: Gary Leshner <gary.leshner-h88ZbnxC6KDQT0dZR+AlfA@public.gmane.org>

Added list_del to ipoib_mcast_free() and path_free() to remove the neigh from
the path's neigh_list link list before freeing the memory it consumes.
This makes the code consistent with ipoib_cm_handle_tx_wc() and other locations where the list_del() preceeds the call to ipoib_neigh_free().

Similarly, at list_del() now preceeds path_free() to remove the path from
ipoib_dev_priv's path_list.

Additionally, the fields neigh->ah and neigh->list were not being initialized
upon allocation.  The patch insures that these two remaining fields are
initialized correctly.

Signed-off-by: Mike Marciniszyn <mike.marciniszyn-h88ZbnxC6KDQT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/ulp/ipoib/ipoib_main.c      |    4 ++++
 drivers/infiniband/ulp/ipoib/ipoib_multicast.c |    1 +
 2 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c b/drivers/infiniband/ulp/ipoib/ipoib_main.c
index 9ff7bc7..ddd52f6 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
@@ -283,6 +283,7 @@ static void path_free(struct net_device *dev, struct ipoib_path *path)
 		if (neigh->ah)
 			ipoib_put_ah(neigh->ah);
 
+		list_del(&neigh->list);
 		ipoib_neigh_free(dev, neigh);
 	}
 
@@ -390,6 +391,7 @@ void ipoib_flush_paths(struct net_device *dev)
 		spin_unlock_irqrestore(&priv->lock, flags);
 		netif_tx_unlock_bh(dev);
 		wait_for_completion(&path->done);
+		list_del(&path->list);
 		path_free(dev, path);
 		netif_tx_lock_bh(dev);
 		spin_lock_irqsave(&priv->lock, flags);
@@ -882,12 +884,14 @@ struct ipoib_neigh *ipoib_neigh_alloc(struct neighbour *neighbour,
 	if (!neigh)
 		return NULL;
 
+	neigh->ah = NULL;
 	neigh->neighbour = neighbour;
 	neigh->dev = dev;
 	memset(&neigh->dgid.raw, 0, sizeof (union ib_gid));
 	*to_ipoib_neigh(neighbour) = neigh;
 	skb_queue_head_init(&neigh->queue);
 	ipoib_cm_set(neigh, NULL);
+	INIT_LIST_HEAD(&neigh->list);
 
 	return neigh;
 }
diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
index 3871ac6..05f4c9a 100644
--- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
+++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
@@ -86,6 +86,7 @@ static void ipoib_mcast_free(struct ipoib_mcast *mcast)
 		 */
 		if (neigh->ah)
 			ipoib_put_ah(neigh->ah);
+		list_del(&neigh->list);
 		ipoib_neigh_free(dev, neigh);
 	}
 

--
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] IPoIB: fix faulty list maintenance in path and neigh list

[Roland Dreier]

I'm not sure I understand what this patch is trying to do.

> Added list_del to ipoib_mcast_free() and path_free() to remove the neigh from
> the path's neigh_list link list before freeing the memory it consumes.
> This makes the code consistent with ipoib_cm_handle_tx_wc() and other locations where the list_del() preceeds the call to ipoib_neigh_free().

It looks like this is not needed -- in both cases, the code walks a
list of neigh
structs and frees them, and then frees the list itself a few lines
later.  So what
does it fix to delete the neigh structs from the list?

> Similarly, at list_del() now preceeds path_free() to remove the path from
> ipoib_dev_priv's path_list.

Similarly in ipoib_flush_paths() the code is walking a local remove_list that
is on the stack and will be thrown away when the function returns -- why does
it matter if we remove the paths from that list before freeing them?

> Additionally, the fields neigh->ah and neigh->list were not being initialized
> upon allocation.  The patch insures that these two remaining fields are
> initialized correctly.

Again, I don't see how this fixes anything; neigh->list does not need to be
initialized, since it is only used to add the neigh struct to other
lists (AFAICT).
And also AFAICT, every place that allocates a neigh always initializes neigh->ah
on every code path.  It might be cleaner to set neigh->ah to NULL but I don't
see how this could fix something.

Pradeep seems to believe that this patch fixes some crashes but I would like
to understand the real cause before blindly applying this patch.
Which part(s) are
the real fix, and what is the race or other bug it fixes?

 - 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] IPoIB: fix faulty list maintenance in path and neigh list

[Pradeep Satyanarayana]

On 02/15/2011 10:24 AM, Roland Dreier wrote:
> I'm not sure I understand what this patch is trying to do.
>
>> Added list_del to ipoib_mcast_free() and path_free() to remove the neigh from
>> the path's neigh_list link list before freeing the memory it consumes.
>> This makes the code consistent with ipoib_cm_handle_tx_wc() and other locations where the list_del() preceeds the call to ipoib_neigh_free().
>
> It looks like this is not needed -- in both cases, the code walks a
> list of neigh
> structs and frees them, and then frees the list itself a few lines
> later.  So what
> does it fix to delete the neigh structs from the list?

Consider the following two threads:

.ipoib_mcast_free+0x48/0x308 [ib_ipoib]
.ipoib_mcast_restart_task+0x3d0/0x560 [ib_ipoib]
.worker_thread+0x1b4/0x330
.kthread+0xc4/0xd0
.kernel_thread+0x54/0x70



.ipoib_mcast_free+0x48/0x308 [ib_ipoib]
.ipoib_mcast_dev_flush+0x188/0x1f8 [ib_ipoib]
.ipoib_ib_dev_down+0x94/0x160 [ib_ipoib]
.ipoib_stop+0x78/0x178 [ib_ipoib]
.dev_close+0xdc/0x148

The list_move_tail() in ipoib_mcast_restart_task() is conditional. So it
feasible that mcast->list is not moved to the remove_list, but the 
ipoib_neigh structure is freed. A subsequent call to ipoib_mcast_free() 
from context of dev_close() tries to free the same ipoib_neigh() 
structure which might cause the crash.

>
>> Similarly, at list_del() now preceeds path_free() to remove the path from
>> ipoib_dev_priv's path_list.
>
> Similarly in ipoib_flush_paths() the code is walking a local remove_list that
> is on the stack and will be thrown away when the function returns -- why does
> it matter if we remove the paths from that list before freeing them?
>
>> Additionally, the fields neigh->ah and neigh->list were not being initialized
>> upon allocation.  The patch insures that these two remaining fields are
>> initialized correctly.
>
> Again, I don't see how this fixes anything; neigh->list does not need to be
> initialized, since it is only used to add the neigh struct to other
> lists (AFAICT).
> And also AFAICT, every place that allocates a neigh always initializes neigh->ah
> on every code path.  It might be cleaner to set neigh->ah to NULL but I don't
> see how this could fix something.
>
> Pradeep seems to believe that this patch fixes some crashes but I would like
> to understand the real cause before blindly applying this patch.
> Which part(s) are
> the real fix, and what is the race or other bug it fixes?

I hope I have addressed the questions regarding the need for a 
list_del() in ipoib_mcast_free(). I presume a similar logic applies to
ipoib_flush_paths().

Thanks
Pradeep
--
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] IPoIB: fix faulty list maintenance in path and neigh list

[Roland Dreier]

On Wed, Feb 16, 2011 at 4:51 PM, Pradeep Satyanarayana
<pradeeps-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> wrote:
> The list_move_tail() in ipoib_mcast_restart_task() is conditional. So it
> feasible that mcast->list is not moved to the remove_list, but the
> ipoib_neigh structure is freed. A subsequent call to ipoib_mcast_free() from
> context of dev_close() tries to free the same ipoib_neigh() structure which
> might cause the crash.

I'm missing something.  ipoib_neigh_free() is only called in
ipoib_mcast_free().  ipoib_mcast_free() is only called on entries in
remove_list.  So if an mcast structure is not moved to the
remove_list, I don't see how an ipoib_neigh struct on its list could
be freed.

We do all manipulation of the mcast list in ipoib_mcast_restart_task()
and ipoib_mcast_dev_flush() with priv->lock held, so I don't see how
we could free the same mcast struct twice, or free an ipoib_neigh
struct without freeing its corresponding mcast struct.

Indeed it's not clear to me why we need to take the lock in
ipoib_mcast_free() or what it could be protecting against -- that
seems like it must be wrong and unnecessary (and the locking predates
git history).

Sorry if I'm being slow but if this patch is fixing some test, it
really seems that it must be doing that by making a race window
smaller or something like that, rather than actually being the right
fix for the underlying problem.

 - 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] IPoIB: fix faulty list maintenance in path and neigh list

[Pradeep Satyanarayana]

On 02/16/2011 05:15 PM, Roland Dreier wrote:
> On Wed, Feb 16, 2011 at 4:51 PM, Pradeep Satyanarayana
> <pradeeps-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>  wrote:
>> The list_move_tail() in ipoib_mcast_restart_task() is conditional. So it
>> feasible that mcast->list is not moved to the remove_list, but the
>> ipoib_neigh structure is freed. A subsequent call to ipoib_mcast_free() from
>> context of dev_close() tries to free the same ipoib_neigh() structure which
>> might cause the crash.
>
> I'm missing something.  ipoib_neigh_free() is only called in
> ipoib_mcast_free().  ipoib_mcast_free() is only called on entries in
> remove_list.  So if an mcast structure is not moved to the
> remove_list, I don't see how an ipoib_neigh struct on its list could
> be freed.

Yes, that is the crux of the issue. I had missed that ipoib_mcast_free() 
is only called on remove_list.

>
> We do all manipulation of the mcast list in ipoib_mcast_restart_task()
> and ipoib_mcast_dev_flush() with priv->lock held, so I don't see how
> we could free the same mcast struct twice, or free an ipoib_neigh
> struct without freeing its corresponding mcast struct.
>
> Indeed it's not clear to me why we need to take the lock in
> ipoib_mcast_free() or what it could be protecting against -- that
> seems like it must be wrong and unnecessary (and the locking predates
> git history).
>
> Sorry if I'm being slow but if this patch is fixing some test, it
> really seems that it must be doing that by making a race window
> smaller or something like that, rather than actually being the right
> fix for the underlying problem.

While we are discussing IPoIB issues, how about the two other issues 
that I illustrated previously. One was Ralph Campbell's patch for fixes 
to ipoib_cm_start_rx_drain() and my questions wrt ipoib_neigh_cleanup()?

Thanks
Pradeep
--
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] IPoIB: fix faulty list maintenance in path and neigh list

[Roland Dreier]

> Yes, that is the crux of the issue. I had missed that ipoib_mcast_free() is
> only called on remove_list.

So do we have any idea of what this patch is fixing?  Any thoughts from
the qlogic people involved in this patch?

> While we are discussing IPoIB issues, how about the two other issues that
> I illustrated previously. One was Ralph Campbell's patch for fixes to
> ipoib_cm_start_rx_drain() and my questions wrt ipoib_neigh_cleanup()?

I do need to take a good look at Ralph's patches to try and understand them
and I hope apply them.  Not sure I still have any link to your questions though.

 - 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] IPoIB: fix faulty list maintenance in path and neigh list

[Mike Marciniszyn]

We too have had installability, perhaps associated with these lists, but it has been difficult to diagnose.

We duplicate it by forcing dropped packets and seeing the QP's come/go at the rate of 1000s a second because of the 0 rnr_retry and retry counts.  This analysis is in line behind other bug investigations.

The list patch was a result of code inspection.

Ralph's patch predates me.   His appears to move some list inserts to before a post, I'm assuming since an intervening completion could occur, but I haven't studied it in detail to see if any locking prevents it.

I would be interested in Pradeep's test (OS, Hardware, scripts...)

Mike

-----Original Message-----
From: roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org [mailto:roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org] On Behalf Of Roland Dreier
Sent: Thursday, February 17, 2011 6:24 PM
To: Pradeep Satyanarayana
Cc: Mike Marciniszyn; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Gary Leshner; Tom Elken
Subject: Re: [PATCH] IPoIB: fix faulty list maintenance in path and neigh list

> Yes, that is the crux of the issue. I had missed that ipoib_mcast_free() is
> only called on remove_list.

So do we have any idea of what this patch is fixing?  Any thoughts from
the qlogic people involved in this patch?

> While we are discussing IPoIB issues, how about the two other issues that
> I illustrated previously. One was Ralph Campbell's patch for fixes to
> ipoib_cm_start_rx_drain() and my questions wrt ipoib_neigh_cleanup()?

I do need to take a good look at Ralph's patches to try and understand them
and I hope apply them.  Not sure I still have any link to your questions though.

 - R.


This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.

--
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] IPoIB: fix faulty list maintenance in path and neigh list

[Pradeep Satyanarayana]

On 02/17/2011 03:34 PM, Mike Marciniszyn wrote:
> We too have had installability, perhaps associated with these lists, but it has been difficult to diagnose.
>
> We duplicate it by forcing dropped packets and seeing the QP's come/go at the rate of 1000s a second because of the 0 rnr_retry and retry counts.  This analysis is in line behind other bug investigations.
>
> The list patch was a result of code inspection.
>
> Ralph's patch predates me.   His appears to move some list inserts to before a post, I'm assuming since an intervening completion could occur, but I haven't studied it in detail to see if any locking prevents it.
>
> I would be interested in Pradeep's test (OS, Hardware, scripts...)

As described in one of my previous mails (in the url given below):

The test is basically to run netperf in a loop from several client 
machines to a server. The server is unloading and reloading the modules 
(basically do an "openibd restart") at random times. The crashes 
recreate in several hours. I used some of the large IBM servers. They 
did not seem to recreate on say smaller blades.

>
> Mike
>
> -----Original Message-----
> From: roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org [mailto:roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org] On Behalf Of Roland Dreier
> Sent: Thursday, February 17, 2011 6:24 PM
> To: Pradeep Satyanarayana
> Cc: Mike Marciniszyn; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Gary Leshner; Tom Elken
> Subject: Re: [PATCH] IPoIB: fix faulty list maintenance in path and neigh list
>
>> Yes, that is the crux of the issue. I had missed that ipoib_mcast_free() is
>> only called on remove_list.
>
> So do we have any idea of what this patch is fixing?  Any thoughts from
> the qlogic people involved in this patch?
>
>> While we are discussing IPoIB issues, how about the two other issues that
>> I illustrated previously. One was Ralph Campbell's patch for fixes to
>> ipoib_cm_start_rx_drain() and my questions wrt ipoib_neigh_cleanup()?
>
> I do need to take a good look at Ralph's patches to try and understand them
> and I hope apply them.  Not sure I still have any link to your questions though.

Here is the link to the detailed mail I sent:
http://www.spinics.net/lists/linux-rdma/msg07352.html

Thanks
Pradeep
--
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] IPoIB: fix faulty list maintenance in path and neigh list

[Pradeep Satyanarayana]

Resending since this got rejected previously as spam.

Thanks
Pradeep
On 02/10/2011 01:18 PM, Pradeep Satyanarayana wrote:
 > On 02/01/2011 08:12 AM, Mike Marciniszyn wrote:
 >> From: Gary Leshner <gary.leshner-h88ZbnxC6KDQT0dZR+AlfA@public.gmane.org>
 >>
 >> Added list_del to ipoib_mcast_free() and path_free() to remove the 
neigh from
 >> the path's neigh_list link list before freeing the memory it consumes.
 >> This makes the code consistent with ipoib_cm_handle_tx_wc() and 
other locations where the list_del() preceeds the call to 
ipoib_neigh_free().
 >>
 >> Similarly, at list_del() now preceeds path_free() to remove the path 
from
 >> ipoib_dev_priv's path_list.
 >>
 >> Additionally, the fields neigh->ah and neigh->list were not being 
initialized
 >> upon allocation. The patch insures that these two remaining fields are
 >> initialized correctly.
 >>
 >> Signed-off-by: Mike Marciniszyn <mike.marciniszyn-h88ZbnxC6KDQT0dZR+AlfA@public.gmane.org>
 >> ---
 >> drivers/infiniband/ulp/ipoib/ipoib_main.c | 4 ++++
 >> drivers/infiniband/ulp/ipoib/ipoib_multicast.c | 1 +
 >> 2 files changed, 5 insertions(+), 0 deletions(-)
 >>
 >> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c 
b/drivers/infiniband/ulp/ipoib/ipoib_main.c
 >> index 9ff7bc7..ddd52f6 100644
 >> --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
 >> +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
 >> @@ -283,6 +283,7 @@ static void path_free(struct net_device *dev, 
struct ipoib_path *path)
 >> if (neigh->ah)
 >> ipoib_put_ah(neigh->ah);
 >>
 >> + list_del(&neigh->list);
 >> ipoib_neigh_free(dev, neigh);
 >> }
 >>
 >> @@ -390,6 +391,7 @@ void ipoib_flush_paths(struct net_device *dev)
 >> spin_unlock_irqrestore(&priv->lock, flags);
 >> netif_tx_unlock_bh(dev);
 >> wait_for_completion(&path->done);
 >> + list_del(&path->list);
 >> path_free(dev, path);
 >> netif_tx_lock_bh(dev);
 >> spin_lock_irqsave(&priv->lock, flags);
 >> @@ -882,12 +884,14 @@ struct ipoib_neigh *ipoib_neigh_alloc(struct 
neighbour *neighbour,
 >> if (!neigh)
 >> return NULL;
 >>
 >> + neigh->ah = NULL;
 >> neigh->neighbour = neighbour;
 >> neigh->dev = dev;
 >> memset(&neigh->dgid.raw, 0, sizeof (union ib_gid));
 >> *to_ipoib_neigh(neighbour) = neigh;
 >> skb_queue_head_init(&neigh->queue);
 >> ipoib_cm_set(neigh, NULL);
 >> + INIT_LIST_HEAD(&neigh->list);
 >>
 >> return neigh;
 >> }
 >> diff --git a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c 
b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
 >> index 3871ac6..05f4c9a 100644
 >> --- a/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
 >> +++ b/drivers/infiniband/ulp/ipoib/ipoib_multicast.c
 >> @@ -86,6 +86,7 @@ static void ipoib_mcast_free(struct ipoib_mcast 
*mcast)
 >> */
 >> if (neigh->ah)
 >> ipoib_put_ah(neigh->ah);
 >> + list_del(&neigh->list);
 >> ipoib_neigh_free(dev, neigh);
 >> }
 >>
 >>
 >> --
 >
 > Roland,
 >
 > To provide a little more context regarding these patches, I have seen 
some sporadic crashes (shown below) that occur pretty
 > infrequently. I came to a very similar conclusion as the patches 
above to fix my problems. These are rarely occurring races that
 > recreate infrequently on large systems. The test is basically to run 
netperf in a loop from several client machines to a server.
 > The server is unloading and reloading the modules (basically do an 
"openibd restart") at random times. The crashes recreate
 > after several hours of run.
 >
 > There are three types of crashes that we have seen that are described 
below.
 >
 > Crash #1 (fixed by Ralph Campbell's patch)
 >
 > 12:mon> t
 > [link register   ] d00000000ec950a8 .cm_destroy_id+0x3d8/0x520 [ib_cm]
 > [c0000007544d2d40] d00000000ec95098 .cm_destroy_id+0x3c8/0x520 [ib_cm]
 > (unreliable)
 > [c0000007544d2e10] d00000000ef9b590 
.ipoib_cm_free_rx_reap_list+0xc8/0x180
 > [ib_ipoib]
 > [c0000007544d2ed0] d00000000ef9e1cc .ipoib_cm_dev_stop+0x23c/0x360 
[ib_ipoib]
 > [c0000007544d2f90] d00000000ef94d94 .ipoib_ib_dev_stop+0xe4/0x4b0 
[ib_ipoib]
 > [c0000007544d30f0] d00000000ef91d68 .ipoib_stop+0x88/0x178 [ib_ipoib]
 > [c0000007544d3180] c0000000004ea1cc .dev_close+0xdc/0x148
 > [c0000007544d3210] c0000000004e9790 .dev_change_flags+0x1f0/0x288
 > [c0000007544d32b0] c0000000004f6a0c .do_setlink+0x2d4/0x498
 > [c0000007544d3390] c0000000004f8b20 .rtnl_newlink+0x520/0x5f0
 > [c0000007544d35a0] c0000000004f853c .rtnetlink_rcv_msg+0x24c/0x310
 > [c0000007544d3650] c000000000513a38 .netlink_rcv_skb+0xf0/0x128
 > [c0000007544d36e0] c0000000004f82cc .rtnetlink_rcv+0x34/0x58
 > [c0000007544d3770] c0000000005134c0 .netlink_unicast+0x498/0x4b0
 > [c0000007544d3840] c000000000514218 .netlink_sendmsg+0x288/0x390
 > [c0000007544d3920] c0000000004cf538 .sock_sendmsg+0x118/0x158
 > [c0000007544d3b30] c0000000004cf720 .SyS_sendmsg+0x1a8/0x348
 > [c0000007544d3d70] c0000000004cdbdc .SyS_socketcall+0x174/0x338
 > [c0000007544d3e30] c0000000000085b4 syscall_exit+0x0/0x40
 > --- Exception: c01 (System Call) at 00000fffa25c35d0
 > SP (fffc5a12fe0) is in userspace
 > 12:mon> e
 > cpu 0x12: Vector: 300 (Data Access) at [c0000007544d2ac0]
 >     pc: c0000000002ef854: .rb_erase+0x64/0x180
 >     lr: d00000000ec950a8: .cm_destroy_id+0x3d8/0x520 [ib_cm]
 >     sp: c0000007544d2d40
 >    msr: 8000000000001432
 >    dar: 10000000100
 >  dsisr: 40000000
 >   current = 0xc0000006948dc240
 >   paca    = 0xc000000000f64a00
 >     pid   = 21593, comm = ip
 > 12:mon>
 >
 > My inference is that the above crash is indeed fixed by Ralph 
Campbell's patch described in :
 > http://www.mail-archive.com/linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg05842.html
 >
 > and for the reasons that he ascribes.
 >
 > IB/ipoib: fix race when handling IPOIB_CM_RX_DRAIN_WRID
 >
 > From: Ralph Campbell <ralph.campb...-h88ZbnxC6KDQT0dZR+AlfA@public.gmane.org>
 >
 > ipoib_cm_start_rx_drain() calls ib_post_send() and *then* moves the
 > struct ipoib_cm_rx onto the rx_drain_list. The ib_post_send() will
 > trigger a completion callback to ipoib_cm_handle_rx_wc() which
 > tries to move the rx_drain_list to the rx_reap_list but if the
 > callback happens before ipoib_cm_start_rx_drain() has moved the
 > structure, it is left in limbo. The fix is to change
 > ipoib_cm_start_rx_drain() to put the struct on the rx_drain_list and
 > then call ib_post_send().
 > Also, only move one struct from rx_flush_list to rx_drain_list since
 > concurrent IPOIB_CM_RX_DRAIN_WRID events on different QPs could put
 > multiple ipoib_cm_rx structs on rx_flush_list.
 >
 > Signed-off-by: Ralph Campbell <ralph.campb...-h88ZbnxC6KDQT0dZR+AlfA@public.gmane.org>
 > ---
 >
 > drivers/infiniband/ulp/ipoib/ipoib_cm.c | 12 +++++++++---
 > 1 files changed, 9 insertions(+), 3 deletions(-)
 >
 >
 > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_cm.c 
b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
 > index bb10041..dfff159 100644
 > --- a/drivers/infiniband/ulp/ipoib/ipoib_cm.c
 > +++ b/drivers/infiniband/ulp/ipoib/ipoib_cm.c
 > @@ -216,15 +216,21 @@ static void ipoib_cm_start_rx_drain(struct 
ipoib_dev_priv *priv)
 > !list_empty(&priv->cm.rx_drain_list))
 > return;
 >
 > + p = list_entry(priv->cm.rx_flush_list.next, typeof(*p), list);
 > +
 > + /*
 > + * Put p on rx_drain_list before calling ib_post_send() or there
 > + * is a race with the ipoib_cm_handle_rx_wc() completion handler
 > + * trying to remove it from rx_drain_list.
 > + */
 > + list_move(&p->list, &priv->cm.rx_drain_list);
 > +
 > /*
 > * QPs on flush list are error state. This way, a "flush
 > * error" WC will be immediately generated for each WR we post.
 > */
 > - p = list_entry(priv->cm.rx_flush_list.next, typeof(*p), list);
 > if (ib_post_send(p->qp, &ipoib_cm_rx_drain_wr, &bad_wr))
 > ipoib_warn(priv, "failed to post drain wr\n");
 > -
 > - list_splice_init(&priv->cm.rx_flush_list, &priv->cm.rx_drain_list);
 > }
 >
 > static void ipoib_cm_rx_event_handler(struct ib_event *event, void *ctx)
 >
 > Once I applied the above patch, the following crash showed up after 
almost a
 > 24 hour run:
 >
 > Crash #2 (fixed by Mike Marciniszn's patch)
 >
 > c957f1ec01 login: cpu 0x26: Vector: 300 (Data Access) at 
[c0000016b8f4f3e0]
 >     pc: d000000018c919c8: .ipoib_neigh_free+0x30/0xe0 [ib_ipoib]
 >     lr: d000000018c96e0c: .ipoib_mcast_free+0xac/0x300 [ib_ipoib]
 >     sp: c0000016b8f4f660
 >    msr: 8000000000009032
 >    dar: 58
 >  dsisr: 42000000
 >   current = 0xc0000016b7958900
 >   paca    = 0xc000000000f67200
 >     pid   = 16926, comm = modprobe
 > enter ? for help
 > [c0000016b8f4f700] d000000018c96e0c .ipoib_mcast_free+0xac/0x300 
[ib_ipoib]
 > [c0000016b8f4f7d0] d000000018c971e8 
.ipoib_mcast_dev_flush+0x188/0x1f8 [ib_ipoib]
 > [c0000016b8f4f8b0] d000000018c9521c .ipoib_ib_dev_down+0x94/0x160 
[ib_ipoib]
 > [c0000016b8f4f960] d000000018c90f20 .ipoib_stop+0x78/0x178 [ib_ipoib]
 > [c0000016b8f4f9f0] c0000000004eacf4 .dev_close+0xdc/0x148
 > [c0000016b8f4fa80] c0000000004ea2b8 .dev_change_flags+0x1f0/0x288
 > [c0000016b8f4fb20] d000000018c911b8 .ipoib_remove_one+0xb8/0x140 
[ib_ipoib]
 > [c0000016b8f4fbc0] d00000000d52425c .ib_unregister_client+0xb4/0x1b8 
[ib_core]
 > [c0000016b8f4fc90] d000000018c9fdb8 .ipoib_cleanup_module+0x20/0x60 
[ib_ipoib]
 > [c0000016b8f4fd20] c0000000000ec408 .SyS_delete_module+0x238/0x320
 > [c0000016b8f4fe30] c0000000000085b4 syscall_exit+0x0/0x40
 > --- Exception: c01 (System Call) at 00000fff983128e0
 > SP (fffe9636ad0) is in userspace
 > 26:mon
 >
 > This crash is caused by the following two threads, both of which are 
trying to delete
 > the same ipoib_neigh structure:
 >
 > .ipoib_mcast_free+0x48/0x308 [ib_ipoib]
 > .ipoib_mcast_restart_task+0x3d0/0x560 [ib_ipoib]
 > .worker_thread+0x1b4/0x330
 > .kthread+0xc4/0xd0
 > .kernel_thread+0x54/0x70
 >
 >
 > .ipoib_mcast_free+0x48/0x308 [ib_ipoib]
 > .ipoib_mcast_dev_flush+0x188/0x1f8 [ib_ipoib]
 > .ipoib_ib_dev_down+0x94/0x160 [ib_ipoib]
 > .ipoib_stop+0x78/0x178 [ib_ipoib]
 > .dev_close+0xdc/0x148
 >
 > This is because there is a list_del() missing which does not delete 
the ipoib_neigh structure from
 > the ipoib_mcast structure, resulting in a double free. And so Mike's 
patch fixed this issue.
 >
 > After I applied both the above patches I was able to run the tests 
for more than 24 hours and
 > here was the crash that was observed:
 >
 > Crash #3 (still to be resolved)
 >
 > <1>Unable to handle kernel paging request for data at address 0x00100108
 > <1>Faulting instruction address: 0xd000000012bd1b0c
 > 22:mon> e
 > cpu 0x22: Vector: 300 (Data Access) at [c000000e68636eb0]
 >     pc: d000000012bd1b0c: .ipoib_neigh_cleanup+0x84/0x178 [ib_ipoib]
 >     lr: d000000012bd1ae0: .ipoib_neigh_cleanup+0x58/0x178 [ib_ipoib]
 >     sp: c000000e68637130
 >    msr: 8000000000009032
 >    dar: 100108
 >  dsisr: 42000000
 >   current = 0xc000000f0590a820
 >   paca    = 0xc000000000f66a00
 >     pid   = 59548, comm = ip
 > 22:mon> t
 > [c000000e686371d0] c0000000004f4784 .neigh_cleanup_and_release+0x3c/0xa0
 > [c000000e68637250] c0000000004f55cc .neigh_flush_dev+0x14c/0x158
 > [c000000e68637310] c0000000004f5630 .neigh_ifdown+0x58/0x158
 > [c000000e686373c0] d00000000e92ed3c .addrconf_ifdown+0x7c/0x520 [ipv6]
 > [c000000e68637480] d00000000e92f470 .inet6_addr_del+0x148/0x168 [ipv6]
 > [c000000e68637520] d00000000e92f544 .inet6_rtm_deladdr+0xb4/0xb8 [ipv6]
 > [c000000e686375f0] c0000000004f9064 .rtnetlink_rcv_msg+0x24c/0x310
 > [c000000e686376a0] c000000000514580 .netlink_rcv_skb+0xf0/0x128
 > [c000000e68637730] c0000000004f8df4 .rtnetlink_rcv+0x34/0x58
 > [c000000e686377c0] c000000000514008 .netlink_unicast+0x498/0x4b0
 > [c000000e68637890] c000000000514d80 .netlink_sendmsg+0x288/0x390
 > [c000000e68637970] c0000000004d0060 .sock_sendmsg+0x118/0x158
 > [c000000e68637b80] c0000000004d12d8 .SyS_sendto+0xe8/0x138
 > [c000000e68637d00] c0000000004ce548 .SyS_send+0x20/0x38
 > [c000000e68637d70] c0000000004ce7fc .SyS_socketcall+0x26c/0x338
 > [c000000e68637e30] c0000000000085b4 syscall_exit+0x0/0x40
 > --- Exception: c01 (System Call) at 00000fffb3cc3524
 > SP (fffec5b9fc0) is in userspace
 >
 > The suspicion is that this is again a case of double free. Again, the 
double free
 > does not happen all the time, but is a race. The two suspected 
threads are:
 >
 > ipoib_neigh_free()
 > path_free()
 > ipoib_flush_paths()
 > ipoib_ib_dev_down()
 > ....
 >
 >
 > ipoib_neigh_free()
 > ipoib_neigh_cleanup()
 > neigh_cleanup_and_release()
 > neigh_ifdown()
 > ....
 >
 > In order to confirm this, I wanted to write a pattern in 
ipoib_neigh_free() that we could
 > look in ipoib_neigh_cleanup(). Also made some small changes in 
ipoib_start_xmit() too.
 >
 > void ipoib_neigh_free(struct net_device *dev, struct ipoib_neigh *neigh)
 > {
 >         struct sk_buff *skb;
 >         *to_ipoib_neigh(neigh->neighbour) = 0xdeaddeed;
 >         dump_stack();
 > #if 0
 >         *to_ipoib_neigh(neigh->neighbour) = NULL;
 > #endif
 >         while ((skb = __skb_dequeue(&neigh->queue))) {
 >                 ++dev->stats.tx_dropped;
 >                 dev_kfree_skb_any(skb);
 >         }
 >         if (ipoib_cm_get(neigh))
 >                 ipoib_cm_destroy_tx(ipoib_cm_get(neigh));
 >         kfree(neigh);
 > }
 >
 > I changed ipoib_neigh_cleanup() as follows:
 >
 > static void ipoib_neigh_cleanup(struct neighbour *n)
 > {
 >         struct ipoib_neigh *neigh;
 >         struct ipoib_dev_priv *priv = netdev_priv(n->dev);
 >         unsigned long flags;
 >         struct ipoib_ah *ah = NULL;
 >
 >         neigh = *to_ipoib_neigh(n);
 >         if ((neigh == 0xdeaddeed ) || (neigh == NULL)) { /* in that 
case ipoib_neigh has been freed and we need to crash */
 >                 printk(KERN_WARNING "ipoib_neigh_cleanup(): for 
neigh=0x%p %06x %pI6\n", neigh,
 >                         IPOIB_QPN(n->ha), n->ha + 4);
 >                 BUG();
 >         }
 >
 > ...
 >
 > The primary problem is that access to *to_ipoib_neigh(n) is 
unprotected. Hence the race between the two threads since
 > ipoib_neigh_cleanup() may have a stale pointer to the ipoib_neigh 
structure and hence the crash.
 >
 > With the above changes here is what is seen in the crash dump:
 >
 >
 > <4>Call Trace:
 > <4>[c000000001eff6b0] [c0000000000129e4] .show_stack+0x6c/0x198 
(unreliable)
 > <4>[c000000001eff760] [d0000000139019f0] .ipoib_neigh_free+0x48/0x100 
[ib_ipoib]
 > <4>[c000000001eff800] [d0000000139028c8] 
.ipoib_start_xmit+0x1a8/0x590 [ib_ipoib]
 > <4>[c000000001eff8c0] [c0000000004e9228] .dev_hard_start_xmit+0x210/0x2b0
 > <4>[c000000001eff970] [c000000000506ff8] .sch_direct_xmit+0x260/0x2d0
 > <4>[c000000001effa20] [c0000000004e98ec] .dev_queue_xmit+0x484/0x648
 > <4>[c000000001effad0] [c0000000004f26e4] 
.neigh_connected_output+0x10c/0x158
 > <4>[c000000001effb70] [d00000000e9233c0] 
.ip6_output_finish+0xe0/0x1a0 [ipv6]
 > <4>[c000000001effc10] [d00000000e94bbd4] .mld_sendpack+0x5b4/0x610 [ipv6]
 > <4>[c000000001effd30] [d00000000e94d1e8] 
.mld_ifc_timer_expire+0x18/0x98 [ipv6]
 > <4>[c000000001effdb0] [c0000000000bd424] .run_timer_softirq+0x214/0x348
 > <4>[c000000001effeb0] [c0000000000b6b8c] .__do_softirq+0x13c/0x240
 > <4>[c000000001efff90] [c000000000030afc] .call_do_softirq+0x14/0x24
 > <4>[c0000007580278e0] [c00000000000ed38] .do_softirq+0xe8/0x108
 > <4>[c000000758027980] [c0000000000b6854] .irq_exit+0xb4/0xe8
 > <4>[c000000758027a00] [c00000000002d03c] .timer_interrupt+0x104/0x138
 > <4>[c000000758027a90] [c000000000003718] decrementer_common+0x118/0x180
 > <4>--- Exception: 901 at .raw_local_irq_restore+0x70/0xc8
 > <4>    LR = .cpu_idle+0x98/0x1e0
 > <4>[c000000758027d80] [0000000000000014] 0x14 (unreliable)
 > <4>[c000000758027e10] [c000000000014b60] .cpu_idle+0x98/0x1e0
 > <4>[c000000758027ec0] [c0000000005b7bd4] .start_secondary+0x394/0x3dc
 > <4>[c000000758027f90] [c0000000000082d4] 
.start_secondary_prolog+0x10/0x14
 > <7>ib0: no IPv6 routers present
 > <7>ib3: no IPv6 routers present
 > <7>ib2: no IPv6 routers present
 > <7>ib1: no IPv6 routers present
 > <4>created rx qp=0x6d4
 > <4>ipoib_neigh_cleanup(): for neigh=0x00000000deaddeed ffffff 
ff12:601b:ffff:0000:0000:0000:0000:0016
 > <0>------------[ cut here ]------------
 > <2>kernel BUG at drivers/infiniband/ulp/ipoib/ipoib_main.c:863!
 > 22:mon> e
 > cpu 0x22: Vector: 700 (Program Check) at [c0000007585238e0]
 >     pc: d000000013901bd4: .ipoib_neigh_cleanup+0x12c/0x1d0 [ib_ipoib]
 >     lr: d000000013901bd0: .ipoib_neigh_cleanup+0x128/0x1d0 [ib_ipoib]
 >     sp: c000000758523b60
 >    msr: 8000000000029032
 >   current = 0xc0000007585009e0
 >   paca    = 0xc000000000f66a00
 >     pid   = 165, comm = events/34
 > kernel BUG at drivers/infiniband/ulp/ipoib/ipoib_main.c:863!
 > 22:mon> t
 > [c000000758523c00] c0000000004f4784 .neigh_cleanup_and_release+0x3c/0xa0
 > [c000000758523c80] c0000000004f4a34 .neigh_periodic_work+0x144/0x210
 > [c000000758523d40] c0000000000c7704 .run_workqueue+0xf4/0x1e0
 > [c000000758523e00] c0000000000c78b0 .worker_thread+0xc0/0x180
 > [c000000758523ed0] c0000000000cd6ac .kthread+0xc4/0xd0
 > [c000000758523f90] c000000000030e04 .kernel_thread+0x54/0x70
 > 22:mon>
 >
 >
 > My question is do we need to do any cleanup at all in 
ipoib_neigh_cleanup() or can it simply return,
 > since the ipoib_neigh_free() does it, or at best just call 
ipoib_put_ah()?
 >
 > Thanks
 > Pradeep

--
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