Question about scsi_qla_host_t.fcports list protection (qla2xxx)

Question about scsi_qla_host_t.fcports list protection (qla2xxx)

[Vladislav Bolkhovitin]

Hello

Looking at the qla2xxx driver source code I noticed that 
scsi_qla_host_t.fcports is used without any protection. For instance, 
qla2x00_loop_reset() walks over this list. This function is called from 
qla2xxx_eh_bus_reset(), which could be called at any time. What does 
protect fcports from being changed, eg, by some async loop event during 
the walking and eventually oops'ing? Do I miss something? There are many 
such unprotected fcports list usages in the driver.

Vlad

Re: [Suspected Spam:#] Question about scsi_qla_host_t.fcports list protection (qla2xxx)

[Andrew Vasquez]

On Tue, 15 Aug 2006, Vladislav Bolkhovitin wrote:

> Looking at the qla2xxx driver source code I noticed that 
> scsi_qla_host_t.fcports is used without any protection. For instance, 
> qla2x00_loop_reset() walks over this list. This function is called from 
> qla2xxx_eh_bus_reset(), which could be called at any time. What does 
> protect fcports from being changed, eg, by some async loop event during 
> the walking and eventually oops'ing? Do I miss something? There are many 
> such unprotected fcports list usages in the driver.

Nothing is ever removed from the fcports list during driver execution.
fcports are also only added from a single context (post init-time)
within the DPC thread.  We are safe here.

Regards,
Andrew Vasquez

Re: Question about scsi_qla_host_t.fcports list protection (qla2xxx)

[Vladislav Bolkhovitin]

Andrew Vasquez wrote:
> On Tue, 15 Aug 2006, Vladislav Bolkhovitin wrote:
> 
> 
>>Looking at the qla2xxx driver source code I noticed that 
>>scsi_qla_host_t.fcports is used without any protection. For instance, 
>>qla2x00_loop_reset() walks over this list. This function is called from 
>>qla2xxx_eh_bus_reset(), which could be called at any time. What does 
>>protect fcports from being changed, eg, by some async loop event during 
>>the walking and eventually oops'ing? Do I miss something? There are many 
>>such unprotected fcports list usages in the driver.
> 
> 
> Nothing is ever removed from the fcports list during driver execution.
> fcports are also only added from a single context (post init-time)
> within the DPC thread.  We are safe here.

Sorry, I don't think so. For lockless iterating over a list concurrently 
with the addition to it you need to use *_rcu() list functions and do 
some RCU locking or, at least, disable preemption. Otherwise it is not safe.

Also, I don't think that keeping dead FC ports entries forever is a good 
idea. What does prevent you from deleting them, for instance, after 
fc_remote_port_delete() call?

Vlad

Re: [Suspected Spam:#] Question about scsi_qla_host_t.fcports list protection (qla2xxx)

[Vladislav Bolkhovitin]

Andrew Vasquez wrote:
> On Tue, 15 Aug 2006, Vladislav Bolkhovitin wrote:
> 
> 
>>Looking at the qla2xxx driver source code I noticed that 
>>scsi_qla_host_t.fcports is used without any protection. For instance, 
>>qla2x00_loop_reset() walks over this list. This function is called from 
>>qla2xxx_eh_bus_reset(), which could be called at any time. What does 
>>protect fcports from being changed, eg, by some async loop event during 
>>the walking and eventually oops'ing? Do I miss something? There are many 
>>such unprotected fcports list usages in the driver.
> 
> 
> Nothing is ever removed from the fcports list during driver execution.
> fcports are also only added from a single context (post init-time)
> within the DPC thread. 

How about qla2x00_send_login_iocb_cb(), where an entry is added to 
fcports list and which is called from IRQ context from 
qla2x00_process_iodesc()?

> We are safe here.

> Regards,
> Andrew Vasquez
> 

Re: Question about scsi_qla_host_t.fcports list protection (qla2xxx)

[Vladislav Bolkhovitin]

Vladislav Bolkhovitin wrote:
> Andrew Vasquez wrote:
> 
>> On Tue, 15 Aug 2006, Vladislav Bolkhovitin wrote:
>>
>>
>>> Looking at the qla2xxx driver source code I noticed that 
>>> scsi_qla_host_t.fcports is used without any protection. For instance, 
>>> qla2x00_loop_reset() walks over this list. This function is called 
>>> from qla2xxx_eh_bus_reset(), which could be called at any time. What 
>>> does protect fcports from being changed, eg, by some async loop event 
>>> during the walking and eventually oops'ing? Do I miss something? 
>>> There are many such unprotected fcports list usages in the driver.
>>
>>
>>
>> Nothing is ever removed from the fcports list during driver execution.
>> fcports are also only added from a single context (post init-time)
>> within the DPC thread. 
> 
> 
> How about qla2x00_send_login_iocb_cb(), where an entry is added to 
> fcports list and which is called from IRQ context from 
> qla2x00_process_iodesc()?

Sorry for the noise, I just checked in 2.6.18-rc6 and from it all that 
code was removed. Anyway, the problem with unsafe fcports list iterating 
remains valid.

Vlad

>> We are safe here.
> 
> 
>> Regards,
>> Andrew Vasquez
>>
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>