Re: Staging: unisys/verisonic: Correct double unlock

[Neil Horman]

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

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