* Re: [syzbot] INFO: task hung in hub_port_init (2) [not found] <20220120080020.2619-1-hdanton@sina.com> @ 2022-01-20 8:11 ` syzbot 2022-01-20 17:28 ` Alan Stern 0 siblings, 1 reply; 4+ messages in thread From: syzbot @ 2022-01-20 8:11 UTC (permalink / raw) To: gregkh, hdanton, johan, linux-kernel, linux-usb, paskripkin, stern, syzkaller-bugs Hello, syzbot has tested the proposed patch and the reproducer did not trigger any issue: Reported-and-tested-by: syzbot+76629376e06e2c2ad626@syzkaller.appspotmail.com Tested on: commit: 6f59bc24 Add linux-next specific files for 20220118 git tree: git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git kernel config: https://syzkaller.appspot.com/x/.config?x=94e8da4df9ab6319 dashboard link: https://syzkaller.appspot.com/bug?extid=76629376e06e2c2ad626 compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2 patch: https://syzkaller.appspot.com/x/patch.diff?x=101ba7efb00000 Note: testing is done by a robot and is best-effort only. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [syzbot] INFO: task hung in hub_port_init (2) 2022-01-20 8:11 ` [syzbot] INFO: task hung in hub_port_init (2) syzbot @ 2022-01-20 17:28 ` Alan Stern 2022-01-20 17:55 ` syzbot 0 siblings, 1 reply; 4+ messages in thread From: Alan Stern @ 2022-01-20 17:28 UTC (permalink / raw) To: syzbot Cc: gregkh, hdanton, johan, linux-kernel, linux-usb, paskripkin, syzkaller-bugs On Thu, Jan 20, 2022 at 12:11:10AM -0800, syzbot wrote: > Hello, > > syzbot has tested the proposed patch and the reproducer did not trigger any issue: > > Reported-and-tested-by: syzbot+76629376e06e2c2ad626@syzkaller.appspotmail.com > > Tested on: > > commit: 6f59bc24 Add linux-next specific files for 20220118 > git tree: git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git > kernel config: https://syzkaller.appspot.com/x/.config?x=94e8da4df9ab6319 > dashboard link: https://syzkaller.appspot.com/bug?extid=76629376e06e2c2ad626 > compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2 > patch: https://syzkaller.appspot.com/x/patch.diff?x=101ba7efb00000 > > Note: testing is done by a robot and is best-effort only. Attempted fix. Alan Stern #syz test: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/ 6f59bc24 Index: usb-devel/drivers/usb/core/hcd.c =================================================================== --- usb-devel.orig/drivers/usb/core/hcd.c +++ usb-devel/drivers/usb/core/hcd.c @@ -1563,6 +1563,12 @@ int usb_hcd_submit_urb (struct urb *urb, urb->hcpriv = NULL; INIT_LIST_HEAD(&urb->urb_list); atomic_dec(&urb->use_count); + /* + * Order the write of urb->use_count above against the read of + * urb->reject below. Pairs with the memory barriers in + * usb_kill_urb() and usb_poison_urb(). + */ + smp_mb__after_atomic(); atomic_dec(&urb->dev->urbnum); if (atomic_read(&urb->reject)) wake_up(&usb_kill_urb_queue); @@ -1665,6 +1671,13 @@ static void __usb_hcd_giveback_urb(struc usb_anchor_resume_wakeups(anchor); atomic_dec(&urb->use_count); + /* + * Order the write of urb->use_count above against the read of + * urb->reject below. Pairs with the memory barriers in + * usb_kill_urb() and usb_poison_urb(). + */ + smp_mb__after_atomic(); + if (unlikely(atomic_read(&urb->reject))) wake_up(&usb_kill_urb_queue); usb_put_urb(urb); Index: usb-devel/drivers/usb/core/urb.c =================================================================== --- usb-devel.orig/drivers/usb/core/urb.c +++ usb-devel/drivers/usb/core/urb.c @@ -715,6 +715,12 @@ void usb_kill_urb(struct urb *urb) if (!(urb && urb->dev && urb->ep)) return; atomic_inc(&urb->reject); + /* + * Order the write of urb->reject above against the read of + * urb->use_count below. Pairs with the barriers in + * __usb_hcd_giveback_urb() and usb_hcd_submit_urb(). + */ + smp_mb__after_atomic(); usb_hcd_unlink_urb(urb, -ENOENT); wait_event(usb_kill_urb_queue, atomic_read(&urb->use_count) == 0); @@ -756,6 +762,12 @@ void usb_poison_urb(struct urb *urb) if (!urb) return; atomic_inc(&urb->reject); + /* + * Order the write of urb->reject above against the read of + * urb->use_count below. Pairs with the barriers in + * __usb_hcd_giveback_urb() and usb_hcd_submit_urb(). + */ + smp_mb__after_atomic(); if (!urb->dev || !urb->ep) return; ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [syzbot] INFO: task hung in hub_port_init (2) 2022-01-20 17:28 ` Alan Stern @ 2022-01-20 17:55 ` syzbot 2022-01-24 20:23 ` [PATCH] USB: core: Fix hang in usb_kill_urb by adding memory barriers Alan Stern 0 siblings, 1 reply; 4+ messages in thread From: syzbot @ 2022-01-20 17:55 UTC (permalink / raw) To: gregkh, hdanton, johan, linux-kernel, linux-usb, paskripkin, stern, syzkaller-bugs Hello, syzbot has tested the proposed patch and the reproducer did not trigger any issue: Reported-and-tested-by: syzbot+76629376e06e2c2ad626@syzkaller.appspotmail.com Tested on: commit: 6f59bc24 Add linux-next specific files for 20220118 git tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/ kernel config: https://syzkaller.appspot.com/x/.config?x=94e8da4df9ab6319 dashboard link: https://syzkaller.appspot.com/bug?extid=76629376e06e2c2ad626 compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2 patch: https://syzkaller.appspot.com/x/patch.diff?x=1596bbdfb00000 Note: testing is done by a robot and is best-effort only. ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] USB: core: Fix hang in usb_kill_urb by adding memory barriers 2022-01-20 17:55 ` syzbot @ 2022-01-24 20:23 ` Alan Stern 0 siblings, 0 replies; 4+ messages in thread From: Alan Stern @ 2022-01-24 20:23 UTC (permalink / raw) To: Greg KH; +Cc: syzkaller-bugs, USB mailing list The syzbot fuzzer has identified a bug in which processes hang waiting for usb_kill_urb() to return. It turns out the issue is not unlinking the URB; that works just fine. Rather, the problem arises when the wakeup notification that the URB has completed is not received. The reason is memory-access ordering on SMP systems. In outline form, usb_kill_urb() and __usb_hcd_giveback_urb() operating concurrently on different CPUs perform the following actions: CPU 0 CPU 1 ---------------------------- --------------------------------- usb_kill_urb(): __usb_hcd_giveback_urb(): ... ... atomic_inc(&urb->reject); atomic_dec(&urb->use_count); ... ... wait_event(usb_kill_urb_queue, atomic_read(&urb->use_count) == 0); if (atomic_read(&urb->reject)) wake_up(&usb_kill_urb_queue); Confining your attention to urb->reject and urb->use_count, you can see that the overall pattern of accesses on CPU 0 is: write urb->reject, then read urb->use_count; whereas the overall pattern of accesses on CPU 1 is: write urb->use_count, then read urb->reject. This pattern is referred to in memory-model circles as SB (for "Store Buffering"), and it is well known that without suitable enforcement of the desired order of accesses -- in the form of memory barriers -- it is entirely possible for one or both CPUs to execute their reads ahead of their writes. The end result will be that sometimes CPU 0 sees the old un-decremented value of urb->use_count while CPU 1 sees the old un-incremented value of urb->reject. Consequently CPU 0 ends up on the wait queue and never gets woken up, leading to the observed hang in usb_kill_urb(). The same pattern of accesses occurs in usb_poison_urb() and the failure pathway of usb_hcd_submit_urb(). The problem is fixed by adding suitable memory barriers. To provide proper memory-access ordering in the SB pattern, a full barrier is required on both CPUs. The atomic_inc() and atomic_dec() accesses themselves don't provide any memory ordering, but since they are present, we can use the optimized smp_mb__after_atomic() memory barrier in the various routines to obtain the desired effect. This patch adds the necessary memory barriers. Reported-and-tested-by: syzbot+76629376e06e2c2ad626@syzkaller.appspotmail.com Signed-off-by: Alan Stern <stern@rowland.harvard.edu> CC: <stable@vger.kernel.org> --- [as1969] drivers/usb/core/hcd.c | 14 ++++++++++++++ drivers/usb/core/urb.c | 12 ++++++++++++ 2 files changed, 26 insertions(+) Index: usb-devel/drivers/usb/core/hcd.c =================================================================== --- usb-devel.orig/drivers/usb/core/hcd.c +++ usb-devel/drivers/usb/core/hcd.c @@ -1563,6 +1563,13 @@ int usb_hcd_submit_urb (struct urb *urb, urb->hcpriv = NULL; INIT_LIST_HEAD(&urb->urb_list); atomic_dec(&urb->use_count); + /* + * Order the write of urb->use_count above before the read + * of urb->reject below. Pairs with the memory barriers in + * usb_kill_urb() and usb_poison_urb(). + */ + smp_mb__after_atomic(); + atomic_dec(&urb->dev->urbnum); if (atomic_read(&urb->reject)) wake_up(&usb_kill_urb_queue); @@ -1665,6 +1672,13 @@ static void __usb_hcd_giveback_urb(struc usb_anchor_resume_wakeups(anchor); atomic_dec(&urb->use_count); + /* + * Order the write of urb->use_count above before the read + * of urb->reject below. Pairs with the memory barriers in + * usb_kill_urb() and usb_poison_urb(). + */ + smp_mb__after_atomic(); + if (unlikely(atomic_read(&urb->reject))) wake_up(&usb_kill_urb_queue); usb_put_urb(urb); Index: usb-devel/drivers/usb/core/urb.c =================================================================== --- usb-devel.orig/drivers/usb/core/urb.c +++ usb-devel/drivers/usb/core/urb.c @@ -715,6 +715,12 @@ void usb_kill_urb(struct urb *urb) if (!(urb && urb->dev && urb->ep)) return; atomic_inc(&urb->reject); + /* + * Order the write of urb->reject above before the read + * of urb->use_count below. Pairs with the barriers in + * __usb_hcd_giveback_urb() and usb_hcd_submit_urb(). + */ + smp_mb__after_atomic(); usb_hcd_unlink_urb(urb, -ENOENT); wait_event(usb_kill_urb_queue, atomic_read(&urb->use_count) == 0); @@ -756,6 +762,12 @@ void usb_poison_urb(struct urb *urb) if (!urb) return; atomic_inc(&urb->reject); + /* + * Order the write of urb->reject above before the read + * of urb->use_count below. Pairs with the barriers in + * __usb_hcd_giveback_urb() and usb_hcd_submit_urb(). + */ + smp_mb__after_atomic(); if (!urb->dev || !urb->ep) return; ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-01-24 20:33 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20220120080020.2619-1-hdanton@sina.com>
2022-01-20 8:11 ` [syzbot] INFO: task hung in hub_port_init (2) syzbot
2022-01-20 17:28 ` Alan Stern
2022-01-20 17:55 ` syzbot
2022-01-24 20:23 ` [PATCH] USB: core: Fix hang in usb_kill_urb by adding memory barriers Alan Stern
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.