diff mbox series

usb: gadget: f_fs: Remove WARN_ON in functionfs_bind

Message ID 20241219125221.1679-1-akash.m5@samsung.com
State New
Headers show
Series usb: gadget: f_fs: Remove WARN_ON in functionfs_bind | expand

Commit Message

Akash M/Akash M Dec. 19, 2024, 12:52 p.m. UTC 
This commit addresses an issue related to below kernel panic where
panic_on_warn is enabled. It is caused by the unnecessary use of WARN_ON
in functionsfs_bind, which easily leads to the following scenarios.

1.adb_write in adbd               2. UDC write via configfs
  =================	             =====================

->usb_ffs_open_thread()           ->UDC write
 ->open_functionfs()               ->configfs_write_iter()
  ->adb_open()                      ->gadget_dev_desc_UDC_store()
   ->adb_write()                     ->usb_gadget_register_driver_owner
                                      ->driver_register()
->StartMonitor()                       ->bus_add_driver()
 ->adb_read()                           ->gadget_bind_driver()
<times-out without BIND event>           ->configfs_composite_bind()
                                          ->usb_add_function()
->open_functionfs()                        ->ffs_func_bind()
 ->adb_open()                               ->functionfs_bind()
                                       <ffs->state !=FFS_ACTIVE>

The adb_open, adb_read, and adb_write operations are invoked from the
daemon, but trying to bind the function is a process that is invoked by
UDC write through configfs, which opens up the possibility of a race
condition between the two paths. In this race scenario, the kernel panic
occurs due to the WARN_ON from functionfs_bind when panic_on_warn is
enabled. This commit fixes the kernel panic by removing the unnecessary
WARN_ON.

Kernel panic - not syncing: kernel: panic_on_warn set ...
[   14.542395] Call trace:
[   14.542464]  ffs_func_bind+0x1c8/0x14a8
[   14.542468]  usb_add_function+0xcc/0x1f0
[   14.542473]  configfs_composite_bind+0x468/0x588
[   14.542478]  gadget_bind_driver+0x108/0x27c
[   14.542483]  really_probe+0x190/0x374
[   14.542488]  __driver_probe_device+0xa0/0x12c
[   14.542492]  driver_probe_device+0x3c/0x220
[   14.542498]  __driver_attach+0x11c/0x1fc
[   14.542502]  bus_for_each_dev+0x104/0x160
[   14.542506]  driver_attach+0x24/0x34
[   14.542510]  bus_add_driver+0x154/0x270
[   14.542514]  driver_register+0x68/0x104
[   14.542518]  usb_gadget_register_driver_owner+0x48/0xf4
[   14.542523]  gadget_dev_desc_UDC_store+0xf8/0x144
[   14.542526]  configfs_write_iter+0xf0/0x138

Fixes: ddf8abd25994 ("USB: f_fs: the FunctionFS driver")
Cc: stable@vger.kernel.org
Signed-off-by: Akash M <akash.m5@samsung.com>

Comments

