Re: [PATCH] qla2xxx: Fix dpc_thread race on the module unload

[Vladislav Bolkhovitin <vst@vlnb.net>]

Andrew Vasquez wrote:
> On Mon, 28 Jul 2008, Vladislav Bolkhovitin wrote:
> 
>> This patch fixes race on dpc_thread field of struct scsi_qla_host,
>> which can lead to crash on the module unload.
>>
>> This patch is against 2.6.26
>>
>> Signed-off-by: Vladislav Bolkhovitin <vst@vlnb.net>
>>
>>  qla_def.h |    1 +
>>  qla_mbx.c |    2 +-
>>  qla_os.c  |   36 ++++++++++++++++++++++++++----------
>>  3 files changed, 28 insertions(+), 11 deletions(-)
>>
>> diff -upr linux-2.6.26/drivers/scsi/qla2xxx/qla_def.h linux-2.6.26/drivers/scsi/qla2xxx/qla_def.h
>> --- linux-2.6.26/drivers/scsi/qla2xxx/qla_def.h	2008-07-14 01:51:29.000000000 +0400
>> +++ linux-2.6.26/drivers/scsi/qla2xxx/qla_def.h	2008-07-24 10:07:39.000000000 +0400
>> @@ -2425,6 +2436,7 @@ typedef struct scsi_qla_host {
>>  	void			*sfp_data;
>>  	dma_addr_t		sfp_data_dma;
>>  
>> +	spinlock_t		dpc_lock;
> 
> As James mentioned in another email, there's something else going on
> with qla2xxx shutdown process which should be addressed...
> 
>>  	struct task_struct	*dpc_thread;
>>  	uint8_t dpc_active;                  /* DPC routine is active */
>>  
>> diff -upr linux-2.6.26/drivers/scsi/qla2xxx/qla_mbx.c linux-2.6.26/drivers/scsi/qla2xxx/qla_mbx.c
>> --- linux-2.6.26/drivers/scsi/qla2xxx/qla_mbx.c	2008-07-14 01:51:29.000000000 +0400
>> +++ linux-2.6.26/drivers/scsi/qla2xxx/qla_mbx.c	2008-07-23 19:27:20.000000000 +0400
>> @@ -2683,7 +2687,7 @@ qla24xx_report_id_acquisition(scsi_qla_h
>>  		set_bit(VP_IDX_ACQUIRED, &vha->vp_flags);
>>  		set_bit(VP_DPC_NEEDED, &ha->dpc_flags);
>>  
>> -		wake_up_process(ha->dpc_thread);
>> +		qla2xxx_wake_dpc(ha);
> 
> Ok, your change looks correct here...  This is defintely the correct
> way the qla24xx_report_id_acquisition() routine should be triggering
> the DPC thread.
> 
> I'm guessing the backtrace in "Gal Rosen's" report pointed to
> qla24xx_report_id_acquisition()?  If, so, I'm not entirely sure we
> need anything more to 'protect' tear-down...

Nope, taking only one that hunk from this patch isn't sufficient. Around 
dpc_thread there is pretty simple and classical race. You can't do

if (x != NULL)
	y = *x;

without any protection, if x can be set to NULL by another thread. It 
can happen exactly between "if" and "*x" and hence lead to a crash, correct?

What this patch does is adding such protection.

Vlad