[PATCH v2] usb: gadget: Fix spinlock lockup on usb_function_deactivate
There is a spinlock lockup as part of composite_disconnect
when it tries to acquire cdev->lock as part of usb_gadget_deactivate.
This is because the usb_gadget_deactivate is called from
usb_function_deactivate with the same spinlock held.
This would result in the below call stack and leads to stall.
rcu: INFO: rcu_preempt detected stalls on CPUs/tasks:
rcu: 3-...0: (1 GPs behind) idle=162/1/0x4000
softirq=10819/10819 fqs=2356
(detected by 2, t=5252 jiffies, g=20129, q=3770)
Task dump for CPU 3:
task:uvc-gadget_wlhe state:R running task stack:0 pid: 674 ppid:
636 flags:0x0202
Call trace:
__switch_to+0xc0/0x170
_raw_spin_lock_irqsave+0x84/0xb0
composite_disconnect+0x28/0x78
configfs_composite_disconnect+0x68/0x70
usb_gadget_disconnect+0x10c/0x128
usb_gadget_deactivate+0xd4/0x108
usb_function_deactivate+0x6c/0x80
uvc_function_disconnect+0x20/0x58
uvc_v4l2_release+0x30/0x88
v4l2_release+0xbc/0xf0
__fput+0x7c/0x230
fput+0x14/0x20
task_work_run+0x88/0x140
do_notify_resume+0x240/0x6f0
work_pending+0x8/0x200
Fix this by doing an unlock on cdev->lock before the usb_gadget_deactivate
call from usb_function_deactivate.
The same lockup can happen in the usb_gadget_activate path. Fix that path
as well.
Reported-by: Peter Chen
Link: https://lore.kernel.org/linux-usb/20201102094936.GA29581@b29397-desktop/
Tested-by: Peter Chen
Signed-off-by: Sriharsha Allenki
---
Changes since v1:
- Updated commit text to reflect fix in usb_gadget_activate as well
drivers/usb/gadget/composite.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index c6d455f2bb92..1a556a628971 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -392,8 +392,11 @@ int usb_function_deactivate(struct usb_function *function)
spin_lock_irqsave(>lock, flags);
- if (cdev->deactivations == 0)
+ if (cdev->deactivations == 0) {
+ spin_unlock_irqrestore(>lock, flags);
status = usb_gadget_deactivate(cdev->gadget);
+ spin_lock_irqsave(>lock, flags);
+ }
if (status == 0)
cdev->deactivations++;
@@ -424,8 +427,11 @@ int usb_function_activate(struct usb_function *function)
status = -EINVAL;
else {
cdev->deactivations--;
- if (cdev->deactivations == 0)
+ if (cdev->deactivations == 0) {
+ spin_unlock_irqrestore(>lock, flags);
status = usb_gadget_activate(cdev->gadget);
+ spin_lock_irqsave(>lock, flags);
+ }
}
spin_unlock_irqrestore(>lock, flags);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a
Linux Foundation Collaborative Project
[PATCH] usb: gadget: Fix spinlock lockup on usb_function_deactivate
There is a spinlock lockup as part of composite_disconnect
when it tries to acquire cdev->lock as part of usb_gadget_deactivate.
This is because the usb_gadget_deactivate is called from
usb_function_deactivate with the same spinlock held.
This would result in the below call stack and leads to stall.
rcu: INFO: rcu_preempt detected stalls on CPUs/tasks:
rcu: 3-...0: (1 GPs behind) idle=162/1/0x4000
softirq=10819/10819 fqs=2356
(detected by 2, t=5252 jiffies, g=20129, q=3770)
Task dump for CPU 3:
task:uvc-gadget_wlhe state:R running task stack:0 pid: 674 ppid:
636 flags:0x0202
Call trace:
__switch_to+0xc0/0x170
_raw_spin_lock_irqsave+0x84/0xb0
composite_disconnect+0x28/0x78
configfs_composite_disconnect+0x68/0x70
usb_gadget_disconnect+0x10c/0x128
usb_gadget_deactivate+0xd4/0x108
usb_function_deactivate+0x6c/0x80
uvc_function_disconnect+0x20/0x58
uvc_v4l2_release+0x30/0x88
v4l2_release+0xbc/0xf0
__fput+0x7c/0x230
fput+0x14/0x20
task_work_run+0x88/0x140
do_notify_resume+0x240/0x6f0
work_pending+0x8/0x200
Fix this by doing an unlock on cdev->lock before the usb_gadget_deactivate
call from usb_function_activate.
Reported-by: Peter Chen
Link: https://lore.kernel.org/linux-usb/20201102094936.GA29581@b29397-desktop/
Tested-by: Peter Chen
Signed-off-by: Sriharsha Allenki
---
drivers/usb/gadget/composite.c | 10 --
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index c6d455f2bb92..1a556a628971 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -392,8 +392,11 @@ int usb_function_deactivate(struct usb_function *function)
spin_lock_irqsave(>lock, flags);
- if (cdev->deactivations == 0)
+ if (cdev->deactivations == 0) {
+ spin_unlock_irqrestore(>lock, flags);
status = usb_gadget_deactivate(cdev->gadget);
+ spin_lock_irqsave(>lock, flags);
+ }
if (status == 0)
cdev->deactivations++;
@@ -424,8 +427,11 @@ int usb_function_activate(struct usb_function *function)
status = -EINVAL;
else {
cdev->deactivations--;
- if (cdev->deactivations == 0)
+ if (cdev->deactivations == 0) {
+ spin_unlock_irqrestore(>lock, flags);
status = usb_gadget_activate(cdev->gadget);
+ spin_lock_irqsave(>lock, flags);
+ }
}
spin_unlock_irqrestore(>lock, flags);
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a
Linux Foundation Collaborative Project
Re: [PATCH] usb: typec: Prevent setting invalid opmode value
Hi Heikki,
Thanks a lot for the review.
On 10/26/2020 6:35 PM, Heikki Krogerus wrote:
> On Thu, Oct 22, 2020 at 03:12:14PM +0530, Sriharsha Allenki wrote:
>> Setting opmode to invalid values would lead to a
>> paging fault failure when there is an access to the
>> power_operation_mode.
>>
>> Prevent this by checking the validity of the value
>> that the opmode is being set.
>>
>> Cc:
>> Fixes: fab9288428ec ("usb: USB Type-C connector class")
>> Signed-off-by: Sriharsha Allenki
>> ---
>> drivers/usb/typec/class.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
>> index 35eec70..63efe16 100644
>> --- a/drivers/usb/typec/class.c
>> +++ b/drivers/usb/typec/class.c
>> @@ -1427,7 +1427,8 @@ void typec_set_pwr_opmode(struct typec_port *port,
>> {
>> struct device *partner_dev;
>>
>> -if (port->pwr_opmode == opmode)
>> +if ((port->pwr_opmode == opmode) || (opmode < TYPEC_PWR_MODE_USB) ||
> You don't need to check if opmode < anything. opmode is enum which
> apparently means that GCC handles it as unsigned. Since
> TYPEC_PWR_MODE_USB is 0 it means opmode < TYPEC_PWR_MODE_USB is never
> true.
>
>> +(opmode > TYPEC_PWR_MODE_PD))
>> return;
> You really need to print an error at the very least. Otherwise we will
> just silently hide possible driver bugs.
>
> To be honest, I'm not a big fan of this kind of checks. They have
> created more problems than they have fixed in more than one occasion
> to me. For example, there really is no guarantee that the maximum will
> always be TYPEC_PWR_MODE_PD, which means we probable should have
> something like TYPEC_PWR_MODE_MAX defined somewhere that you compare
> the opmode value to instead of TYPEC_PWR_MODE_PD to play it safe, but
> let's not bother with that for now (it will create other problems).
>
> Basically, with functions like this, especially since it doesn't
> return anything, the responsibility of checking the validity of the
> parameters that the caller supplies to it belongs to the caller IMO,
> not the function itself. I would be happy to explain that in the
> kernel doc style comment of the function.
>
> If you still feel that this change is really necessary, meaning you
> have some actual case where the caller can _not_ check the range
> before calling this function, then explain the case you have carefully
> in the commit message and add the check as a separate condition:
We had a bug that was setting this out of index opmode leading to the
mentioned paging fault, and as you have suggested we could have added
the range check there and prevented this. But there are many calls to
this function, and I thought it would be a good idea to abstract that
range check into this function to prevent adding multiple range checks
over the driver. And further motivation was also to prevent any
potential unknown bugs.
I will resend the patch with the suggested changes; separate condition and
anerror statement if the above justification is acceptable, else will
propose a patch to improve the documentation.
Thanks,
Sriharsha
>
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index 35eec707cb512..7de6913d90f9c 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -1430,6 +1430,11 @@ void typec_set_pwr_opmode(struct typec_port *port,
> if (port->pwr_opmode == opmode)
> return;
>
> + if (opmode > TYPEC_PWR_OPMODE_PD) {
> + dev_err(>dev, "blah-blah-blah\n");
> + return;
> + }
> +
> port->pwr_opmode = opmode;
> sysfs_notify(>dev.kobj, NULL, "power_operation_mode");
> kobject_uevent(>dev.kobj, KOBJ_CHANGE);
>
> Otherwise you can just propose a patch that improves the documentation
> of this function, explaining that it does not take any responsibility
> over the parameters passed to it for now.
>
>
> thanks,
>
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
[PATCH] usb: typec: Prevent setting invalid opmode value
Setting opmode to invalid values would lead to a
paging fault failure when there is an access to the
power_operation_mode.
Prevent this by checking the validity of the value
that the opmode is being set.
Cc:
Fixes: fab9288428ec ("usb: USB Type-C connector class")
Signed-off-by: Sriharsha Allenki
---
drivers/usb/typec/class.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 35eec70..63efe16 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -1427,7 +1427,8 @@ void typec_set_pwr_opmode(struct typec_port *port,
{
struct device *partner_dev;
- if (port->pwr_opmode == opmode)
+ if ((port->pwr_opmode == opmode) || (opmode < TYPEC_PWR_MODE_USB) ||
+ (opmode > TYPEC_PWR_MODE_PD))
return;
port->pwr_opmode = opmode;
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a
Linux Foundation Collaborative Project
