Re: [PATCH 4/5] usb:Push BKL on open down into the drivers

Re: [PATCH 4/5] usb:Push BKL on open down into the drivers

[Jiri Kosina]

On Wed, 13 Jan 2010, Oliver Neukum wrote:

> From 271f10a4643c788be08cc481dc594ee87af25140 Mon Sep 17 00:00:00 2001
> From: Oliver Neukum <oliver@neukum.org>
> Date: Wed, 13 Jan 2010 14:46:08 +0100
> Subject: [PATCH 4/5] usb:Push BKL on open down into the drivers
> 
> Straightforward push into the drivers to allow
> auditing individual drivers separately
> 
> Signed-off-by: Oliver Neukum <oliver@neukum.org>
> ---
>  drivers/hid/usbhid/hiddev.c           |    7 +++++--
>  drivers/media/video/dabusb.c          |    8 +++++++-
>  drivers/staging/frontier/alphatrack.c |    2 ++
>  drivers/staging/frontier/tranzport.c  |    2 ++
>  drivers/usb/class/cdc-wdm.c           |    3 +++
>  drivers/usb/class/usblp.c             |    3 +++
>  drivers/usb/class/usbtmc.c            |    3 +++
>  drivers/usb/core/file.c               |    2 --
>  drivers/usb/image/mdc800.c            |    3 +++
>  drivers/usb/misc/adutux.c             |    3 +++
>  drivers/usb/misc/ftdi-elan.c          |   15 ++++++++++++---
>  drivers/usb/misc/idmouse.c            |    8 +++++++-
>  drivers/usb/misc/iowarrior.c          |    4 ++++
>  drivers/usb/misc/ldusb.c              |   12 ++++++++++--
>  drivers/usb/misc/legousbtower.c       |    3 +++
>  drivers/usb/misc/rio500.c             |    3 ++-
>  drivers/usb/misc/sisusbvga/sisusb.c   |   14 ++++++++++++--
>  drivers/usb/misc/usblcd.c             |    5 +++++
>  drivers/usb/misc/vstusb.c             |    9 ++++++++-
>  drivers/usb/usb-skeleton.c            |    3 +++
>  20 files changed, 97 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
> index 867e084..433602a 100644
> --- a/drivers/hid/usbhid/hiddev.c
> +++ b/drivers/hid/usbhid/hiddev.c
> @@ -265,9 +265,10 @@ static int hiddev_release(struct inode * inode, struct file * file)
>  static int hiddev_open(struct inode *inode, struct file *file)
>  {
>  	struct hiddev_list *list;
> -	int res;
> +	int res, i;
>  
> -	int i = iminor(inode) - HIDDEV_MINOR_BASE;
> +	lock_kernel();
> +	i = iminor(inode) - HIDDEV_MINOR_BASE;
>  
>  	if (i >= HIDDEV_MINORS || i < 0 || !hiddev_table[i])
>  		return -ENODEV;
> @@ -313,10 +314,12 @@ static int hiddev_open(struct inode *inode, struct file *file)
>  			usbhid_open(hid);
>  		}
>  
> +	unlock_kernel();
>  	return 0;
>  bail:
>  	file->private_data = NULL;
>  	kfree(list);
> +	unlock_kernel();
>  	return res;
>  }
>  
[ ... snip ... ]
> diff --git a/drivers/usb/core/file.c b/drivers/usb/core/file.c
> index bfc6c2e..c3536f1 100644
> --- a/drivers/usb/core/file.c
> +++ b/drivers/usb/core/file.c
> @@ -34,7 +34,6 @@ static int usb_open(struct inode * inode, struct file * file)
>  	int err = -ENODEV;
>  	const struct file_operations *old_fops, *new_fops = NULL;
>  
> -	lock_kernel();
>  	down_read(&minor_rwsem);
>  	c = usb_minors[minor];
>  
> @@ -53,7 +52,6 @@ static int usb_open(struct inode * inode, struct file * file)
>  	fops_put(old_fops);
>   done:
>  	up_read(&minor_rwsem);
> -	unlock_kernel();
>  	return err;
>  }
>  

Hi Oliver,

looking at this a little bit more, this seems wrong, at least for the 
hiddev case. The reason is -- it changes the dependency order of BKL and 
minor_rwsem.

For hiddev, this causes a problem, as you introduced BKL into 
hiddev_connect() exactly because of the proper dependency -- we have the 
BKL there to avoid race after usb_register_dev() -- once the device node 
has been created, open() could happen on it.

The obvious fix -- introducing mutex to guard hiddev_table[] -- is wrong 
work, as usb_open() and usb_register_dev() both take minor_rwsem, thus 
there will be AB-BA deadlock between this mutex and minor_rwsem. 

So you avoided this situation by BKL (as usb_open() has been taking it in 
the right order), but now we are exactly in AB-BA deadlock situation 
because of the reversed dependency between BKL and minor_rwsem.

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH 4/5] usb:Push BKL on open down into the drivers

[Oliver Neukum]

Am Montag 29 März 2010 12:04:51 schrieb Jiri Kosina:
> For hiddev, this causes a problem, as you introduced BKL into 
> hiddev_connect() exactly because of the proper dependency -- we have the 
> BKL there to avoid race after usb_register_dev() -- once the device node 
> has been created, open() could happen on it.

Hi,

it seems to me you cannot deadlock with the BKL and an rwsem,
as BKL will be dropped if a task blocks on the rwsem. This is not
a nice feature of the BKL to use. Do you have an alternative idea?

	Regards
		Oliver

Re: [PATCH 4/5] usb:Push BKL on open down into the drivers

[Jiri Kosina]

On Mon, 29 Mar 2010, Oliver Neukum wrote:

> > For hiddev, this causes a problem, as you introduced BKL into 
> > hiddev_connect() exactly because of the proper dependency -- we have the 
> > BKL there to avoid race after usb_register_dev() -- once the device node 
> > has been created, open() could happen on it.
> 
> Hi,
> 
> it seems to me you cannot deadlock with the BKL and an rwsem,
> as BKL will be dropped if a task blocks on the rwsem. 

Ah, right you are.

> This is not a nice feature of the BKL to use. Do you have an alternative 
> idea?

Not right now, without introducing additional lock into USB core or 
redesigning the hiddev node registration completely. Hmmm ...

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: [PATCH 4/5] usb:Push BKL on open down into the drivers

[Oliver Neukum]

Am Montag, 29. März 2010 15:46:59 schrieb Jiri Kosina:
> > it seems to me you cannot deadlock with the BKL and an rwsem,
> > as BKL will be dropped if a task blocks on the rwsem. 
> 
> Ah, right you are.
> 
> > This is not a nice feature of the BKL to use. Do you have an alternative 
> > idea?
> 
> Not right now, without introducing additional lock into USB core or 
> redesigning the hiddev node registration completely. Hmmm ...

Then you might want to add a comment so the next developer who wants to
eradicate BKL is warned.

	Regards
		Oliver

Re: [PATCH 4/5] usb:Push BKL on open down into the drivers

[Jiri Kosina]

On Tue, 30 Mar 2010, Oliver Neukum wrote:

> > > This is not a nice feature of the BKL to use. Do you have an alternative 
> > > idea?
> > 
> > Not right now, without introducing additional lock into USB core or 
> > redesigning the hiddev node registration completely. Hmmm ...
> 
> Then you might want to add a comment so the next developer who wants to
> eradicate BKL is warned.

That I already have in my tree.

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.