linux-kernel - Re: [PATCH] csiostor: Fix backwards locking in the function __csio_unreg

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]

Message-ID: <1459949907.2372.13.camel@linux.vnet.ibm.com>
Date:	Wed, 06 Apr 2016 09:38:27 -0400
From:	James Bottomley <jejb@...ux.vnet.ibm.com>
To:	Bastien Philbert <bastienphilbert@...il.com>,
	Julian Calaby <julian.calaby@...il.com>
Cc:	"Martin K. Petersen" <martin.petersen@...cle.com>,
	linux-scsi <linux-scsi@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] csiostor: Fix backwards locking in the function
 __csio_unreg_rnode

On Wed, 2016-04-06 at 09:21 -0400, Bastien Philbert wrote:
> 
> On 2016-04-06 03:48 AM, Julian Calaby wrote:
> > Hi Bastien,
> > 
> > On Wed, Apr 6, 2016 at 7:19 AM, Bastien Philbert
> > <bastienphilbert@...il.com> wrote:
> > > This fixes backwards locking in the function __csio_unreg_rnode
> > > to
> > > properly lock before the call to the function csio_unreg_rnode
> > > and
> > > not unlock with spin_unlock_irq as this would not allow the
> > > proper
> > > protection for concurrent access on the shared csio_hw structure
> > > pointer hw. In addition switch the locking after the critical
> > > region
> > > function call to properly unlock instead with spin_unlock_irq on
> > > 
> > > Signed-off-by: Bastien Philbert <bastienphilbert@...il.com>
> > > ---
> > >  drivers/scsi/csiostor/csio_rnode.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/scsi/csiostor/csio_rnode.c
> > > b/drivers/scsi/csiostor/csio_rnode.c
> > > index e9c3b04..029a09e 100644
> > > --- a/drivers/scsi/csiostor/csio_rnode.c
> > > +++ b/drivers/scsi/csiostor/csio_rnode.c
> > > @@ -580,9 +580,9 @@ __csio_unreg_rnode(struct csio_rnode *rn)
> > >                 ln->last_scan_ntgts--;
> > >         }
> > > 
> > > -       spin_unlock_irq(&hw->lock);
> > > -       csio_unreg_rnode(rn);
> > >         spin_lock_irq(&hw->lock);
> > > +       csio_unreg_rnode(rn);
> > > +       spin_unlock_irq(&hw->lock);
> > 
> > Are you _certain_ this is correct? This construct usually appears
> > when
> > a function has a particular lock held, then needs to unlock it to
> > call
> > some other function. Are you _certain_ that this isn't the case?
> > 
> > Thanks,
> > 
> Yes I am pretty certain this is correct. I checked the paths that
> called this function
> and it was weired that none of them gradded the spinlock before hand.

That's not good enough.  If your theory is correct, lockdep should be
dropping an already unlocked assertion in this codepath ... do you see
this?

James