lkml.org 
[lkml]   [2022]   [Jan]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 5.16 080/200] USB: core: Fix hang in usb_kill_urb by adding memory barriers
    Date
    From: Alan Stern <stern@rowland.harvard.edu>

    commit 26fbe9772b8c459687930511444ce443011f86bf upstream.

    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.

    CC: <stable@vger.kernel.org>
    Reported-and-tested-by: syzbot+76629376e06e2c2ad626@syzkaller.appspotmail.com
    Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
    Link: https://lore.kernel.org/r/Ye8K0QYee0Q0Nna2@rowland.harvard.edu
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    ---
    drivers/usb/core/hcd.c | 14 ++++++++++++++
    drivers/usb/core/urb.c | 12 ++++++++++++
    2 files changed, 26 insertions(+)

    --- a/drivers/usb/core/hcd.c
    +++ b/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);
    --- a/drivers/usb/core/urb.c
    +++ b/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;

    \
     
     \ /
      Last update: 2022-01-31 13:09    [W:4.159 / U:0.032 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site