Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753739AbbG0M3j (ORCPT ); Mon, 27 Jul 2015 08:29:39 -0400 Received: from mx2.suse.de ([195.135.220.15]:48502 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753603AbbG0M3h (ORCPT ); Mon, 27 Jul 2015 08:29:37 -0400 Message-ID: <1438000168.32457.18.camel@suse.com> Subject: Re: Several races in "usbnet" module (kernel 4.1.x) From: Oliver Neukum To: Eugene Shatokhin Cc: LKML , linux-usb@vger.kernel.org, netdev@vger.kernel.org Date: Mon, 27 Jul 2015 14:29:28 +0200 In-Reply-To: <55B27805.90601@rosalab.ru> References: <55AD3A41.2040100@rosalab.ru> <1437480243.3823.5.camel@suse.com> <55B27805.90601@rosalab.ru> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.12.11 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2136 Lines: 72 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? > 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 > + 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 Oliver -- 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/