Re: [syzbot] WARNING in driver_unregister

Re: [syzbot] WARNING in driver_unregister

[Hillf Danton]

On Sat, 28 May 2022 16:22:56 -0400 Alan Stern  wrote:
> On Sat, May 28, 2022 at 10:55:22AM -0700, syzbot wrote:
> > Hello,
> > 
> > syzbot has tested the proposed patch and the reproducer did not trigger any issue:
> > 
> > Reported-and-tested-by: syzbot+02b16343704b3af1667e@syzkaller.appspotmail.com
> > 
> > Tested on:
> > 
> > commit:         97fa5887 USB: new quirk for Dell Gen 2 devices
> > git tree:       https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
> > kernel config:  https://syzkaller.appspot.com/x/.config?x=d7b232ec3adf5c8d
> > dashboard link: https://syzkaller.appspot.com/bug?extid=02b16343704b3af1667e
> > compiler:       gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
> > patch:          https://syzkaller.appspot.com/x/patch.diff?x=10f44625f00000
> > 
> > Note: testing is done by a robot and is best-effort only.
> 
> Yeah, I don't believe this result.
> 
> In any case, I believe the second problem (unexpected unregistration) 
> arises because the driver has no protection against multiple threads 
> calling raw_ioctl_run() concurrently.  Fixing that should be a second 
> patch, but for testing purposes the two are combined below.
> 
> Alan Stern
> 
> #syz test: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git 97fa5887cf28
> 
> Index: usb-devel/drivers/usb/gadget/legacy/raw_gadget.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/gadget/legacy/raw_gadget.c
> +++ usb-devel/drivers/usb/gadget/legacy/raw_gadget.c
> @@ -11,6 +11,7 @@
>  #include <linux/ctype.h>
>  #include <linux/debugfs.h>
>  #include <linux/delay.h>
> +#include <linux/idr.h>
>  #include <linux/kref.h>
>  #include <linux/miscdevice.h>
>  #include <linux/module.h>
> @@ -36,6 +37,9 @@ MODULE_LICENSE("GPL");
>  
>  /*----------------------------------------------------------------------*/
>  
> +static DEFINE_IDA(driver_id_numbers);
> +#define DRIVER_DRIVER_NAME_LENGTH_MAX	32
> +
>  #define RAW_EVENT_QUEUE_SIZE	16
>  
>  struct raw_event_queue {
> @@ -145,6 +149,7 @@ enum dev_state {
>  	STATE_DEV_INVALID = 0,
>  	STATE_DEV_OPENED,
>  	STATE_DEV_INITIALIZED,
> +	STATE_DEV_REGISTERING,
>  	STATE_DEV_RUNNING,
>  	STATE_DEV_CLOSED,
>  	STATE_DEV_FAILED

Thanks for your plumber fix. Feel free to add

Acked-by: Hillf Danton <hdanton@sina.com>