All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] usb: f_fs: Make sure to unregister gadget item in unbind
@ 2023-01-11 12:17 bhuvanesh_surachari
  2023-01-11 15:28 ` John Keeping
  0 siblings, 1 reply; 2+ messages in thread
From: bhuvanesh_surachari @ 2023-01-11 12:17 UTC (permalink / raw)
  Cc: gregkh, Bhuvanesh Surachari, John Keeping, Linyu Yuan,
	Udipto Goswami, Kees Cook, Gustavo A. R. Silva, Wolfram Sang,
	Dan Carpenter, linux-usb, linux-kernel

From: Bhuvanesh Surachari <bhuvanesh_surachari@mentor.com>

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

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@mentor.com>
---
 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))
 		unregister_gadget_item(ci);
 	return;
 done:
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 2+ messages in thread
* Re: [PATCH] usb: f_fs: Make sure to unregister gadget item in unbind
  2023-01-11 12:17 [PATCH] usb: f_fs: Make sure to unregister gadget item in unbind bhuvanesh_surachari
@ 2023-01-11 15:28 ` John Keeping
  0 siblings, 0 replies; 2+ messages in thread
From: John Keeping @ 2023-01-11 15:28 UTC (permalink / raw)
  To: bhuvanesh_surachari
  Cc: gregkh, Linyu Yuan, Udipto Goswami, Kees Cook,
	Gustavo A. R. Silva, Wolfram Sang, Dan Carpenter, linux-usb,
	linux-kernel

On Wed, Jan 11, 2023 at 05:47:17PM +0530, bhuvanesh_surachari@mentor.com wrote:
> From: Bhuvanesh Surachari <bhuvanesh_surachari@mentor.com>
> 
> 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@mentor.com>
> ---
>  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
> 

^ permalink raw reply	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-01-11 15:29 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-11 12:17 [PATCH] usb: f_fs: Make sure to unregister gadget item in unbind bhuvanesh_surachari
2023-01-11 15:28 ` John Keeping

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.