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

From: Bastien Philbert
Date: Wed Apr 06 2016 - 10:36:47 EST




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