qla2xx command abort regression

qla2xx command abort regression

[Hannes Reinecke]

Hi Andrew,

there is a regression in the qla2xxx driver, introduced by this commit:

commit 083a469db4ecf3b286a96b5b722c37fc1affe0be
Author: Giridhar Malavali <giridhar.malavali@qlogic.com>
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 <andrew.vasquez@qlogic.com>
    Signed-off-by: Giridhar Malavali <giridhar.malavali@qlogic.com>
    Signed-off-by: James Bottomley <James.Bottomley@suse.de>

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
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: qla2xx command abort regression

[Hannes Reinecke]

Hannes Reinecke wrote:
> Hi Andrew,
> 
> there is a regression in the qla2xxx driver, introduced by this commit:
> 
> commit 083a469db4ecf3b286a96b5b722c37fc1affe0be
> Author: Giridhar Malavali <giridhar.malavali@qlogic.com>
> 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 <andrew.vasquez@qlogic.com>
>     Signed-off-by: Giridhar Malavali <giridhar.malavali@qlogic.com>
>     Signed-off-by: James Bottomley <James.Bottomley@suse.de>
> 
> 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.
> 
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
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html