Akash M/Akash M Dec. 24, 2024, 6:42 a.m. UTC | #1 
On 12/19/2024 6:22 PM, Akash M wrote:
> This commit addresses an issue related to below kernel panic where
> panic_on_warn is enabled. It is caused by the unnecessary use of WARN_ON
> in functionsfs_bind, which easily leads to the following scenarios.
>
> 1.adb_write in adbd               2. UDC write via configfs
>    =================	             =====================
>
> ->usb_ffs_open_thread()           ->UDC write
>   ->open_functionfs()               ->configfs_write_iter()
>    ->adb_open()                      ->gadget_dev_desc_UDC_store()
>     ->adb_write()                     ->usb_gadget_register_driver_owner
>                                        ->driver_register()
> ->StartMonitor()                       ->bus_add_driver()
>   ->adb_read()                           ->gadget_bind_driver()
> <times-out without BIND event>           ->configfs_composite_bind()
>                                            ->usb_add_function()
> ->open_functionfs()                        ->ffs_func_bind()
>   ->adb_open()                               ->functionfs_bind()
>                                         <ffs->state !=FFS_ACTIVE>
>
> The adb_open, adb_read, and adb_write operations are invoked from the
> daemon, but trying to bind the function is a process that is invoked by
> UDC write through configfs, which opens up the possibility of a race
> condition between the two paths. In this race scenario, the kernel panic
> occurs due to the WARN_ON from functionfs_bind when panic_on_warn is
> enabled. This commit fixes the kernel panic by removing the unnecessary
> WARN_ON.
>
> Kernel panic - not syncing: kernel: panic_on_warn set ...
> [   14.542395] Call trace:
> [   14.542464]  ffs_func_bind+0x1c8/0x14a8
> [   14.542468]  usb_add_function+0xcc/0x1f0
> [   14.542473]  configfs_composite_bind+0x468/0x588
> [   14.542478]  gadget_bind_driver+0x108/0x27c
> [   14.542483]  really_probe+0x190/0x374
> [   14.542488]  __driver_probe_device+0xa0/0x12c
> [   14.542492]  driver_probe_device+0x3c/0x220
> [   14.542498]  __driver_attach+0x11c/0x1fc
> [   14.542502]  bus_for_each_dev+0x104/0x160
> [   14.542506]  driver_attach+0x24/0x34
> [   14.542510]  bus_add_driver+0x154/0x270
> [   14.542514]  driver_register+0x68/0x104
> [   14.542518]  usb_gadget_register_driver_owner+0x48/0xf4
> [   14.542523]  gadget_dev_desc_UDC_store+0xf8/0x144
> [   14.542526]  configfs_write_iter+0xf0/0x138
>
> Fixes: ddf8abd25994 ("USB: f_fs: the FunctionFS driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Akash M <akash.m5@samsung.com>
>
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index 2920f8000bbd..92c883440e02 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -2285,7 +2285,7 @@ static int functionfs_bind(struct ffs_data *ffs, struct usb_composite_dev *cdev)
>   	struct usb_gadget_strings **lang;
>   	int first_id;
>   
> -	if (WARN_ON(ffs->state != FFS_ACTIVE
> +	if ((ffs->state != FFS_ACTIVE
>   		 || test_and_set_bit(FFS_FL_BOUND, &ffs->flags)))
>   		return -EBADFD;
>   
Hi Greg,

I realized there's a minor nitpick with the patch I submitted - 
specifically a pair of extra brackets not removed.

Do you want me to proceed with sending a v2 to address this, or is this 
something you can take care while applying this patch?

Looking forward to your advice.

Thanks,
Akash
Greg KH Dec. 24, 2024, 7:38 a.m. UTC | #2 
On Tue, Dec 24, 2024 at 12:12:49PM +0530, Akash M/Akash M wrote:
> 
> On 12/19/2024 6:22 PM, Akash M wrote:
> > This commit addresses an issue related to below kernel panic where
> > panic_on_warn is enabled. It is caused by the unnecessary use of WARN_ON
> > in functionsfs_bind, which easily leads to the following scenarios.
> >
> > 1.adb_write in adbd               2. UDC write via configfs
> >    =================	             =====================
> >
> > ->usb_ffs_open_thread()           ->UDC write
> >   ->open_functionfs()               ->configfs_write_iter()
> >    ->adb_open()                      ->gadget_dev_desc_UDC_store()
> >     ->adb_write()                     ->usb_gadget_register_driver_owner
> >                                        ->driver_register()
> > ->StartMonitor()                       ->bus_add_driver()
> >   ->adb_read()                           ->gadget_bind_driver()
> > <times-out without BIND event>           ->configfs_composite_bind()
> >                                            ->usb_add_function()
> > ->open_functionfs()                        ->ffs_func_bind()
> >   ->adb_open()                               ->functionfs_bind()
> >                                         <ffs->state !=FFS_ACTIVE>
> >
> > The adb_open, adb_read, and adb_write operations are invoked from the
> > daemon, but trying to bind the function is a process that is invoked by
> > UDC write through configfs, which opens up the possibility of a race
> > condition between the two paths. In this race scenario, the kernel panic
> > occurs due to the WARN_ON from functionfs_bind when panic_on_warn is
> > enabled. This commit fixes the kernel panic by removing the unnecessary
> > WARN_ON.
> >
> > Kernel panic - not syncing: kernel: panic_on_warn set ...
> > [   14.542395] Call trace:
> > [   14.542464]  ffs_func_bind+0x1c8/0x14a8
> > [   14.542468]  usb_add_function+0xcc/0x1f0
> > [   14.542473]  configfs_composite_bind+0x468/0x588
> > [   14.542478]  gadget_bind_driver+0x108/0x27c
> > [   14.542483]  really_probe+0x190/0x374
> > [   14.542488]  __driver_probe_device+0xa0/0x12c
> > [   14.542492]  driver_probe_device+0x3c/0x220
> > [   14.542498]  __driver_attach+0x11c/0x1fc
> > [   14.542502]  bus_for_each_dev+0x104/0x160
> > [   14.542506]  driver_attach+0x24/0x34
> > [   14.542510]  bus_add_driver+0x154/0x270
> > [   14.542514]  driver_register+0x68/0x104
> > [   14.542518]  usb_gadget_register_driver_owner+0x48/0xf4
> > [   14.542523]  gadget_dev_desc_UDC_store+0xf8/0x144
> > [   14.542526]  configfs_write_iter+0xf0/0x138
> >
> > Fixes: ddf8abd25994 ("USB: f_fs: the FunctionFS driver")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Akash M <akash.m5@samsung.com>
> >
> > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> > index 2920f8000bbd..92c883440e02 100644
> > --- a/drivers/usb/gadget/function/f_fs.c
> > +++ b/drivers/usb/gadget/function/f_fs.c
> > @@ -2285,7 +2285,7 @@ static int functionfs_bind(struct ffs_data *ffs, struct usb_composite_dev *cdev)
> >   	struct usb_gadget_strings **lang;
> >   	int first_id;
> >   
> > -	if (WARN_ON(ffs->state != FFS_ACTIVE
> > +	if ((ffs->state != FFS_ACTIVE
> >   		 || test_and_set_bit(FFS_FL_BOUND, &ffs->flags)))
> >   		return -EBADFD;
> >   
> Hi Greg,
> 
> I realized there's a minor nitpick with the patch I submitted - 
> specifically a pair of extra brackets not removed.
> 
> Do you want me to proceed with sending a v2 to address this, or is this 
> something you can take care while applying this patch?

It's already in my tree, as you should have gotten an email about that.

Just send a cleanup patch for later, it's not a big deal.

thanks,

greg k-h
diff mbox series

Patch

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 2920f8000bbd..92c883440e02 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -2285,7 +2285,7 @@  static int functionfs_bind(struct ffs_data *ffs, struct usb_composite_dev *cdev)
 	struct usb_gadget_strings **lang;
 	int first_id;
 
-	if (WARN_ON(ffs->state != FFS_ACTIVE
+	if ((ffs->state != FFS_ACTIVE
 		 || test_and_set_bit(FFS_FL_BOUND, &ffs->flags)))
 		return -EBADFD;