Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1193127 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: Several races in "usbnet" module (kernel 4.1.x) Eugene Shatokhin <eugene.shatokhin@rosalab.ru> - 2015-07-27 16:00 +0200

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [standalone]

Back to top | Article view | linux.kernel

csiph-web