Subject: Don't kmalloc with BKL held.
From: Sven-Thorsten Dietrich <[email protected]>
I'm eyeballing this file for complete removal of lock_kernel but
at first glance, we definitely don't need BKL to kmalloc().
Signed-off-by: Sven-Thorsten Dietrich <[email protected]>
diff --git a/drivers/usb/host/uhci-debug.c b/drivers/usb/host/uhci-debug.c
index e52b954..325d508 100644
--- a/drivers/usb/host/uhci-debug.c
+++ b/drivers/usb/host/uhci-debug.c
@@ -497,7 +497,6 @@ static int uhci_debug_open(struct inode *inode, struct file *file)
int ret = -ENOMEM;
unsigned long flags;
- lock_kernel();
up = kmalloc(sizeof(*up), GFP_KERNEL);
if (!up)
goto out;
@@ -509,6 +508,8 @@ static int uhci_debug_open(struct inode *inode, struct file *file)
}
up->size = 0;
+
+ lock_kernel();
spin_lock_irqsave(&uhci->lock, flags);
if (uhci->is_initialized)
up->size = uhci_sprint_schedule(uhci, up->data, MAX_OUTPUT);
Hi Sven-Thorsten,
On Fri, Oct 02, 2009 at 11:12:24AM +0200, Sven-Thorsten Dietrich wrote:
> Subject: Don't kmalloc with BKL held.
> From: Sven-Thorsten Dietrich <[email protected]>
>
> I'm eyeballing this file for complete removal of lock_kernel but
> at first glance, we definitely don't need BKL to kmalloc().
You need to move the unlock_kernel() above the out: label too.
Regards,
Frederik
>
> Signed-off-by: Sven-Thorsten Dietrich <[email protected]>
>
>
> diff --git a/drivers/usb/host/uhci-debug.c b/drivers/usb/host/uhci-debug.c
> index e52b954..325d508 100644
> --- a/drivers/usb/host/uhci-debug.c
> +++ b/drivers/usb/host/uhci-debug.c
> @@ -497,7 +497,6 @@ static int uhci_debug_open(struct inode *inode, struct file *file)
> int ret = -ENOMEM;
> unsigned long flags;
>
> - lock_kernel();
> up = kmalloc(sizeof(*up), GFP_KERNEL);
> if (!up)
> goto out;
> @@ -509,6 +508,8 @@ static int uhci_debug_open(struct inode *inode, struct file *file)
> }
>
> up->size = 0;
> +
> + lock_kernel();
> spin_lock_irqsave(&uhci->lock, flags);
> if (uhci->is_initialized)
> up->size = uhci_sprint_schedule(uhci, up->data, MAX_OUTPUT);
>
>
> --
> 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/
Am Freitag, 2. Oktober 2009 11:12:24 schrieb Sven-Thorsten Dietrich:
> Subject: Don't kmalloc with BKL held.
> From: Sven-Thorsten Dietrich <[email protected]>
>
> I'm eyeballing this file for complete removal of lock_kernel but
> at first glance, we definitely don't need BKL to kmalloc().
The reasoning is correct, but this patch screws up the error case.
You also need to move the label.
Regards
Oliver
On Fri, 2009-10-02 at 09:20 +0000, Frederik Deweerdt wrote:
> Hi Sven-Thorsten,
>
> On Fri, Oct 02, 2009 at 11:12:24AM +0200, Sven-Thorsten Dietrich wrote:
> > Subject: Don't kmalloc with BKL held.
> > From: Sven-Thorsten Dietrich <[email protected]>
> >
> > I'm eyeballing this file for complete removal of lock_kernel but
> > at first glance, we definitely don't need BKL to kmalloc().
> You need to move the unlock_kernel() above the out: label too.
>
How about this ?
Subject: Don't kmalloc with BKL held.
From: Sven-Thorsten Dietrich [email protected] Sat Oct 3 01:27:27 2009 -0700
Date: Sat Oct 3 01:27:27 2009 -0700:
Git: 4f62eb81e827eb857129101e49757d84ac9ee7eb
We don't need BKL to kmalloc
diff --git a/drivers/usb/host/uhci-debug.c b/drivers/usb/host/uhci-debug.c
index e52b954..3fd5602 100644
--- a/drivers/usb/host/uhci-debug.c
+++ b/drivers/usb/host/uhci-debug.c
@@ -494,32 +494,30 @@ static int uhci_debug_open(struct inode *inode, struct file *file)
{
struct uhci_hcd *uhci = inode->i_private;
struct uhci_debug *up;
- int ret = -ENOMEM;
unsigned long flags;
- lock_kernel();
up = kmalloc(sizeof(*up), GFP_KERNEL);
if (!up)
- goto out;
+ return -ENOMEM;
up->data = kmalloc(MAX_OUTPUT, GFP_KERNEL);
if (!up->data) {
kfree(up);
- goto out;
+ return -ENOMEM;
}
up->size = 0;
+
+ lock_kernel();
spin_lock_irqsave(&uhci->lock, flags);
if (uhci->is_initialized)
up->size = uhci_sprint_schedule(uhci, up->data, MAX_OUTPUT);
spin_unlock_irqrestore(&uhci->lock, flags);
file->private_data = up;
-
- ret = 0;
-out:
unlock_kernel();
- return ret;
+
+ return 0;
}
static loff_t uhci_debug_lseek(struct file *file, loff_t off, int whence)
On Sat, Oct 03, 2009 at 05:35:59AM -0700, Sven-Thorsten Dietrich wrote:
> On Fri, 2009-10-02 at 09:20 +0000, Frederik Deweerdt wrote:
> > Hi Sven-Thorsten,
> >
> > On Fri, Oct 02, 2009 at 11:12:24AM +0200, Sven-Thorsten Dietrich wrote:
> > > Subject: Don't kmalloc with BKL held.
> > > From: Sven-Thorsten Dietrich <[email protected]>
> > >
> > > I'm eyeballing this file for complete removal of lock_kernel but
> > > at first glance, we definitely don't need BKL to kmalloc().
> > You need to move the unlock_kernel() above the out: label too.
> >
>
> How about this ?
Yes, looks good to me.
>
> Subject: Don't kmalloc with BKL held.
> From: Sven-Thorsten Dietrich [email protected] Sat Oct 3 01:27:27 2009 -0700
> Date: Sat Oct 3 01:27:27 2009 -0700:
> Git: 4f62eb81e827eb857129101e49757d84ac9ee7eb
>
> We don't need BKL to kmalloc
>
> diff --git a/drivers/usb/host/uhci-debug.c b/drivers/usb/host/uhci-debug.c
> index e52b954..3fd5602 100644
> --- a/drivers/usb/host/uhci-debug.c
> +++ b/drivers/usb/host/uhci-debug.c
> @@ -494,32 +494,30 @@ static int uhci_debug_open(struct inode *inode, struct file *file)
> {
> struct uhci_hcd *uhci = inode->i_private;
> struct uhci_debug *up;
> - int ret = -ENOMEM;
> unsigned long flags;
>
> - lock_kernel();
> up = kmalloc(sizeof(*up), GFP_KERNEL);
> if (!up)
> - goto out;
> + return -ENOMEM;
>
> up->data = kmalloc(MAX_OUTPUT, GFP_KERNEL);
> if (!up->data) {
> kfree(up);
> - goto out;
> + return -ENOMEM;
> }
>
> up->size = 0;
> +
> + lock_kernel();
> spin_lock_irqsave(&uhci->lock, flags);
> if (uhci->is_initialized)
> up->size = uhci_sprint_schedule(uhci, up->data, MAX_OUTPUT);
> spin_unlock_irqrestore(&uhci->lock, flags);
>
> file->private_data = up;
> -
> - ret = 0;
> -out:
> unlock_kernel();
> - return ret;
> +
> + return 0;
> }
>
> static loff_t uhci_debug_lseek(struct file *file, loff_t off, int whence)
>
>
On Sat, 3 Oct 2009, Sven-Thorsten Dietrich wrote:
> On Fri, 2009-10-02 at 09:20 +0000, Frederik Deweerdt wrote:
> > Hi Sven-Thorsten,
> >
> > On Fri, Oct 02, 2009 at 11:12:24AM +0200, Sven-Thorsten Dietrich wrote:
> > > Subject: Don't kmalloc with BKL held.
> > > From: Sven-Thorsten Dietrich <[email protected]>
> > >
> > > I'm eyeballing this file for complete removal of lock_kernel but
> > > at first glance, we definitely don't need BKL to kmalloc().
> > You need to move the unlock_kernel() above the out: label too.
> >
>
> How about this ?
>
> Subject: Don't kmalloc with BKL held.
> From: Sven-Thorsten Dietrich [email protected] Sat Oct 3 01:27:27 2009 -0700
> Date: Sat Oct 3 01:27:27 2009 -0700:
> Git: 4f62eb81e827eb857129101e49757d84ac9ee7eb
>
> We don't need BKL to kmalloc
>
> diff --git a/drivers/usb/host/uhci-debug.c b/drivers/usb/host/uhci-debug.c
> index e52b954..3fd5602 100644
> --- a/drivers/usb/host/uhci-debug.c
> +++ b/drivers/usb/host/uhci-debug.c
> @@ -494,32 +494,30 @@ static int uhci_debug_open(struct inode *inode, struct file *file)
> {
> struct uhci_hcd *uhci = inode->i_private;
> struct uhci_debug *up;
> - int ret = -ENOMEM;
> unsigned long flags;
>
> - lock_kernel();
> up = kmalloc(sizeof(*up), GFP_KERNEL);
> if (!up)
> - goto out;
> + return -ENOMEM;
>
> up->data = kmalloc(MAX_OUTPUT, GFP_KERNEL);
> if (!up->data) {
> kfree(up);
> - goto out;
> + return -ENOMEM;
> }
>
> up->size = 0;
> +
> + lock_kernel();
Why don't you remove it completely ? That lock_kernel() protects
exactly nothing.
Thanks,
tglx