Locking in ch_open is covered by the spin_lock, it serializes the calls
to idr_find and scsi_device_get. The BKL appears redundant to me here.
>From b385c85bb5c2579e542cfe55475b729325eb65e1 Mon Sep 17 00:00:00 2001
From: John Kacur <[email protected]>
Date: Sun, 11 Oct 2009 13:06:54 +0200
Subject: [PATCH] drivers/scsi/ch.c: Remove BKL in ch_open
Everything in ch_open is covered by a spin_lock, so the lock_kernel is redundant
Remove it.
Signed-off-by: John Kacur <[email protected]>
---
drivers/scsi/ch.c | 5 +----
1 files changed, 1 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
index fe11c1d..4ba8b67 100644
--- a/drivers/scsi/ch.c
+++ b/drivers/scsi/ch.c
@@ -579,19 +579,16 @@ ch_open(struct inode *inode, struct file *file)
scsi_changer *ch;
int minor = iminor(inode);
- lock_kernel();
spin_lock(&ch_index_lock);
ch = idr_find(&ch_index_idr, minor);
if (NULL == ch || scsi_device_get(ch->device)) {
spin_unlock(&ch_index_lock);
- unlock_kernel();
return -ENXIO;
}
+ file->private_data = ch;
spin_unlock(&ch_index_lock);
- file->private_data = ch;
- unlock_kernel();
return 0;
}
--
1.6.0.6
On Sun, 11 Oct 2009 15:45:16 +0200 (CEST)
John Kacur <[email protected]> wrote:
[linux-scsi cc'd]
> Locking in ch_open is covered by the spin_lock, it serializes the calls
> to idr_find and scsi_device_get. The BKL appears redundant to me here.
I'm not so sure. In fact there are some quite umm interesting questions
about this code, and some of them are shared with other modules too.
Consider the following sequence
CPU1 CPU2
register_chrdev
ok
open device
takes lock
scsi_register_driver
error
unregister_chrdev
error
unload
??????
We don't allocate any idr entries in open so the paths don't seem to leak
and the module itself looks correct. Looking at the bigger picture
however I am not sure what the module loader is trying to do in
kernel/module.c with the code at
if (ret < 0) {
/* Init routine failed: abort. Try to protect us from
buggy refcounters. */
mod->state = MODULE_STATE_GOING;
synchronize_sched();
module_put(mod);
blocking_notifier_call_chain(&module_notify_list,
but it doesn't seem to be sufficient for what may go on ?
Rusty ?
On Sun, 11 Oct 2009 15:37:09 +0100
Alan Cox <[email protected]> wrote:
> On Sun, 11 Oct 2009 15:45:16 +0200 (CEST)
> John Kacur <[email protected]> wrote:
>
> [linux-scsi cc'd]
>
> > Locking in ch_open is covered by the spin_lock, it serializes the
> > calls to idr_find and scsi_device_get. The BKL appears redundant to
> > me here.
>
> I'm not so sure. In fact there are some quite umm interesting
> questions about this code, and some of them are shared with other
> modules too.
>
> Consider the following sequence
>
> CPU1 CPU2
> register_chrdev
> ok
> open device
> takes lock
but open does not take the BKL, so the BKL is not protecting you at
all against this..
--
Arjan van de Ven Intel Open Source Technology Centre
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
> > I'm not so sure. In fact there are some quite umm interesting
> > questions about this code, and some of them are shared with other
> > modules too.
> >
> > Consider the following sequence
> >
> > CPU1 CPU2
> > register_chrdev
> > ok
> > open device
> > takes lock
>
>
> but open does not take the BKL, so the BKL is not protecting you at
> all against this..
But module load/unload does - so while the change doesn't appear to cause
any problems there is an underlying problem here that wants looking at.
On Sun, 11 Oct 2009 16:54:08 +0100
Alan Cox <[email protected]> wrote:
> > > I'm not so sure. In fact there are some quite umm interesting
> > > questions about this code, and some of them are shared with other
> > > modules too.
> > >
> > > Consider the following sequence
> > >
> > > CPU1 CPU2
> > > register_chrdev
> > > ok
> > > open device
> > > takes lock
> >
> >
> > but open does not take the BKL, so the BKL is not protecting you at
> > all against this..
>
> But module load/unload does
it does?
looking at the code.... so far failing to find where.
--
Arjan van de Ven Intel Open Source Technology Centre
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
> > But module load/unload does
>
> it does?
>
> looking at the code.... so far failing to find where.
Interesting - it used to and there is code that still relies on that.
The more general problem of
register_chrdev
open
do something
error
unload
whoops
definitely needs looking at however.
On Sun, 11 Oct 2009 17:14:03 +0100
Alan Cox <[email protected]> wrote:
> > > But module load/unload does
> >
> > it does?
> >
> > looking at the code.... so far failing to find where.
>
> Interesting - it used to and there is code that still relies on that.
I can't find that to be the case anymore though
>
> The more general problem of
>
> register_chrdev
> open
> do something
> error
> unload
> whoops
>
> definitely needs looking at however.
absolutely
we used to have the same issue with networking devices,
and there it ended up with a split "allocate" and "register"
--
Arjan van de Ven Intel Open Source Technology Centre
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
On Sun, 11 Oct 2009 09:19:04 -0700
Arjan van de Ven <[email protected]> wrote:
> > The more general problem of
> >
> > register_chrdev
> > open
> > do something
> > error
> > unload
> > whoops
> >
> > definitely needs looking at however.
>
> absolutely
>
> we used to have the same issue with networking devices,
> and there it ended up with a split "allocate" and "register"
Char devs are really the same way - request_chrdev_region() and
cdev_add(). I've thought for a while that register_chrdev() should be
seen as a legacy interface and removed - LDD3 was written from that
point of view. But that takes work, and nobody's found the issue
urgent enough to put time into...
jon
On 10/11/2009 3:45 PM, John Kacur wrote:
> Locking in ch_open is covered by the spin_lock, it serializes the calls
> to idr_find and scsi_device_get. The BKL appears redundant to me here.
Superficially this looks OK (i.e. no race with driver init in general).
But I do wonder if there isn't already a race condition possible with
the current code, between ch_probe and the file operations. ch_probe
makes a scsi_changer instance available in the IDR and creates the
corresponding character device file _before_ the scsi_changer instance
is fully initialized.
(Full quote follows for lsml)
> From b385c85bb5c2579e542cfe55475b729325eb65e1 Mon Sep 17 00:00:00 2001
> From: John Kacur <[email protected]>
> Date: Sun, 11 Oct 2009 13:06:54 +0200
> Subject: [PATCH] drivers/scsi/ch.c: Remove BKL in ch_open
>
> Everything in ch_open is covered by a spin_lock, so the lock_kernel is redundant
> Remove it.
>
> Signed-off-by: John Kacur <[email protected]>
> ---
> drivers/scsi/ch.c | 5 +----
> 1 files changed, 1 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
> index fe11c1d..4ba8b67 100644
> --- a/drivers/scsi/ch.c
> +++ b/drivers/scsi/ch.c
> @@ -579,19 +579,16 @@ ch_open(struct inode *inode, struct file *file)
> scsi_changer *ch;
> int minor = iminor(inode);
>
> - lock_kernel();
> spin_lock(&ch_index_lock);
> ch = idr_find(&ch_index_idr, minor);
>
> if (NULL == ch || scsi_device_get(ch->device)) {
> spin_unlock(&ch_index_lock);
> - unlock_kernel();
> return -ENXIO;
> }
> + file->private_data = ch;
> spin_unlock(&ch_index_lock);
>
> - file->private_data = ch;
> - unlock_kernel();
> return 0;
> }
>
--
Stefan Richter
-=====-==--= =-=- -==--
http://arcgraph.de/sr/