[PATCH][ATM]: [clip] fix race between modifying entry->vccs and clip_start_xmit()

[PATCH][ATM]: [clip] fix race between modifying entry->vccs and clip_start_xmit()

[chas williams]

please apply to 2.6 and 2.4 -- thanks

# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher.
# This patch includes the following deltas:
#	           ChangeSet	1.1315  -> 1.1316 
#	      net/atm/clip.c	1.22    -> 1.23   
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/09/15	chas@relax.cmf.nrl.navy.mil	1.1316
# [ATM]: [clip] fix race between modifying entry->vccs and clip_start_xmit()
# --------------------------------------------
#
diff -Nru a/net/atm/clip.c b/net/atm/clip.c
--- a/net/atm/clip.c	Mon Sep 15 15:08:57 2003
+++ b/net/atm/clip.c	Mon Sep 15 15:08:57 2003
@@ -93,6 +93,7 @@
 		printk(KERN_CRIT "!clip_vcc->entry (clip_vcc %p)\n",clip_vcc);
 		return;
 	}
+	spin_lock_bh(&entry->neigh->dev->xmit_lock);	/* block clip_start_xmit() */
 	entry->neigh->used = jiffies;
 	for (walk = &entry->vccs; *walk; walk = &(*walk)->next)
 		if (*walk == clip_vcc) {
@@ -102,17 +103,20 @@
 			clip_vcc->entry = NULL;
 			if (clip_vcc->xoff)
 				netif_wake_queue(entry->neigh->dev);
-			if (entry->vccs) return;
+			if (entry->vccs)
+				goto out;
 			entry->expires = jiffies-1;
 				/* force resolution or expiration */
 			error = neigh_update(entry->neigh,NULL,NUD_NONE,0,0);
 			if (error)
 				printk(KERN_CRIT "unlink_clip_vcc: "
 				    "neigh_update failed with %d\n",error);
-			return;
+			goto out;
 		}
 	printk(KERN_CRIT "ATMARP: unlink_clip_vcc failed (entry %p, vcc "
 	  "0x%p)\n",entry,clip_vcc);
+out:
+	spin_unlock_bh(&entry->neigh->dev->xmit_lock);
 }
 
 

Re: [PATCH][ATM]: [clip] fix race between modifying entry->vccs and clip_start_xmit()

[Francois Romieu]

chas williams <chas@cmf.nrl.navy.mil> :
[unlink_clip_vcc() locking change]

Afaik unlink_clip_vcc() can be called from a clip_push() issued in IRQ
context through vcc->push() in a device driver as well as from user-space
through vcc_ioctl()/ATMARP_SETENTRY. If the lock is taken by user-space
first, what avoids that unlink_clip_vcc(IRQ) deadlocks on it ?

No comment for the clip_start_xmit() part btw.

--
Ueimor

Re: [PATCH][ATM]: [clip] fix race between modifying entry->vccs and clip_start_xmit()

[chas williams]

In message <20030915225901.A22239@electric-eye.fr.zoreil.com>,Francois Romieu w
rites:
>Afaik unlink_clip_vcc() can be called from a clip_push() issued in IRQ
>context through vcc->push() in a device driver as well as from user-space
>through vcc_ioctl()/ATMARP_SETENTRY. If the lock is taken by user-space
>first, what avoids that unlink_clip_vcc(IRQ) deadlocks on it ?

i believe you are talking about:

        if (!skb) {
                DPRINTK("removing VCC %p\n",clip_vcc);
                if (clip_vcc->entry) unlink_clip_vcc(clip_vcc);
                clip_vcc->old_push(vcc,NULL); /* pass on the bad news */
                kfree(clip_vcc);
                return;
        }

this is triggered by vcc_destroy_sock(), which is part of vcc_release()
which is always going to be in user context.  its a bit subtle but
there is no path to unlink_clip_vcc() that isnt in user context.

>No comment for the clip_start_xmit() part btw.

actually, you dont need to modify that part.

Re: [PATCH][ATM]: [clip] fix race between modifying entry->vccs and clip_start_xmit()

[David S. Miller]

On Mon, 15 Sep 2003 17:59:02 -0400
chas williams <chas@cmf.nrl.navy.mil> wrote:

> this is triggered by vcc_destroy_sock(), which is part of vcc_release()
> which is always going to be in user context.  its a bit subtle but
> there is no path to unlink_clip_vcc() that isnt in user context.

Are you really totally sure that no interrupt path can release
a VCC?  That's not how I understood this stuff to work last time
I looked at it.

Re: [PATCH][ATM]: [clip] fix race between modifying entry->vccs and clip_start_xmit()

[David S. Miller]

On Mon, 15 Sep 2003 15:30:48 -0700
Mitchell Blank Jr <mitch@sfgoth.com> wrote:

> David S. Miller wrote:
> > Are you really totally sure that no interrupt path can release
> > a VCC?
> 
> That should be the case at least for any VCC on a real interface[*].
> Some ATM cards have ->close() methods that can take a while so connection
> teardown has to happen with some sort of sleepable context.  So if there
> are cases where an interrupt causes the VCC to die they would need to be
> fixed anyways.

Ok, I'm convinced. :)

I'll apply Chas's patch then, thanks.

Re: [PATCH][ATM]: [clip] fix race between modifying entry->vccs and clip_start_xmit()

[Mitchell Blank Jr]

David S. Miller wrote:
> > which is always going to be in user context.  its a bit subtle but
> > there is no path to unlink_clip_vcc() that isnt in user context.
> 
> Are you really totally sure that no interrupt path can release
> a VCC?

That should be the case at least for any VCC on a real interface[*].
Some ATM cards have ->close() methods that can take a while so connection
teardown has to happen with some sort of sleepable context.  So if there
are cases where an interrupt causes the VCC to die they would need to be
fixed anyways.

-Mitch

[*] some protocols use psuedo-interfaces for their control connections - in
    theory I guess they could be different but I don't believe there are
    any cases where they are.  They wouldn't affect this issue anyways though.

Re: [PATCH][ATM]: [clip] fix race between modifying entry->vccs and clip_start_xmit()

[chas williams]

In message <20030915223048.GD92642@gaz.sfgoth.com>,Mitchell Blank Jr writes:
>That should be the case at least for any VCC on a real interface[*].
>...
>[*] some protocols use psuedo-interfaces for their control connections - in
>    theory I guess they could be different but I don't believe there are
>    any cases where they are.  They wouldn't affect this issue anyways though.

afaict all vcc's are open/closed from user space including any control
vccs (often labelled ATM_VF_META).  this is a bit of a sore point with
some people who want to open atm sockets in the kernel.  if i remember
correctly the atm api explicitly states that you can sleep in atm_dev->close()
so it must be a user context.