RE: Staging: unisys/verisonic: Correct double unlock

[Sell, Timothy C]

> -----Original Message-----
> From: Neil Horman [mailto:nhorman@xxxxxxxxxx]
> 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@xxxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxx]
> > > 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@xxxxxxxxxxxxxxxxxxxx; linux-
> > > kernel@xxxxxxxxxxxxxxx
> > > 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@xxxxxxx]
> > > > > 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@xxxxxxxxxxxxxxxxxxxx; linux-
> > > > > kernel@xxxxxxxxxxxxxxx; 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@xxxxxxx>
> > > > > ---
> > > > >  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)?

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.

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
> > > >