Re: [PATCH] csiostor: Fix backwards locking in the function __csio_unreg_rnode

[Bastien Philbert]

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