| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 4 Apr 2016 21:40:13 +0000
From: "Sell, Timothy C" <Timothy.Sell@...sys.com>
To: Neil Horman <nhorman@...hat.com>
CC: Iban Rodriguez <iban.rodriguez@....com>,
"Kershner, David A" <David.Kershner@...sys.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Benjamin Romer <benjamin.romer@...sys.com>,
*S-Par-Maintainer <SParMaintainer@...sys.com>,
"devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: Staging: unisys/verisonic: Correct double unlock
> -----Original Message-----
> From: Neil Horman [mailto:nhorman@...hat.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@...verdev.osuosl.org; linux-
> kernel@...r.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@....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@...verdev.osuosl.org; linux-
> > > kernel@...r.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@....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.
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):
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
> >
Powered by blists - more mailing lists