Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759309AbcDEQEJ (ORCPT ); Tue, 5 Apr 2016 12:04:09 -0400 Received: from mx1.redhat.com ([209.132.183.28]:60295 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753721AbcDEQEG (ORCPT ); Tue, 5 Apr 2016 12:04:06 -0400 Date: Tue, 5 Apr 2016 12:04:01 -0400 From: Neil Horman To: "Sell, Timothy C" Cc: Iban Rodriguez , "Kershner, David A" , Greg Kroah-Hartman , *S-Par-Maintainer , "devel@driverdev.osuosl.org" , "linux-kernel@vger.kernel.org" Subject: Re: Staging: unisys/verisonic: Correct double unlock Message-ID: <20160405160401.GK11269@hmsreliant.think-freely.org> References: <1459619226-9786-1-git-send-email-iban.rodriguez@ono.com> <20160404143521.GB8807@hmsreliant.think-freely.org> <20160405145758.GD11269@hmsreliant.think-freely.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9987 Lines: 246 On Tue, Apr 05, 2016 at 03:49:57PM +0000, Sell, Timothy C wrote: > > -----Original Message----- > > From: Neil Horman [mailto:nhorman@redhat.com] > > Sent: Tuesday, April 05, 2016 10:58 AM > > To: Sell, Timothy C > > Cc: Iban Rodriguez; Kershner, David A; Greg Kroah-Hartman; Benjamin > > Romer; *S-Par-Maintainer; devel@driverdev.osuosl.org; linux- > > kernel@vger.kernel.org > > Subject: Re: Staging: unisys/verisonic: Correct double unlock > > > > On Mon, Apr 04, 2016 at 09:40:13PM +0000, Sell, Timothy C wrote: > > > > -----Original Message----- > > > > From: Neil Horman [mailto:nhorman@redhat.com] > > > > Sent: Monday, April 04, 2016 10:35 AM > > > > To: Sell, Timothy C > > > > Cc: Iban Rodriguez; Kershner, David A; Greg Kroah-Hartman; Benjamin > > > > Romer; *S-Par-Maintainer; devel@driverdev.osuosl.org; linux- > > > > kernel@vger.kernel.org > > > > Subject: Re: Staging: unisys/verisonic: Correct double unlock > > > > > > > > On Sat, Apr 02, 2016 at 11:20:14PM +0000, Sell, Timothy C wrote: > > > > > > -----Original Message----- > > > > > > From: Iban Rodriguez [mailto:iban.rodriguez@ono.com] > > > > > > Sent: Saturday, April 02, 2016 1:47 PM > > > > > > To: Kershner, David A; Greg Kroah-Hartman; Benjamin Romer; Sell, > > > > Timothy > > > > > > C; Neil Horman > > > > > > Cc: *S-Par-Maintainer; devel@driverdev.osuosl.org; linux- > > > > > > kernel@vger.kernel.org; Iban Rodriguez > > > > > > Subject: Staging: unisys/verisonic: Correct double unlock > > > > > > > > > > > > 'priv_lock' is unlocked twice. The first one is removed and > > > > > > the function 'visornic_serverdown_complete' is now called with > > > > > > 'priv_lock' locked because 'devdata' is modified inside. > > > > > > > > > > > > Signed-off-by: Iban Rodriguez > > > > > > --- > > > > > > drivers/staging/unisys/visornic/visornic_main.c | 1 - > > > > > > 1 file changed, 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/staging/unisys/visornic/visornic_main.c > > > > > > b/drivers/staging/unisys/visornic/visornic_main.c > > > > > > index be0d057346c3..af03f2938fe9 100644 > > > > > > --- a/drivers/staging/unisys/visornic/visornic_main.c > > > > > > +++ b/drivers/staging/unisys/visornic/visornic_main.c > > > > > > @@ -368,7 +368,6 @@ visornic_serverdown(struct visornic_devdata > > > > > > *devdata, > > > > > > } > > > > > > devdata->server_change_state = true; > > > > > > devdata->server_down_complete_func = complete_func; > > > > > > - spin_unlock_irqrestore(&devdata->priv_lock, flags); > > > > > > visornic_serverdown_complete(devdata); > > > > > > } else if (devdata->server_change_state) { > > > > > > dev_dbg(&devdata->dev->device, "%s changing state\n", > > > > > > > > > > I agree there is a bug here involving priv_lock being unlocked > > > > > twice, but this patch isn't the appropriate fix. Reason is, we can NOT > > > > > call visornic_serverdown_complete() while holding a spinlock > > > > > (which is what this patch would cause to occur) because > > > > > visornic_serverdown_complete() might block when it calls > > > > > rtnl_lock() in this code sequence (rtnl_lock() grabs a mutex): > > > > > > > > > > rtnl_lock(); > > > > > dev_close(netdev); > > > > > rtnl_unlock(); > > > > > > > > > > Blocking with a spinlock held is always a bad idea. :-( > > > > > > > > > > > > > You should just get rid of the priv_lock entirely, its not needed. > > > > > > > > priv_lock is used the following functions: > > > > > > > > visornic_serverdown - only called at the end of a tx_timeout reset > > > > operation, so > > > > you are sure that the rx and tx paths are quiesced (i.e. no data access > > > > happening) > > > > > > > > visornic_disable_with_timeout - move the netif_stop_queue operation > > to > > > > the top > > > > of this function and you will be guaranteed no concurrent access in the tx > > > > path > > > > > > > > visornic_enable_with_timeout - same as above, make sure that > > > > netif_start_queue > > > > and napi_enable are at the end of the function and you are guarantted > > no > > > > concurrent access. > > > > > > > > visornic_xmit - The queue lock in the netdev_start_xmit routine > > guarantees > > > > you > > > > single access here from multiple transmits. > > > > > > > > visornic_xmit_timeout - only called on a tx timeout, when you are > > > > guaranteed not > > > > to have concurrent transmit occuing, by definition. > > > > > > > > visornic_rx - the only tests made here are to devdata members that are > > > > altered > > > > in service_resp_queue, and the visornic_rx is only called from > > > > service_resp_queue, so you are guaranteed a stable data structure, as > > there > > > > is > > > > only ever one context in service_resp_queue as its called from the napi > > poll > > > > routine > > > > > > > > service_resp_queue - Same as above, for any given queue, > > > > service_resp_queue only > > > > has one context exectuing at once. > > > > > > > > host_side_disappeared - only called from visornic_remove, when > > implies > > > > that all > > > > associated devices are closed already, guaranteeing single access. > > > > > > > > visornic_remove > > > > visornic_resume - Both of these function only get called when all > > network > > > > interfaces are quiesced. > > > > > > > > just remove the lock and make the minor changes needed to guarantee > > > > isolated > > > > access. It makes the code cleaner and faster > > > > > > > > Neil > > > > > > Neil, > > > > > > Although I would also love to get rid of this lock, I think we still > > > need it, and will attempt to explain. > > > > > > There's a thread of execution present in visornic that doesn't exist > > > in traditional network drivers, which involves the visornic_pause() and > > > visornic_resume() functions registered during: > > > > > > visorbus_register_visor_driver(&visornic_driver) > > > > > > visornic_pause() and visornic_resume() are called on a thread managed > > > by visorbus, in response to messages received from our hypervisor > > > back-end. > > > > > Ok, but you still can get away without the lock. The other lock points are all > > in the tx/rx paths, so insted of holding the lock, stop the transmit queues > > with > > netif_tx_stop_all_queues, and pause the napi instance with napi_disable. > > That > > allows you to guarantee that the tx and rx paths have no execute going on, > > and > > you can complete the serverdown path safely. > > > > > Note that visornic_pause() calls visornic_serverdown(), which is one of > > > the users of priv_lock. (I.e., visornic_serverdown() is called from > > > other places besides the end of a tx_timeout reset operation, which is > > > what you called out in your explanation above). We need priv_lock to > > > do a false --> true transition of devdata->server_change_state in the > > > pause/resume path, so we can prevent this transition from occurring > > > during critical sections in the normal networking path. > > > > > > The comment present on priv_lock's declaration: > > > > > > spinlock_t priv_lock; /* spinlock to access devdata structures */ > > > > > > is indeed inadequate to the point of being misleading. > > > > > > visornic_serverdown() in its present form is hard-to-follow, in > > > addition to having the double-unlock bug. I would prefer if it were > > > corrected and rewritten to look like this (where the main-path falls > > > thru down the left side of the screen): > > > > > Right, but the point of the lock is still to protect the devdata structure, and > > there are ways to do so (in my previous email and point above), without > > needing > > a lock. You just have to ensure mutual exclusion > > > > Neil > > You convinced me; I like the idea. > > Would it be acceptable to fix the immediate double-unlock bug now, and > address the lock-removal along with the other features we have queued > up for after the driver gets out of staging (like interrupt support)? > Thats fine with me, I just personally worry about the "later becomming never" potential. But yeah, if you think you'll get to it, I'm fine with fixing the immediate problem > The lock removal will touch many of our code paths dealing with > back-end hypervisor recovery scenarios, which is historically very > time-consuming to debug and test. Plus it seems like this could be > classified as a 'feature'. > > Thanks Neil. > Thanks! Neil > Tim > > > > > static int > > > visornic_serverdown(struct visornic_devdata *devdata, > > > visorbus_state_complete_func complete_func) > > > { > > > unsigned long flags; > > > int err; > > > > > > spin_lock_irqsave(&devdata->priv_lock, flags); > > > if (devdata->server_change_state) { > > > dev_dbg(&devdata->dev->device, "%s changing state\n", > > > __func__); > > > err = -EINVAL; > > > goto err_unlock; > > > } > > > if (devdata->server_down) { > > > dev_dbg(&devdata->dev->device, "%s already down\n", > > > __func__); > > > err = -EINVAL; > > > goto err_unlock; > > > } > > > if (devdata->going_away) { > > > dev_dbg(&devdata->dev->device, > > > "%s aborting because device removal pending\n", > > > __func__); > > > err = -ENODEV; > > > goto err_unlock; > > > } > > > devdata->server_change_state = true; > > > devdata->server_down_complete_func = complete_func; > > > spin_unlock_irqrestore(&devdata->priv_lock, flags); > > > > > > visornic_serverdown_complete(devdata); > > > return 0; > > > > > > err_unlock: > > > spin_unlock_irqrestore(&devdata->priv_lock, flags); > > > return err; > > > } > > > > > > Tim > > > > > > > > > > > > > -- > > > > > > 1.9.1 > > > > >