Re: [PATCH v2] hid: usbhid: hid-core: fix recursive deadlock

[Jiri Kosina]

On Wed, 20 Jan 2016, Jason Gerecke wrote:

> I've been prompted to resurrect this question after fielding yet another 
> question from a user encountering a locked-up system. We received a 
> surprising number of reports of people encountering this issue in the 
> past several weeks and that's only going to get worse as 4.4 works its 
> way into distros. Integrating back even just to 4.4 instead of 3.19 
> would be appreciated since this issue has (so far) only ever been 
> encountered with the new Intuos tablets that were added in 4.4...

I am in favor of pushing this to -stable. As you guys seem to be the ones 
able to test it with your hardware, would you guys (Ioan-Adrian? Jason?) 
be willing to prepare the backport and submit it once it passes your 
testing?

Thanks,

-- 
Jiri Kosina
SUSE Labs

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] hid: usbhid: hid-core: fix recursive deadlock

[Jason Gerecke]

Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one  /
(That is to say, eight) to the two, /
But you can’t take seven from three,/
So you look at the sixty-fours



On Sun, Nov 29, 2015 at 2:29 AM, Ioan-Adrian Ratiu  wrote:
> On Fri, 20 Nov 2015 22:19:02 +0200
> Ioan-Adrian Ratiu  wrote:
>
>> The critical section protected by usbhid->lock in hid_ctrl() is too
>> big and because of this it causes a recursive deadlock. "Too big" means
>> the case statement and the call to hid_input_report() do not need to be
>> protected by the spinlock (no URB operations are done inside them).
>>
>> The deadlock happens because in certain rare cases drivers try to grab
>> the lock while handling the ctrl irq which grabs the lock before them
>> as described above. For example newer wacom tablets like 056a:033c try
>> to reschedule proximity reads from wacom_intuos_schedule_prox_event()
>> calling hid_hw_request() -> usbhid_request() -> usbhid_submit_report()
>> which tries to grab the usbhid lock already held by hid_ctrl().
>>
>> There are two ways to get out of this deadlock:
>> 1. Make the drivers work "around" the ctrl critical region, in the
>> wacom case for ex. by delaying the scheduling of the proximity read
>> request itself to a workqueue.
>> 2. Shrink the critical region so the usbhid lock protects only the
>> instructions which modify usbhid state, calling hid_input_report()
>> with the spinlock unlocked, allowing the device driver to grab the
>> lock first, finish and then grab the lock afterwards in hid_ctrl().
>>
>> This patch implements the 2nd solution.
>>
>> Signed-off-by: Ioan-Adrian Ratiu 
>> ---
>>  drivers/hid/usbhid/hid-core.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
>> index 36712e9..5dd426f 100644
>> --- a/drivers/hid/usbhid/hid-core.c
>> +++ b/drivers/hid/usbhid/hid-core.c
>> @@ -477,8 +477,6 @@ static void hid_ctrl(struct urb *urb)
>>   struct usbhid_device *usbhid = hid->driver_data;
>>   int unplug = 0, status = urb->status;
>>
>> - spin_lock(>lock);
>> -
>>   switch (status) {
>>   case 0: /* success */
>>   if (usbhid->ctrl[usbhid->ctrltail].dir == USB_DIR_IN)
>> @@ -498,6 +496,8 @@ static void hid_ctrl(struct urb *urb)
>>   hid_warn(urb->dev, "ctrl urb status %d received\n", status);
>>   }
>>
>> + spin_lock(>lock);
>> +
>>   if (unplug) {
>>   usbhid->ctrltail = usbhid->ctrlhead;
>>   } else {
>
> Hello again
>
> Can this please be merged in the 4.4? There are quite a few people who have
> their tablets deadlock and don't know how to patch their kernels so are stuck
> waiting for a new release.
>
> The severity of this issue is much bigger than I initially thought. Since I've
> posted on the wacom mailing list that I have a fix for this deadlock I've
> recived lots of email from people complaining of the same problem on a wide
> range of tablets.
>
> Some of those people know how to patch a kernel, some found this patch on the
> mailing list and tested it and confirmed that it works on the wacom mailing
> list (you can verify the deadlock + fix on that mailing list).
>
> Thank you,
> Adrian


On Tue, Dec 1, 2015 at 4:00 PM, Ping Cheng  wrote:
> Hi Jiri,
>
> Since wacom_intuos_schedule_prox_event() was introduced on March 5,
> 2015, the call was first used in kernel 3.19. Shouldn't we back-port
> this patch to other stable versions?
>
> Thank you.
>
> Ping

I've been prompted to resurrect this question after fielding yet
another question from a user encountering a locked-up system. We
received a surprising number of reports of people encountering this
issue in the past several weeks and that's only going to get worse as
4.4 works its way into distros. Integrating back even just to 4.4
instead of 3.19 would be appreciated since this issue has (so far)
only ever been encountered with the new Intuos tablets that were added
in 4.4...

Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one  /
(That is to say, eight) to the two, /
But you can’t take seven from three,/
So you look at the sixty-fours
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] hid: usbhid: hid-core: fix recursive deadlock

[Ping Cheng]

Hi Jiri,

