Re: [ofa-general][PATCH 4/4] SRP fail-over faster

Vu Pham
2009-10-14T21:11:52Z

Bart Van Assche wrote:
>
>> +
>> + switch (event->event) {
>> + case IB_EVENT_PORT_ERR:
>> + list_for_each_entry_safe(host, tmp_host,
>> + &srp_dev->dev_list, list) {
>> + if (event->element.port_num == host->port) {
>> + spin_lock(&host->target_lock);
>>
>
> Can srp_remove_work() be executed concurrently with
> srp_event_handler() ? In that case the above code isn't safe and the
> spin_lock(&host->target_lock) should be moved to just before
> list_for_each_entry_safe(). The current implementation can trigger
> reading deallocated memory.
>
>
>> + list_for_each_entry_safe(target, tmp_target,
>> + &host->target_list, list) {
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(target->scsi_host->host_lock,
>> + flags);
>> + if (!target->qp_in_error &&
>> + target->state == SRP_TARGET_LIVE)
>> + srp_qp_err_add_timer(target,
>> + srp_dev_loss_tmo);
>> + spin_unlock_irqrestore(target->scsi_host->host_lock,
>> + flags);
>> + }
>> + spin_unlock(&host->target_lock);
>> + }
>> + }
>> + break;
>> + case IB_EVENT_PORT_ACTIVE:
>> + case IB_EVENT_LID_CHANGE:
>> + case IB_EVENT_PKEY_CHANGE:
>> + case IB_EVENT_SM_CHANGE:
>> + list_for_each_entry_safe(host, tmp_host, &srp_dev->dev_list,
>> + list) {
>> + if (event->element.port_num == host->port) {
>> + spin_lock(&host->target_lock);
>>
>
> A remark similar to the above: this code isn't safe against concurrent
> calls of srp_remove_one().
>
>
I did not pay attention to the races created by unloading module (same
for patch 3/4). I'll rework patches and address these races.

thanks,
-vu
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html