Skip to contentSkip to conversation

FAILED: patch "[PATCH] USB: gadget: f_hid: fix deadlock in f_hidg_write()" failed to apply to 4.9-stable tree

PATCH [email protected] 3 participants 5 messages Mar 30, 2019

Subsystems (2)
Stable branch USB
lore
Share & tools
lore

[email protected]

author maintainer
#0 lore raw

The patch below does not apply to the 4.9-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From 072684e8c58d17e853f8e8b9f6d9ce2e58d2b036 Mon Sep 17 00:00:00 2001
From: Radoslav Gerganov <[email protected]>
Date: Tue, 5 Mar 2019 10:10:34 +0000
Subject: [PATCH] USB: gadget: f_hid: fix deadlock in f_hidg_write()

In f_hidg_write() the write_spinlock is acquired before calling usb_ep_queue() which causes a deadlock when dummy_hcd is being used. This is because dummy_queue() callbacks into f_hidg_req_complete() which tries to acquire the same spinlock. This is (part of) the backtrace when the deadlock occurs:

0xffffffffc06b1410 in f_hidg_req_complete 0xffffffffc06a590a in usb_gadget_giveback_request 0xffffffffc06cfff2 in dummy_queue 0xffffffffc06a4b96 in usb_ep_queue 0xffffffffc06b1eb6 in f_hidg_write 0xffffffff8127730b in __vfs_write 0xffffffff812774d1 in vfs_write 0xffffffff81277725 in SYSC_write

Fix this by releasing the write_spinlock before calling usb_ep_queue()

Reviewed-by James Bottomley <[email protected]>
Tested-by James Bottomley <[email protected]>
Cc [email protected] # 4.11+
Fixes 749494b6bdbb ("usb: gadget: f_hid: fix: Move IN request allocation to set_alt()")
Signed-off-by Radoslav Gerganov <[email protected]>
Signed-off-by Felipe Balbi <[email protected]>
drivers/usb/gadget/function/f_hid.c +3 -3
index 75b113a5b25c..f3816a5c861e 100644
@@ -391,20 +391,20 @@ static ssize_t f_hidg_write(struct file *file, const char __user *buffer,
 	req->complete = f_hidg_req_complete;
 	req->context  = hidg;
 
+	spin_unlock_irqrestore(&hidg->write_spinlock, flags);
+
 	status = usb_ep_queue(hidg->in_ep, req, GFP_ATOMIC);
 	if (status < 0) {
 		ERROR(hidg->func.config->cdev,
 			"usb_ep_queue error on int endpoint %zd\n", status);
-		goto release_write_pending_unlocked;
+		goto release_write_pending;
 	} else {
 		status = count;
 	}
-	spin_unlock_irqrestore(&hidg->write_spinlock, flags);
 
 	return status;
 release_write_pending:
 	spin_lock_irqsave(&hidg->write_spinlock, flags);
-release_write_pending_unlocked:
 	hidg->write_pending = 0;
 	spin_unlock_irqrestore(&hidg->write_spinlock, flags);
#1 lore raw
Quoted 4 lines · On Sat, 2019-03-30 at 20:57 +0100, [email protected] wrote:

The patch below does not apply to the 4.9-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

[...]

Quoted 3 lines
Cc [email protected] # 4.11+
Fixes 749494b6bdbb ("usb: gadget: f_hid: fix: Move IN request

allocation to set_alt()")

The cc: stable specifies 4.11+ because the commit it fixes went in in the 4.11 merge window. Is there something incorrect with the tagging that makes you think it should apply to 4.9?

James

author maintainer
#2 lore raw
Quoted 8 lines · James Bottomley
Quoted 4 lines · On Sat, 2019-03-30 at 20:57 +0100, [email protected] wrote:

The patch below does not apply to the 4.9-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

[...]

Quoted 7 lines
Quoted 3 lines
Cc [email protected] # 4.11+
Fixes 749494b6bdbb ("usb: gadget: f_hid: fix: Move IN request

allocation to set_alt()")

The cc: stable specifies 4.11+ because the commit it fixes went in in the 4.11 merge window. Is there something incorrect with the tagging that makes you think it should apply to 4.9?

Commit 749494b6bdbb originally showed up in 4.11, but it was then backported to 4.10.2 and 4.9.90. That is why I tried to apply it to 4.9.y and when it did not work, sent out the rejection notice.

thanks,

greg k-h

#3 lore raw
Quoted 21 lines · Greg KH
Quoted 14 lines · James Bottomley
Quoted 5 lines · On Sat, 2019-03-30 at 20:57 +0100, [email protected] wrote:

The patch below does not apply to the 4.9-stable tree. If someone wants it applied there, or to any other stable or longterm
tree, then please email the backport, including the original git commit id to <[email protected]>.

[...]

Quoted 3 lines
Cc [email protected] # 4.11+
Fixes 749494b6bdbb ("usb: gadget: f_hid: fix: Move IN request

allocation to set_alt()")

Quoted 5 lines

The cc: stable specifies 4.11+ because the commit it fixes went in in
the 4.11 merge window. Is there something incorrect with the tagging
that makes you think it should apply to 4.9?

Quoted 3 lines

Commit 749494b6bdbb originally showed up in 4.11, but it was then backported to 4.10.2 and 4.9.90. That is why I tried to apply it to 4.9.y and when it did not work, sent out the rejection notice.

Turns out there's a missing patch:

commit 25cd9721c2b16ee0d775e36ec3af31f392003f80

Author: Krzysztof Opasiak <[email protected]>
Date:   Tue Jan 31 18:12:31 2017 +0100
    usb: gadget: f_hid: fix: Don't access hidg->req without spinlock

held

It changes one of the match lines from

	status = usb_ep_queue(hidg->in_ep, hidg->req, GFP_ATOMIC);

to

	status = usb_ep_queue(hidg->in_ep, req, GFP_ATOMIC);

which is why you get the rejection.

James

maintainer
#4 lore raw
In reply to James Bottomley
Quoted 8 lines · James Bottomley
Quoted 4 lines · On Sat, 2019-03-30 at 20:57 +0100, [email protected] wrote:

The patch below does not apply to the 4.9-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

[...]

Quoted 7 lines
Quoted 3 lines
Cc [email protected] # 4.11+
Fixes 749494b6bdbb ("usb: gadget: f_hid: fix: Move IN request

allocation to set_alt()")

The cc: stable specifies 4.11+ because the commit it fixes went in in the 4.11 merge window. Is there something incorrect with the tagging that makes you think it should apply to 4.9?

The commit it fixes was tagged for stable and backported to kernels such as 4.9.

--
Thanks,
Sasha