From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bastien Philbert Subject: Re: [PATCH] csiostor: Fix backwards locking in the function __csio_unreg_rnode Date: Wed, 6 Apr 2016 10:36:35 -0400 Message-ID: <57051EF3.2070508@gmail.com> References: <1459891143-20451-1-git-send-email-bastienphilbert@gmail.com> <57050D62.3090802@gmail.com> <1459949907.2372.13.camel@linux.vnet.ibm.com> <57051913.4050304@gmail.com> <1459952680.2372.25.camel@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Return-path: Received: from mail-qg0-f68.google.com ([209.85.192.68]:33286 "EHLO mail-qg0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751639AbcDFOgi (ORCPT ); Wed, 6 Apr 2016 10:36:38 -0400 In-Reply-To: <1459952680.2372.25.camel@linux.vnet.ibm.com> Sender: linux-scsi-owner@vger.kernel.org List-Id: linux-scsi@vger.kernel.org To: James Bottomley , Julian Calaby Cc: "Martin K. Petersen" , linux-scsi , "linux-kernel@vger.kernel.org" On 2016-04-06 10:24 AM, James Bottomley wrote: > On Wed, 2016-04-06 at 10:11 -0400, Bastien Philbert wrote: >> >> On 2016-04-06 09:38 AM, James Bottomley wrote: >>> 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 >>>>> 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 >>>>>> --- >>>>>> 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 >>> >>> >> Yes I do. > > You mean you don't see the lockdep assert, since you're asking to drop > the patch? > >> For now just drop the patch but I am still concerned that we are >> double unlocking here. > > Really, no. The pattern in the code indicates the lock is expected to > be held. This can be wrong (sometimes code moves or people forget), > but if it is wrong we'll get an assert about unlock of an already > unlocked lock. If there's no assert, the lock is held on entry and the > code is correct. > > You're proposing patches based on misunderstandings of the code which > aren't backed up by actual issues and wasting everyone's time to look > at them. Please begin with the hard evidence of a problem first, so > post the lockdep assert in the changelog so we know there's a real > problem. > > James > Certainly James. I think I just got carried away with the last few patches :(. Bastien