On Tue, Dec 1, 2015 at 8:36 AM, Jiri Kosina  wrote:
> On Fri, 20 Nov 2015, Ioan-Adrian Ratiu wrote:
>
>> The critical section protected by usbhid->lock in hid_ctrl() is too
>> big and because of this it causes a recursive deadlock. "Too big" means
>> the case statement and the call to hid_input_report() do not need to be
>> protected by the spinlock (no URB operations are done inside them).
>>
>> The deadlock happens because in certain rare cases drivers try to grab
>> the lock while handling the ctrl irq which grabs the lock before them
>> as described above. For example newer wacom tablets like 056a:033c try
>> to reschedule proximity reads from wacom_intuos_schedule_prox_event()
>> calling hid_hw_request() -> usbhid_request() -> usbhid_submit_report()
>> which tries to grab the usbhid lock already held by hid_ctrl().
>>
>> There are two ways to get out of this deadlock:
>> 1. Make the drivers work "around" the ctrl critical region, in the
>> wacom case for ex. by delaying the scheduling of the proximity read
>> request itself to a workqueue.
>> 2. Shrink the critical region so the usbhid lock protects only the
>> instructions which modify usbhid state, calling hid_input_report()
>> with the spinlock unlocked, allowing the device driver to grab the
>> lock first, finish and then grab the lock afterwards in hid_ctrl().
>>
>> This patch implements the 2nd solution.
>>
>> Signed-off-by: Ioan-Adrian Ratiu 
>
> Applied to for-4.5/core branch. Thanks,

Since wacom_intuos_schedule_prox_event() was introduced on March 5,
2015, the call was first used in kernel 3.19. Shouldn't we back-port
this patch to other stable versions?

Thank you.

Ping
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] hid: usbhid: hid-core: fix recursive deadlock

[Jiri Kosina]

On Fri, 20 Nov 2015, Ioan-Adrian Ratiu wrote:

> The critical section protected by usbhid->lock in hid_ctrl() is too
> big and because of this it causes a recursive deadlock. "Too big" means
> the case statement and the call to hid_input_report() do not need to be
> protected by the spinlock (no URB operations are done inside them).
> 
> The deadlock happens because in certain rare cases drivers try to grab
> the lock while handling the ctrl irq which grabs the lock before them
> as described above. For example newer wacom tablets like 056a:033c try
> to reschedule proximity reads from wacom_intuos_schedule_prox_event()
> calling hid_hw_request() -> usbhid_request() -> usbhid_submit_report()
> which tries to grab the usbhid lock already held by hid_ctrl().
> 
> There are two ways to get out of this deadlock:
> 1. Make the drivers work "around" the ctrl critical region, in the
> wacom case for ex. by delaying the scheduling of the proximity read
> request itself to a workqueue.
> 2. Shrink the critical region so the usbhid lock protects only the
> instructions which modify usbhid state, calling hid_input_report()
> with the spinlock unlocked, allowing the device driver to grab the
> lock first, finish and then grab the lock afterwards in hid_ctrl().
> 
> This patch implements the 2nd solution.
> 
> Signed-off-by: Ioan-Adrian Ratiu 

Applied to for-4.5/core branch. Thanks,

-- 
Jiri Kosina
SUSE Labs

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] hid: usbhid: hid-core: fix recursive deadlock

[Ioan-Adrian Ratiu]

On Fri, 20 Nov 2015 22:19:02 +0200
Ioan-Adrian Ratiu  wrote:

> The critical section protected by usbhid->lock in hid_ctrl() is too
> big and because of this it causes a recursive deadlock. "Too big" means
> the case statement and the call to hid_input_report() do not need to be
> protected by the spinlock (no URB operations are done inside them).
> 
> The deadlock happens because in certain rare cases drivers try to grab
> the lock while handling the ctrl irq which grabs the lock before them
> as described above. For example newer wacom tablets like 056a:033c try
> to reschedule proximity reads from wacom_intuos_schedule_prox_event()
> calling hid_hw_request() -> usbhid_request() -> usbhid_submit_report()
> which tries to grab the usbhid lock already held by hid_ctrl().
> 
> There are two ways to get out of this deadlock:
> 1. Make the drivers work "around" the ctrl critical region, in the
> wacom case for ex. by delaying the scheduling of the proximity read
> request itself to a workqueue.
> 2. Shrink the critical region so the usbhid lock protects only the
> instructions which modify usbhid state, calling hid_input_report()
> with the spinlock unlocked, allowing the device driver to grab the
> lock first, finish and then grab the lock afterwards in hid_ctrl().
> 
> This patch implements the 2nd solution.
> 
> Signed-off-by: Ioan-Adrian Ratiu 
> ---
>  drivers/hid/usbhid/hid-core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 36712e9..5dd426f 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -477,8 +477,6 @@ static void hid_ctrl(struct urb *urb)
>   struct usbhid_device *usbhid = hid->driver_data;
>   int unplug = 0, status = urb->status;
>  
> - spin_lock(>lock);
> -
>   switch (status) {
>   case 0: /* success */
>   if (usbhid->ctrl[usbhid->ctrltail].dir == USB_DIR_IN)
> @@ -498,6 +496,8 @@ static void hid_ctrl(struct urb *urb)
>   hid_warn(urb->dev, "ctrl urb status %d received\n", status);
>   }
>  
> + spin_lock(>lock);
> +
>   if (unplug) {
>   usbhid->ctrltail = usbhid->ctrlhead;
>   } else {

Hello again

Can this please be merged in the 4.4? There are quite a few people who have
their tablets deadlock and don't know how to patch their kernels so are stuck
waiting for a new release.

The severity of this issue is much bigger than I initially thought. Since I've
posted on the wacom mailing list that I have a fix for this deadlock I've
recived lots of email from people complaining of the same problem on a wide
range of tablets.

Some of those people know how to patch a kernel, some found this patch on the
mailing list and tested it and confirmed that it works on the wacom mailing
list (you can verify the deadlock + fix on that mailing list).

Thank you,
Adrian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html