Re: Staging: unisys/verisonic: Correct double unlock

[Neil Horman <nhorman@redhat.com>]

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 <iban.rodriguez@ono.com>
> > > > > > ---
> > > > > >  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
> > > > >