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

[Eugene Shatokhin]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/