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;
Greetings,
Maksim Krasnyanskiy <[email protected]> :
> --- 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
>- 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
Maksim Krasnyanskiy <[email protected]> :
[...]
> >- 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
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? :-))))
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
From: Dave Jones <[email protected]>
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.
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 [email protected]
> 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 |
| [email protected] | Coudersport PA 16915 | only on AXP |
+------------------------------------------------------------------+
>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
From: Maksim Krasnyanskiy <[email protected]>
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.