[PATCH] ATM locking fix. [Re: [PATCH] spinlock not locked when unlocking in atm_dev_register]

[PATCH] ATM locking fix. [Re: [PATCH] spinlock not locked when unlocking in atm_dev_register]

[Maksim Krasnyanskiy]

Hi Folks,

Some time ago I promised a patch that fixes ATM device allocation code.
Fix that Alan's got in his 2.4.19-pre2-ac3 is wrong because it just removes 
locking instead of fixing it.
So here goes my patch. Tested on dual Athlon box.

Marcelo, Alan, please apply.

Patch URL:
	http://bluez.sf.net/atm.patch-2.4.19-pre2.gz


--- linux/net/atm/resources.c.orig	Thu Mar  7 13:56:00 2002
+++ linux/net/atm/resources.c	Thu Mar  7 13:56:40 2002
@@ -32,7 +32,7 @@
  {
  	struct atm_dev *dev;

-	dev = kmalloc(sizeof(*dev),GFP_KERNEL);
+	dev = kmalloc(sizeof(*dev), GFP_ATOMIC);
  	if (!dev) return NULL;
  	memset(dev,0,sizeof(*dev));
  	dev->type = type;
@@ -40,32 +40,24 @@
  	dev->link_rate = ATM_OC3_PCR;
  	dev->next = NULL;

-	spin_lock(&atm_dev_lock);
-
  	dev->prev = last_dev;

  	if (atm_devs) last_dev->next = dev;
  	else atm_devs = dev;
  	last_dev = dev;
-	spin_unlock(&atm_dev_lock);
  	return dev;
  }


  static void free_atm_dev(struct atm_dev *dev)
  {
-	spin_lock (&atm_dev_lock);
-	
  	if (dev->prev) dev->prev->next = dev->next;
  	else atm_devs = dev->next;
  	if (dev->next) dev->next->prev = dev->prev;
  	else last_dev = dev->prev;
  	kfree(dev);
-	
-	spin_unlock (&atm_dev_lock);
  }

-
  struct atm_dev *atm_find_dev(int number)
  {
  	struct atm_dev *dev;
@@ -75,17 +67,18 @@
  	return NULL;
  }

-
  struct atm_dev *atm_dev_register(const char *type,const struct atmdev_ops 
*ops,
      int number,atm_dev_flags_t *flags)
  {
-	struct atm_dev *dev;
+	struct atm_dev *dev = NULL;
+
+	spin_lock(&atm_dev_lock);

  	dev = alloc_atm_dev(type);
  	if (!dev) {
  		printk(KERN_ERR "atm_dev_register: no space for dev %s\n",
  		    type);
-		return NULL;
+		goto done;
  	}
  	if (number != -1) {
  		if (atm_find_dev(number)) {
@@ -93,8 +86,7 @@
  			return NULL;
  		}
  		dev->number = number;
-	}
-	else {
+	} else {
  		dev->number = 0;
  		while (atm_find_dev(dev->number)) dev->number++;
  	}
@@ -102,20 +94,23 @@
  	dev->dev_data = NULL;
  	barrier();
  	dev->ops = ops;
-	if (flags) dev->flags = *flags;
-	else memset(&dev->flags,0,sizeof(dev->flags));
+	if (flags)
+		dev->flags = *flags;
+	else
+		memset(&dev->flags,0,sizeof(dev->flags));
  	memset((void *) &dev->stats,0,sizeof(dev->stats));
  #ifdef CONFIG_PROC_FS
  	if (ops->proc_read)
  		if (atm_proc_dev_register(dev) < 0) {
  			printk(KERN_ERR "atm_dev_register: "
  			    "atm_proc_dev_register failed for dev %s\n",type);
-			spin_unlock (&atm_dev_lock);		
  			free_atm_dev(dev);
-			return NULL;
+			goto done;
  		}
  #endif
-	spin_unlock (&atm_dev_lock);		
+
+done:
+	spin_unlock(&atm_dev_lock);
  	return dev;
  }

@@ -125,10 +120,11 @@
  #ifdef CONFIG_PROC_FS
  	if (dev->ops->proc_read) atm_proc_dev_deregister(dev);
  #endif
+	spin_lock(&atm_dev_lock);
  	free_atm_dev(dev);
+	spin_unlock(&atm_dev_lock);
  }

