Re: Staging: unisys/verisonic: Correct double unlock

From: Neil Horman
Date: Tue Apr 05 2016 - 10:58:09 EST


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