Patchwork libsas: fix ata list corruption issue

login
register
mail settings
Submitter James Bottomley
Date March 10, 2011, 11:13 p.m.
Message ID <1299798798.11933.167.camel@mulgrave.site>
Download mbox | patch
Permalink /patch/86343/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

James Bottomley - March 10, 2011, 11:13 p.m.
I think this stems from a misunderstanding of how the ata error handler
works.  ata_scsi_cmd_error_handler() gets called with a passed in list
of commands to handle.  However, that list may still not be empty when
it exits.  The command ata_scsi_port_error_handler() must be called
(which takes no list) before the list will be completely emptied.  This
bites the sas error handler because the two are called from different
functions and the original list has gone out of scope before
ata_scsi_port_error_handler() is called. leading to some commands
dangling on bare stack, which is a potential memory corruption issue.
Fix this by manually deleting all outstanding commands from the on-stack
list before it goes out of scope.

James

---



--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Garzik - March 11, 2011, 1:28 a.m.
On 03/10/2011 06:13 PM, James Bottomley wrote:
> I think this stems from a misunderstanding of how the ata error handler
> works.  ata_scsi_cmd_error_handler() gets called with a passed in list
> of commands to handle.  However, that list may still not be empty when
> it exits.  The command ata_scsi_port_error_handler() must be called
> (which takes no list) before the list will be completely emptied.  This
> bites the sas error handler because the two are called from different
> functions and the original list has gone out of scope before
> ata_scsi_port_error_handler() is called. leading to some commands
> dangling on bare stack, which is a potential memory corruption issue.
> Fix this by manually deleting all outstanding commands from the on-stack
> list before it goes out of scope.

Good catch...


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
James Bottomley - March 11, 2011, 5:19 p.m.
On Thu, 2011-03-10 at 20:28 -0500, Jeff Garzik wrote:
> On 03/10/2011 06:13 PM, James Bottomley wrote:
> > I think this stems from a misunderstanding of how the ata error handler
> > works.  ata_scsi_cmd_error_handler() gets called with a passed in list
> > of commands to handle.  However, that list may still not be empty when
> > it exits.  The command ata_scsi_port_error_handler() must be called
> > (which takes no list) before the list will be completely emptied.  This
> > bites the sas error handler because the two are called from different
> > functions and the original list has gone out of scope before
> > ata_scsi_port_error_handler() is called. leading to some commands
> > dangling on bare stack, which is a potential memory corruption issue.
> > Fix this by manually deleting all outstanding commands from the on-stack
> > list before it goes out of scope.
> 
> Good catch...

I cannot tell a lie: it was the list debugger code that told me
something was wrong ... I just looked at it to see what the problem was.

James


--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 16c5094..3356bf3 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -802,6 +802,19 @@  int sas_ata_eh(struct Scsi_Host *shost, struct list_head *work_q,
 		if (!list_empty(&sata_q)) {
 			ata_port_printk(ap, KERN_DEBUG, "sas eh calling libata cmd error handler\n");
 			ata_scsi_cmd_error_handler(shost, ap, &sata_q);
+			/*
+			 * ata's error handler may leave the cmd on the list
+			 * so make sure they don't remain on a stack list
+			 * about to go out of scope.
+			 *
+			 * This looks strange, since the commands are
+			 * now part of no list, but the next error
+			 * action will be ata_port_error_handler()
+			 * which takes no list and sweeps them up
+			 * anyway from the ata tag array.
+			 */
+			while (!list_empty(&sata_q))
+				list_del_init(sata_q.next);
 		}
 	} while (ap);