linux-kernel - Re: [PATCH] staging: unisys: visorbus: visorchannel: Refactor locking code to be statically deterministic.

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 19 Oct 2016 22:56:43 +0100
From:   Chuckleberryfinn <chuckleberryfinn@...il.com>
To:     "Sell, Timothy C" <Timothy.Sell@...sys.com>
Cc:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        *S-Par-Maintainer <SParMaintainer@...sys.com>,
        "devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Kershner, David A" <David.Kershner@...sys.com>
Subject: Re: [PATCH] staging: unisys: visorbus: visorchannel: Refactor
 locking code to be statically deterministic.

On Wed, Oct 19, 2016 at 05:00:53PM +0000, Sell, Timothy C wrote:
> On Wednesday, October 19, 2016 7:31 AM, Cathal Mullaney wrote:
> > This patch makes locking in visorchannel_signalempty statically deterministic.
> > As a result this patch fixes the sparse warning:
> > Context imbalance in 'visorchannel_signalempty' - different lock contexts for
> > basic block.
> > 
> > The logic of the locking code doesn't change but the layout of the original
> > code is "frowned upon"
> > according to mails on sparse context checking.
> > Refactoring removes the warning and makes the code more readable.
> > 
> > Signed-off-by: Cathal Mullaney <chuckleberryfinn@...il.com>
> > ---
> >  drivers/staging/unisys/visorbus/visorchannel.c | 26 +++++++++++++++++---
> > ------
> >  1 file changed, 17 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/staging/unisys/visorbus/visorchannel.c
> > b/drivers/staging/unisys/visorbus/visorchannel.c
> > index a1381eb..1eea5d8 100644
> > --- a/drivers/staging/unisys/visorbus/visorchannel.c
> > +++ b/drivers/staging/unisys/visorbus/visorchannel.c
> > @@ -300,22 +300,30 @@
> > ---
> >  bool
> >  visorchannel_signalempty(struct visorchannel *channel, u32 queue)
> >  {
> >  	unsigned long flags = 0;
> > -	struct signal_queue_header sig_hdr;
> >  	bool rc = false;
> 
> It appears as if you no longer need to initialize 'rc' above.
> 
> Although this is NOT caused by your patch, it also looks like 'flags'
> is being unnecessarily initialized.  You may want to fix that too
> while you're in the neighborhood.
> 
> (Kernel folks seem to frown on unnecessary variable initializations.)
> 
> > 
> > -	if (channel->needs_lock)
> > -		spin_lock_irqsave(&channel->remove_lock, flags);
> > +	if (!channel->needs_lock)
> > +		return queue_empty(channel, queue);
> > 
> > -	if (sig_read_header(channel, queue, &sig_hdr))
> > -		rc = true;
> > -	if (sig_hdr.head == sig_hdr.tail)
> > -		rc = true;
> > -	if (channel->needs_lock)
> > -		spin_unlock_irqrestore(&channel->remove_lock, flags);
> > +	spin_lock_irqsave(&channel->remove_lock, flags);
> > +	rc = queue_empty(channel, queue);
> > +	spin_unlock_irqrestore(&channel->remove_lock, flags);
> > 
> >  	return rc;
> >  }
> > --
> > 2.7.4
> 
> Besides that, your patch looks good to me.  Thanks.
> 
> - Tim Sell
>

Thanks for your feedback Tim.
Submitted v2 with the suggested updates.
Thanks again.

Kind regards,
Cathal