Re: [PATCH v2] qla2xxx: Fix race condition between iocb timeout and initialisation

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Ben, 

> On Mar 21, 2018, at 10:58 AM, Ben Hutchings <ben.hutchings@xxxxxxxxxxxxxxx> wrote:
> 
> On Wed, 2018-03-21 at 17:55 +0000, Madhani, Himanshu wrote:
>> Hi Ben, 
>> 
>>> On Mar 21, 2018, at 10:45 AM, Ben Hutchings <ben.hutchings@xxxxxxxxxxxxxxx> wrote:
>>> 
>>> On Wed, 2018-03-21 at 17:17 +0000, Madhani, Himanshu wrote:
>>>> Hi Ben, 
>>>> 
>>>>> On Mar 20, 2018, at 2:36 PM, Ben Hutchings <ben.hutchings@xxxxxxxxxxxxxxx> wrote:
>>>>> 
>>>>> qla2x00_init_timer() calls add_timer() on the iocb timeout timer,
>>>>> which means the timeout function pointer and any data that the
>>>>> function depends on must be initialised beforehand.
>>>>> 
>>>>> Move this initialisation before each call to qla2x00_init_timer().  In
>>>>> some cases qla2x00_init_timer() initialises a completion structure
>>>>> needed by the timeout function, so move the call to add_timer() after
>>>>> that.
>>> 
>>> [...]
>>>> What motivated this patch? are you debugging any crash which was
>>>> helped by moving code around? 
>>> 
>>> I saw the recent fix that added a call to del_timer(), noticed the lack
>>> of synchronisation and then kept looking further.
>>> 
>>>> What I see from this patch is that its moving iocb.cmd.timeout field
>>>> before qla2x00_init_timer(),
>>>> which are completely different from each other. 
>>> 
>>> How are they "completely different"?  qla2x00_init_timer() starts a
>>> timer that will call qla2x00_sp_timeout(), which in turn uses the
>>> iocb_cmd.timeout function pointer.  We should not assume that any
>>> initialisation code after the call to add_timer() will run before the
>>> timer expires, since the scheduler might run other tasks for the whole
>>> time.  It's unlikely but not impossible.
>>> 
>> 
>> I see. I used “completely different” due to the lack of better wording. 
>> 
>> I wanted to get context and understand if there was any issue that would
>> have helped with these changes. 
>> 
>> your explanation does help and I agree that there is window where timer() will 
>> pop before timeout is initialized. 
>> 
>> Let me put this patch in our test cycle for couple days and will respond on this one
> 
> Thanks.
> 
> Ben.
> 
> -- 
> Ben Hutchings
> Software Developer, Codethink Ltd.

Thanks again for the patch. Testing was successful. 

Acked-by: Himanshu Madhani <himanshu.madhani@xxxxxxxxxx>

Thanks,
- Himanshu





[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]

  Powered by Linux