lkml.org 
[lkml]   [2015]   [Jul]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectSeveral races in "usbnet" module (kernel 4.1.x)
    Date
    Hi,

    I have recently found several data races in "usbnet" module, checked on
    vanilla kernel 4.1.0 on x86_64. The races do actually happen, I have
    confirmed it by adding delays and using hardware breakpoints to detect
    the conflicting memory accesses (with RaceHound tool,
    https://github.com/winnukem/racehound).

    I have not analyzed yet how harmful these races are (if they are), but
    it is better to report them anyway, I think.

    Everything was checked using YOTA 4G LTE Modem that works via "usbnet"
    and "cdc_ether" kernel modules.
    --------------------------

    [Race #1]

    Race on skb_queue ('next' pointer) between usbnet_stop() and rx_complete().

    Reproduced that by unplugging the device while the system was
    downloading a large file from the Net.

    Here is part of the call stack with the code where the changes to the
    queue happen:

    #0 __skb_unlink (skbuff.h:1517)
    prev->next = next;
    #1 defer_bh (usbnet.c:430)
    spin_lock_irqsave(&list->lock, flags);
    old_state = entry->state;
    entry->state = state;
    __skb_unlink(skb, list);
    spin_unlock(&list->lock);
    spin_lock(&dev->done.lock);
    __skb_queue_tail(&dev->done, skb);
    if (dev->done.qlen == 1)
    tasklet_schedule(&dev->bh);
    spin_unlock_irqrestore(&dev->done.lock, flags);
    #2 rx_complete (usbnet.c:640)
    state = defer_bh(dev, skb, &dev->rxq, state);

    At the same time, the following code repeatedly checks if the queue is
    empty and reads the same values concurrently with the above changes:

    #0 usbnet_terminate_urbs (usbnet.c:765)
    /* maybe wait for deletions to finish. */
    while (!skb_queue_empty(&dev->rxq)
    && !skb_queue_empty(&dev->txq)
    && !skb_queue_empty(&dev->done)) {
    schedule_timeout(msecs_to_jiffies(UNLINK_TIMEOUT_MS));
    set_current_state(TASK_UNINTERRUPTIBLE);
    netif_dbg(dev, ifdown, dev->net,
    "waited for %d urb completions\n", temp);
    }
    #1 usbnet_stop (usbnet.c:806)
    if (!(info->flags & FLAG_AVOID_UNLINK_URBS))
    usbnet_terminate_urbs(dev);

    For example, it is possible that the skb is removed from dev->rxq by
    __skb_unlink() before the check "!skb_queue_empty(&dev->rxq)" in
    usbnet_terminate_urbs() is made. It is also possible in this case that
    the skb is added to dev->done queue after "!skb_queue_empty(&dev->done)"
    is checked. So usbnet_terminate_urbs() may stop waiting and return while
    dev->done queue still has an item.
    --------------------------

    Unrelated the that race, if the goal of that while loop in
    usbnet_terminate_urbs() is to wait till all three queues (dev->rxq,
    dev->txq, dev->done) become empty, perhaps the code should be changed as
    follows:

    while (!skb_queue_empty(&dev->rxq)
    - && !skb_queue_empty(&dev->txq)
    - && !skb_queue_empty(&dev->done)) {
    + || !skb_queue_empty(&dev->txq)
    + || !skb_queue_empty(&dev->done)) {
    schedule_timeout(...));
    --------------------------

    [Race #2]

    Races on dev->rx_qlen. Reproduced these by repeatedly changing MTU (1500
    <-> 1400) while downloading large files.

    dev->rx_qlen is written to here:
    #0 usbnet_update_max_qlen (usbnet.c:351)
    case USB_SPEED_HIGH:
    dev->rx_qlen = MAX_QUEUE_MEMORY / dev->rx_urb_size;
    #1 __handle_link_change (usbnet.c:1049)
    /* hard_mtu or rx_urb_size may change during link change */
    usbnet_update_max_qlen(dev);
    #2 usbnet_deferred_kevent (usbnet.c:1172)
    if (test_bit (EVENT_LINK_CHANGE, &dev->flags))
    __handle_link_change(dev);

    Here are the conflicting reads from dev->rx_qlen (via RX_QLEN(dev)), 3
    code locations:

    * usbnet_bh (usbnet.c:1492)
    if (temp < RX_QLEN(dev)) { ...
    * usbnet_bh (usbnet.c:1499)
    if (dev->rxq.qlen < RX_QLEN(dev)) ...
    * rx_alloc_submit (usbnet.c:1431)
    for (i = 0; i < 10 && dev->rxq.qlen < RX_QLEN(dev); i++) { ...
    --------------------------

    [Race #3]

    Similar to race #2 but on dev->tx_qlen. I reproduced it the same way.

    dev->tx_qlen is written to here:
    #0 usbnet_update_max_qlen (usbnet.c:352)
    case USB_SPEED_HIGH:
    dev->rx_qlen = MAX_QUEUE_MEMORY / dev->rx_urb_size;
    dev->tx_qlen = MAX_QUEUE_MEMORY / dev->hard_mtu;
    #1 __handle_link_change (usbnet.c:1049)
    /* hard_mtu or rx_urb_size may change during link change */
    usbnet_update_max_qlen(dev);
    #2 usbnet_deferred_kevent (usbnet.c:1172)
    if (test_bit (EVENT_LINK_CHANGE, &dev->flags))
    __handle_link_change(dev);

    Here are the conflicting reads from dev->tx_qlen (via TX_QLEN(dev)), 2
    code locations:

    * usbnet_bh (usbnet.c:1502)
    if (dev->txq.qlen < TX_QLEN (dev))
    netif_wake_queue (dev->net);
    * usbnet_start_xmit (usbnet.c:1398)
    if (dev->txq.qlen >= TX_QLEN (dev))
    netif_stop_queue (net);
    --------------------------

    [Race #4]

    Race on dev->flags. It happened when I unplugged the device while a
    large file was being downloaded.

    dev->flags is set to 0 here:

    #0 usbnet_stop (usbnet.c:816)
    /* deferred work (task, timer, softirq) must also stop.
    * can't flush_scheduled_work() until we drop rtnl (later),
    * else workers could deadlock; so make workers a NOP.
    */
    dev->flags = 0;
    del_timer_sync (&dev->delay);
    tasklet_kill (&dev->bh);

    And here, the code clears EVENT_RX_KILL bit in dev->flags, which may
    execute concurrently with the above operation:
    #0 clear_bit (bitops.h:113, inlined)
    #1 usbnet_bh (usbnet.c:1475)
    /* restart RX again after disabling due to high error rate */
    clear_bit(EVENT_RX_KILL, &dev->flags);

    If clear_bit() is atomic w.r.t. setting dev->flags to 0, this race is
    not a problem, I guess. Otherwise, it may be.
    --------------------------

    [Race #5]

    Race on dev->rx_urb_size. I reproduced it a similar way as the races #2
    and #3 (changing MTU while downloading files).

    dev->rx_urb_size is written to here:
    #0 usbnet_change_mtu (usbnet.c:392)
    dev->rx_urb_size = dev->hard_mtu;

    Here is the conflicting read from dev->rx_urb_size:
    * rx_submit (usbnet.c:467)
    size_t size = dev->rx_urb_size;
    --------------------------

    Regards,
    Eugene

    --
    Eugene Shatokhin, ROSA
    www.rosalab.com


    \
     
     \ /
      Last update: 2015-07-20 20:41    [W:3.740 / U:0.316 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site