Re: [PATCH] usb: f_fs: Make sure to unregister gadget item in unbind

[John Keeping]

On Wed, Jan 11, 2023 at 05:47:17PM +0530, bhuvanesh_surachari@xxxxxxxxxx wrote:
> From: Bhuvanesh Surachari <bhuvanesh_surachari@xxxxxxxxxx>
> 
> In functionfs_unbind() the FFS_FL_BOUND flag was cleared before
> calling ffs_data_put() which was preventing the execution of function
> unregister_gadget_item().
> This was leading to Kernel panic due to NULL pointer dereference as
> below:
> Unable to handle kernel NULL pointer dereference at virtual address 00000020
> Mem abort info:
>   Exception class = DABT (current EL), IL = 32 bits
>   SET = 0, FnV = 0
>   EA = 0, S1PTW = 0
> Data abort info:
>   ISV = 0, ISS = 0x00000006
>   CM = 0, WnR = 0
> user pgtable: 4k pages, 48-bit VAs, pgd = ffff80062cb6a000
> [0000000000000020] *pgd=000000066c966003, *pud=000000067a170003, *pmd=0000000000000000
> Internal error: Oops: 96000006 [#1] PREEMPT SMP
> tftp nf_nat nf_conntrack_tftp nf_conntrack adv7180 optee tee quota_v2 quota_tree max20010_regulator aesi adc_inc input_inc cpufreq_dt thermal_sys ravb snd_soc_rcar snd_aloop snd_soc_skeleton ravb_mdio snd_soc_generic_card
> tp pps_core sbrrc spidev spi_sh_msiof evdev boottime gpio_inc i2c_dev usb8251x_firmware ipv6 autofs4 [last unloaded: atmel_mxt_ts]
> Process swapper/1 (pid: 0, stack limit = 0xffff000008e30000)
> CPU: 1 PID: 0 Comm: swapper/1 Tainted: G         C      4.14.295-ltsi-08448-g8e327c2d87fb #1
> Hardware name: RBCM A-IVI2 CCS1.1 B board based on r8a7796 (DT)
> pc : usb_ep_queue+0xe0/0x110 [udc_core]
> lr : eth_start_xmit+0x280/0x30c [u_ether]
> sp : ffff00000800bde0 pstate : 80000145
> x29: ffff00000800bde0 x28: 0000000000000006
> x27: 0000000000000140 x26: ffff8005fbb7a518
> x25: ffff80063b2c98a8 x24: ffff80063a6f73b8
> x23: ffff80063b2c98a0 x22: ffff8005fbb7a518
> x21: ffff80063b2c9000 x20: ffff80063a6f73b8
> x19: ffff8005fbb7a558 x18: 000000000049a1dc
> x17: 000000365edd6f88 x16: ffff000008204254
> x15: 0000000000000000 x14: 0000000000000400
> x13: 0000000000000400 x12: 0000000000000000
> x11: 0101010101010101 x10: 0000000000000000
> x9 : 0000000000000484 x8 : ffff8005d9a44214
> x7 : 0000000000000000 x6 : ffff8005d9a44210
> x5 : ffff8005d9a44210 x4 : 0000000000000214
> x3 : 0000000000000001 x2 : 0000000001080020
> x1 : ffff8005fbb7a518 x0 : 0000000000000000
> Call trace:
>  usb_ep_queue+0xe0/0x110 [udc_core]
>  eth_start_xmit+0x280/0x30c [u_ether]
>  ncm_tx_tasklet+0x3c/0x50 [usb_f_ncm]
>  tasklet_action+0xa0/0x104
>  __do_softirq+0x260/0x3b8
>  irq_exit+0x7c/0xd8
>  __handle_domain_irq+0x78/0xac
>  gic_handle_irq+0x68/0xa8
>  el1_irq+0xb4/0x12c
>  cpuidle_enter_state+0x1b4/0x2d4
>  cpuidle_enter+0x18/0x20
>  call_cpuidle+0x34/0x38
>  do_idle+0x158/0x1a8
>  cpu_startup_entry+0x20/0x30
>  secondary_start_kernel+0x10c/0x118
> Code: 95e9c147 17ffffe3 f9400a80 aa1603e1 (f9401003)
> ---[ end trace ffcd984d149a0f4e ]---
> Kernel panic - not syncing: Fatal exception in interrupt
> SMP: stopping secondary CPUs
> Kernel Offset: disabled
> CPU features: 0x21002004
> Memory Limit: 3968 MB
> Rebooting in 3 seconds..

I don't understand this - that looks like a networking gadget which is
not FFS.  How does an issue in the FFS function affect the underlying
UDC for a totally different gadget function?

> Hence clear the FFS_FL_BOUND flag after checking using
> test_and_clear_bit() in function ffs_closed() which ensures calling
> of unregister_gadget_item().
> 
> Signed-off-by: Bhuvanesh Surachari <bhuvanesh_surachari@xxxxxxxxxx>
> ---
>  drivers/usb/gadget/function/f_fs.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index 73dc10a77cde..8bed3c800dff 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -1895,7 +1895,6 @@ static void functionfs_unbind(struct ffs_data *ffs)
>  		usb_ep_free_request(ffs->gadget->ep0, ffs->ep0req);
>  		ffs->ep0req = NULL;
>  		ffs->gadget = NULL;
> -		clear_bit(FFS_FL_BOUND, &ffs->flags);
>  		ffs_data_put(ffs);
>  	}
>  }
> @@ -3847,7 +3846,7 @@ static void ffs_closed(struct ffs_data *ffs)
>  	ci = opts->func_inst.group.cg_item.ci_parent->ci_parent;
>  	ffs_dev_unlock();
>  
> -	if (test_bit(FFS_FL_BOUND, &ffs->flags))
> +	if (test_and_clear_bit(FFS_FL_BOUND, &ffs->flags))

If you're clearing the FFS_FL_BOUND flag here, then doesn't the name of
the flag need to change as it's no longer tracking what it claims to
(and indeed this is effectively unconditional if the flag can't be
cleared anywhere else).

>  		unregister_gadget_item(ci);
>  	return;
>  done:
> -- 
> 2.17.1
>