Re: Several races in "usbnet" module (kernel 4.1.x)

[Eugene Shatokhin <eugene.shatokhin@rosalab.ru>]

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