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

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 [off-list ref] wrote:

On Fri, 20 Nov 2015 22:19:02 +0200
Ioan-Adrian Ratiu [off-list ref] wrote:

quoted

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 <redacted>
---
 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(&usbhid->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(&usbhid->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 [off-list ref] 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-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html