Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754229AbbG0Nxy (ORCPT ); Mon, 27 Jul 2015 09:53:54 -0400 Received: from collab.rosalab.ru ([195.19.76.181]:44977 "EHLO collab.rosalab.ru" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752553AbbG0Nxw (ORCPT ); Mon, 27 Jul 2015 09:53:52 -0400 Subject: Re: Several races in "usbnet" module (kernel 4.1.x) To: Oliver Neukum References: <55AD3A41.2040100@rosalab.ru> <1437480243.3823.5.camel@suse.com> <55B27805.90601@rosalab.ru> <1438000168.32457.18.camel@suse.com> Cc: LKML , linux-usb@vger.kernel.org, netdev@vger.kernel.org From: Eugene Shatokhin Organization: ROSA Message-ID: <55B637ED.2080905@rosalab.ru> Date: Mon, 27 Jul 2015 16:53:49 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.0.1 MIME-Version: 1.0 In-Reply-To: <1438000168.32457.18.camel@suse.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2607 Lines: 88 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/