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

[Bastien Philbert]

On 2016-04-06 01:14 PM, James Bottomley wrote:
> On Wed, 2016-04-06 at 10:36 -0400, Bastien Philbert wrote:
>>
>> 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 :(.
> 
> Is this Nick Krause?  An email reply that Martin forwarded but the list
> didn't pick up (because it had a html part) suggests this.  What you're
> doing is what got you banned from LKML the last time: sending patches
> without evidence there's a problem or understanding the code you're
> patching.  Repeating the behaviour under a new identity isn't going to
> help improve your standing.
> 
> James
> 
No I am not Nick Krause. I am just aware of how he got banned a few years ago.
That email was a mistake by typo and was hoping nobody picked it up as they 
would then believe I was Nick Krause.
Bastien