-
  void shutdown_atm_dev(struct atm_dev *dev)
  {
  	if (dev->vccs) {
@@ -146,7 +142,6 @@
  	kfree(sk->protinfo.af_atm);
  }

-
  struct sock *alloc_atm_vcc_sk(int family)
  {
  	struct sock *sk;

Re: [PATCH] ATM locking fix. [Re: [PATCH] spinlock not locked when unlocking in atm_dev_register]

[Francois Romieu]

Greetings,

Maksim Krasnyanskiy <maxk@qualcomm.com> :
> --- linux/net/atm/resources.c.orig	Thu Mar  7 13:56:00 2002
> +++ linux/net/atm/resources.c	Thu Mar  7 13:56:40 2002
[...]
>   		if (atm_proc_dev_register(dev) < 0) {
>   			printk(KERN_ERR "atm_dev_register: "
>   			    "atm_proc_dev_register failed for dev %s\n",type);
> -			spin_unlock (&atm_dev_lock);		
>   			free_atm_dev(dev);   <====== (A)
> -			return NULL;
> +			goto done;
>   		}
>   #endif
> -	spin_unlock (&atm_dev_lock);
> +
> +done:
> +	spin_unlock(&atm_dev_lock); 
>   	return dev;                          <====== (B)
>   }

- dev = NULL is to be inserted between (A) and (B) or the caller won't 
  notice the failure
- atm_proc_dev_register issues kmalloc(...,GFP_KERNEL) and atm_dev_lock 
  is hold

-- 
Ueimor

Re: [PATCH] ATM locking fix. [Re: [PATCH] spinlock not locked when unlocking in atm_dev_register]

[Maksim Krasnyanskiy]

>- dev = NULL is to be inserted between (A) and (B) or the caller won't
>   notice the failure
Oops. Missed that one.

>- atm_proc_dev_register issues kmalloc(...,GFP_KERNEL) and atm_dev_lock
>   is hold
No. It uses GFP_ATOMIC.

Max

Re: [PATCH] ATM locking fix. [Re: [PATCH] spinlock not locked when unlocking in atm_dev_register]

[Francois Romieu]

Maksim Krasnyanskiy <maxk@qualcomm.com> :
[...]
> >- atm_proc_dev_register issues kmalloc(...,GFP_KERNEL) and atm_dev_lock
> >   is hold
> No. It uses GFP_ATOMIC.

atm_proc_dev_register
    ^^^^
2.4.18 - net/atm/proc.c:554
        dev->proc_name = kmalloc(strlen(dev->type)+digits+2,GFP_KERNEL);

> grep 'net/atm/proc' patch-2.4.19-pre2 -> nada
> grep 'net/atm/proc' patch-2.4.19-pre2-ac3 -> nada

I am alone in my parallel universe ? :o)

-- 
Ueimor 

Re: [PATCH] ATM locking fix.

[David S. Miller]

I'm applying this patch with some minor cleanups of my own.

I went and then checked around for atm_find_dev() users to
make sure they held the atm_dev_lock, and I discovered several pieces
of "hidden treasure".

Firstly, have a look at net/atm/common.c:atm_ioctl() and how it
accesses userspace while holding atm_dev_lock.  That is just the
tip of the iceberg.

ATM sorely needs a maintainer.  Any of the kernel janitors want to
learn how ATM works? :-))))

Re: [PATCH] ATM locking fix.

[Dave Jones]

On Tue, Mar 26, 2002 at 02:52:02PM -0800, David S. Miller wrote:
 > ATM sorely needs a maintainer.  Any of the kernel janitors want to
 > learn how ATM works? :-))))

Isn't someone maintaining it outside the tree somewhere ?
Or was that the hamradio stuff ?

-- 
| Dave Jones.        http://www.codemonkey.org.uk
| SuSE Labs

Re: [PATCH] ATM locking fix.

[David S. Miller]

   From: Dave Jones <davej@suse.de>
   Date: Wed, 27 Mar 2002 00:08:06 +0100

   On Tue, Mar 26, 2002 at 02:52:02PM -0800, David S. Miller wrote:
    > ATM sorely needs a maintainer.  Any of the kernel janitors want to
    > learn how ATM works? :-))))
   
   Isn't someone maintaining it outside the tree somewhere ?
   Or was that the hamradio stuff ?

If they are, they aren't sending me any patches or updates, which
effectively means it is still not maintained.

Re: [PATCH] ATM locking fix.

[Mr. James W. Laferriere]

	Hello Dave ,  I have attached the linux-atm list so they can see
	what you have found .  Hth ,  JimL

On Tue, 26 Mar 2002, David S. Miller wrote:

>
> I'm applying this patch with some minor cleanups of my own.
>
> I went and then checked around for atm_find_dev() users to
> make sure they held the atm_dev_lock, and I discovered several pieces
> of "hidden treasure".
>
> Firstly, have a look at net/atm/common.c:atm_ioctl() and how it
> accesses userspace while holding atm_dev_lock.  That is just the
> tip of the iceberg.
>
> ATM sorely needs a maintainer.  Any of the kernel janitors want to
> learn how ATM works? :-))))
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

       +------------------------------------------------------------------+
       | James   W.   Laferriere | System    Techniques | Give me VMS     |
       | Network        Engineer |     P.O. Box 854     |  Give me Linux  |
       | babydr@baby-dragons.com | Coudersport PA 16915 |   only  on  AXP |
       +------------------------------------------------------------------+

Re: [PATCH] ATM locking fix.

[Maksim Krasnyanskiy]

>I'm applying this patch with some minor cleanups of my own.
Ok. And don't forget a patch sent by Francois on top of mine.

>I went and then checked around for atm_find_dev() users to
>make sure they held the atm_dev_lock, and I discovered several pieces
>of "hidden treasure".
Hmm, I thought I check all of them.

>Firstly, have a look at net/atm/common.c:atm_ioctl() and how it
>accesses userspace while holding atm_dev_lock.  That is just the
>tip of the iceberg.
Yeah, that's what I said from the very beginning. ATM locking is messed up.

Max

Re: [PATCH] ATM locking fix.

[David S. Miller]

   From: Maksim Krasnyanskiy <maxk@qualcomm.com>
   Date: Wed, 27 Mar 2002 17:55:50 -0800
   
   >I'm applying this patch with some minor cleanups of my own.
   Ok. And don't forget a patch sent by Francois on top of mine.
   
Can you please send this to me privately so that I make sure I have
it?  Thanks.