Re: [PATCH][ROSE][AX25] af_ax25: possible circular locking

[David Miller <davem@davemloft.net>]

From: Jarek Poplawski <jarkao2@gmail.com>
Date: Sun, 30 Dec 2007 15:13:23 +0100

> On Sat, Dec 29, 2007 at 07:14:43PM -0800, David Miller wrote:
> ...
> > You can't just drop this linked list lock and expect it to stay
> > consistent like that.
> > 
> > Once you drop it, any thread of control can get in there and delete
> > entries from the list.
> > 
> > Since we know it can happen, using a WARN_ON_ONCE(1) is not
> > appropriate.
> 
> The problem is 'we' don't know if it can happen... In the first
> message with this patch I've tried to get this information, and
> now it seems you are the only one with this knowledge, but of
> course this is more than enough for me to agree with your decision
> to dump this patch.

I've removed the warning and made the branch back to 'again'
unconditional as I think this is the safest version of the
change.

I'll push this upstream, thanks for fixing this Jarek.

diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index ecb14ee..b4725ff 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -87,10 +87,22 @@ static void ax25_kill_by_device(struct net_device *dev)
 		return;
 
 	spin_lock_bh(&ax25_list_lock);
+again:
 	ax25_for_each(s, node, &ax25_list) {
 		if (s->ax25_dev == ax25_dev) {
 			s->ax25_dev = NULL;
+			spin_unlock_bh(&ax25_list_lock);
 			ax25_disconnect(s, ENETUNREACH);
+			spin_lock_bh(&ax25_list_lock);
+
+			/* The entry could have been deleted from the
+			 * list meanwhile and thus the next pointer is
+			 * no longer valid.  Play it safe and restart
+			 * the scan.  Forward progress is ensured
+			 * because we set s->ax25_dev to NULL and we
+			 * are never passed a NULL 'dev' argument.
+			 */
+			goto again;
 		}
 	}
 	spin_unlock_bh(&ax25_list_lock);
-- 
1.5.4.rc2.84.gf85fd