lkml.org 
[lkml]   [2015]   [Jul]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: Several races in "usbnet" module (kernel 4.1.x)
From
Date
27.07.2015 15:29, Oliver Neukum пишет:
> On Fri, 2015-07-24 at 20:38 +0300, Eugene Shatokhin wrote:
>> 21.07.2015 15:04, Oliver Neukum пишет:
>
>>> 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?
>>
>> Just an idea, I haven't tested it.
>>
>> How about moving the operations with dev->done under &list->lock in
>> defer_bh, while keeping dev->done.lock too and changing
>
> Why keep dev->done.lock?
> Does it make sense at all?

I think it does.

Both skb_queue_tail(&dev->done, skb) called from rx_process() and
skb_dequeue (&dev->done) called from usbnet_bh() take dev->done.lock
internally. So, to synchronize accesses to dev->done, one needs that
lock in defer_bh() too.

>
>> usbnet_terminate_urbs() as described below?
>>
>> Like this:
>> @@ -428,12 +428,12 @@ static enum skb_state defer_bh(struct usbnet *dev,
>> struct sk_buff *skb,
>> 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);
>> + spin_unlock(&dev->done.lock);
>> + spin_unlock_irqrestore(&list->lock, flags);
>> return old_state;
>> }
>> -------------------
>>
>> usbnet_terminate_urbs() can then be changed as follows:
>>
>> @@ -749,6 +749,20 @@ EXPORT_SYMBOL_GPL(usbnet_unlink_rx_urbs);
>>
>>
>> /*-------------------------------------------------------------------------*/
>>
>> +static void wait_skb_queue_empty(struct sk_buff_head *q)
>> +{
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&q->lock, flags);
>> + while (!skb_queue_empty(q)) {
>> + spin_unlock_irqrestore(&q->lock, flags);
>> + schedule_timeout(msecs_to_jiffies(UNLINK_TIMEOUT_MS));
>> + set_current_state(TASK_UNINTERRUPTIBLE);
>
> I suppose you want to invert those lines

Do you mean
+set_current_state(TASK_UNINTERRUPTIBLE);
+schedule_timeout(msecs_to_jiffies(UNLINK_TIMEOUT_MS));
?

>
>> + spin_lock_irqsave(&q->lock, flags);
>> + }
>> + spin_unlock_irqrestore(&q->lock, flags);
>> +}
>> +
>
> Your changes make sense, but it locks to me as if a lock would
> become totally redundant.
>

Regards,

Eugene



\
 
 \ /
  Last update: 2015-07-27 16:21    [W:0.230 / U:0.088 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site