Staging: unisys/verisonic: Correct double unlock

Staging: unisys/verisonic: Correct double unlock

[Iban Rodriguez]

'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",
-- 
1.9.1

RE: Staging: unisys/verisonic: Correct double unlock

[Sell, Timothy C]

> -----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.  :-(

> --
> 1.9.1

Re: Staging: unisys/verisonic: Correct double unlock

[Neil Horman]

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


> > --
> > 1.9.1
> 

RE: Staging: unisys/verisonic: Correct double unlock

[Sell, Timothy C]

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

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

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

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

RE: Staging: unisys/verisonic: Correct double unlock

[Sell, Timothy C]

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

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

Re: Staging: unisys/verisonic: Correct double unlock

[Neil Horman]

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