On Mon, 2015-07-20 at 21:13 +0300, Eugene Shatokhin wrote:
> 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.
Hi,
your analysis is correct and it looks like in addition to your proposed
fix locking needs to be simplified and a common lock to be taken.
Suggestions?
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
