[PATCH RFC] [PATCH] drivers/scsi/ch.c: Remove BKL in ch_open

[PATCH RFC] [PATCH] drivers/scsi/ch.c: Remove BKL in ch_open

[John Kacur]

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 <jkacur@redhat.com>
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 <jkacur@redhat.com>
---
 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

Re: [PATCH RFC] [PATCH] drivers/scsi/ch.c: Remove BKL in ch_open

[Alan Cox]

On Sun, 11 Oct 2009 15:45:16 +0200 (CEST)
John Kacur <jkacur@redhat.com> 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 ?

Re: [PATCH RFC] [PATCH] drivers/scsi/ch.c: Remove BKL in ch_open

[Arjan van de Ven]

On Sun, 11 Oct 2009 15:37:09 +0100
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> On Sun, 11 Oct 2009 15:45:16 +0200 (CEST)
> John Kacur <jkacur@redhat.com> 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

Re: [PATCH RFC] [PATCH] drivers/scsi/ch.c: Remove BKL in ch_open

[Alan Cox]

> > 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.

Re: [PATCH RFC] [PATCH] drivers/scsi/ch.c: Remove BKL in ch_open

[Arjan van de Ven]

On Sun, 11 Oct 2009 16:54:08 +0100
Alan Cox <alan@lxorguk.ukuu.org.uk> 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

Re: [PATCH RFC] [PATCH] drivers/scsi/ch.c: Remove BKL in ch_open

[Alan Cox]

> > 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.

Re: [PATCH RFC] [PATCH] drivers/scsi/ch.c: Remove BKL in ch_open

[Arjan van de Ven]

On Sun, 11 Oct 2009 17:14:03 +0100
Alan Cox <alan@lxorguk.ukuu.org.uk> 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

Re: [PATCH RFC] [PATCH] drivers/scsi/ch.c: Remove BKL in ch_open

[Jonathan Corbet]

On Sun, 11 Oct 2009 09:19:04 -0700
Arjan van de Ven <arjan@infradead.org> 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

Re: [PATCH RFC] [PATCH] drivers/scsi/ch.c: Remove BKL in ch_open

[Stefan Richter]

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 <jkacur@redhat.com>
> 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 <jkacur@redhat.com>
> ---
>  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/