• Alan Stern's avatar
    USB: core: Fix hang in usb_kill_urb by adding memory barriers · 26fbe977
    Alan Stern authored
    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: default avatarAlan Stern <stern@rowland.harvard.edu>
    Link: https://lore.kernel.org/r/Ye8K0QYee0Q0Nna2@rowland.harvard.eduSigned-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
    26fbe977
urb.c 34.1 KB