qla2xx command abort regression
Hi Andrew,
there is a regression in the qla2xxx driver, introduced by this commit:
commit 083a469db4ecf3b286a96b5b722c37fc1affe0be
Author: Giridhar Malavali <[email protected]> Date: Fri May 28 15:08:18 2010 -0700
[SCSI] qla2xxx: Correct use-after-free oops seen during EH-abort.
Hold a reference to the srb (sp) while aborting an I/O -- as the I/O can/will complete from within the interrupt-context.
With this patch a reference counting is introduced for srb's. However, there is this code in qla2xxx_eh_abort():
spin_unlock_irqrestore(&ha->hardware_lock, flags);
/* Wait for the command to be returned. */
if (wait) {
if (qla2x00_eh_wait_on_command(cmd) != QLA_SUCCESS) {
qla_printk(KERN_ERR, ha,
"scsi(%ld:%d:%d): Abort handler timed out -- %lx "
"%x.\n", vha->host_no, id, lun, serial, ret);
ret = FAILED;
}
}
if (got_ref)
qla2x00_sp_compl(ha, sp);where qla2x00_eh_wait_on_command() is waiting for a command to be completed by the midlayer. Which will never happen, as the refcount is held during that time and only released on the last lines. Hence any command abort will be timed out and the error will be escalated further.
I have fixed it by simply moving the last two lines above the 'if (wait)' condition. however I fail to see the race condition mentioned, and hence the validity of the reference counting in the first place.
So it might be that I'm missing something subtle here, so I would ask you to have a look here.
Cheers,
Hannes
Signature · 8 lines
Dr. Hannes Reinecke zSeries & Storage [email protected] +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Markus Rex, HRB 16746 (AG Nürnberg) -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 42 lines · Hannes Reinecke
Hi Andrew,
there is a regression in the qla2xxx driver, introduced by this commit:
commit
083a469db4ecf3b286a96b5b722c37fc1affe0beAuthor: Giridhar Malavali <[email protected]> Date: Fri May 28 15:08:18 2010 -0700[SCSI] qla2xxx: Correct use-after-free oops seen during EH-abort.
Hold a reference to the srb (sp) while aborting an I/O -- as the I/O can/will complete from within the interrupt-context.
Signed-off-by Andrew Vasquez <[email protected]>Signed-off-by Giridhar Malavali <[email protected]>Signed-off-by James Bottomley <[email protected]>With this patch a reference counting is introduced for srb's. However, there is this code in
qla2xxx_eh_abort():spin_unlock_irqrestore(&ha->hardware_lock, flags); /* Wait for the command to be returned. */ if (wait) { if (qla2x00_eh_wait_on_command(cmd) != QLA_SUCCESS) { qla_printk(KERN_ERR, ha, "scsi(%ld:%d:%d): Abort handler timed out -- %lx " "%x.\n", vha->host_no, id, lun, serial, ret); ret = FAILED; } } if (got_ref) qla2x00_sp_compl(ha, sp);where
qla2x00_eh_wait_on_command()is waiting for a command to be completed by the midlayer. Which will never happen, as the refcount is held during that time and only released on the last lines. Hence any command abort will be timed out and the error will be escalated further.
Quoted 7 lines
I have fixed it by simply moving the last two lines above the 'if (wait)' condition. however I fail to see the race condition mentioned, and hence the validity of the reference counting in the first place.
So it might be that I'm missing something subtle here, so I would ask you to have a look here.
Actually, I found the race condition; it's here
(mentioned commit removed):
DEBUG2(printk("%s(%ld): aborting sp %p from RISC."
" pid=%ld.\n", __func__, vha->host_no, sp, serial));
spin_unlock_irqrestore(&ha->hardware_lock, flags);
if (ha->isp_ops->abort_command(sp)) {
DEBUG2(printk("%s(%ld): abort_command "
"mbx failed.\n", __func__, vha->host_no));
ret = FAILED;So the command referenced by 'sp' might've been completed after the unlock, in which case ->abort_command() will be accessing an invalid 'sp' point. Ok. But then my fix is indeed valid.
I'll be sending a proper patch.
Cheers,
Hannes
Signature · 8 lines
Dr. Hannes Reinecke zSeries & Storage [email protected] +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Markus Rex, HRB 16746 (AG Nürnberg) -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html