libata: a fix and a cleanup

libata: a fix and a cleanup

[Christoph Hellwig]

The first patch fixes order of command completion vs tag freeing, and the
second is a trivial cleanup of something I noticed while digging into that
area.

[PATCH 1/2] libata: only call ->done once all per-tag ressources are released

[Christoph Hellwig]

Without this the SCSI midlayer can race and queue another command
before the tag data has been released, which could lead to incorrect
manipulation of the ata command state.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/ata/libata-scsi.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 0d7f0da..8445620 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1757,6 +1757,15 @@ nothing_to_do:
 	return 1;
 }
 
+static void ata_qc_done(struct ata_queued_cmd *qc)
+{
+	struct scsi_cmnd *cmd = qc->scsicmd;
+	void (*done)(struct scsi_cmnd *) = qc->scsidone;
+
+	ata_qc_free(qc);
+	done(cmd);
+}
+
 static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
@@ -1793,9 +1802,7 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
 	if (need_sense && !ap->ops->error_handler)
 		ata_dump_status(ap->print_id, &qc->result_tf);
 
-	qc->scsidone(cmd);
-
-	ata_qc_free(qc);
+	ata_qc_done(qc);
 }
 
 /**
@@ -2594,8 +2601,7 @@ static void atapi_sense_complete(struct ata_queued_cmd *qc)
 		ata_gen_passthru_sense(qc);
 	}
 
-	qc->scsidone(qc->scsicmd);
-	ata_qc_free(qc);
+	ata_qc_done(qc);
 }
 
 /* is it pointless to prefer PIO for "safety reasons"? */
@@ -2690,8 +2696,7 @@ static void atapi_qc_complete(struct ata_queued_cmd *qc)
 			qc->dev->sdev->locked = 0;
 
 		qc->scsicmd->result = SAM_STAT_CHECK_CONDITION;
-		qc->scsidone(cmd);
-		ata_qc_free(qc);
+		ata_qc_done(qc);
 		return;
 	}
 
@@ -2735,8 +2740,7 @@ static void atapi_qc_complete(struct ata_queued_cmd *qc)
 		cmd->result = SAM_STAT_GOOD;
 	}
 
-	qc->scsidone(cmd);
-	ata_qc_free(qc);
+	ata_qc_done(qc);
 }
 /**
  *	atapi_xlat - Initialize PACKET taskfile
-- 
1.9.1

Re: [PATCH 1/2] libata: only call ->done once all per-tag ressources are released

[Tejun Heo]

Hello, Christoph.

On Sat, Oct 03, 2015 at 07:21:10PM +0200, Christoph Hellwig wrote:
> Without this the SCSI midlayer can race and queue another command
> before the tag data has been released, which could lead to incorrect
> manipulation of the ata command state.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Hmmm... has this actually been observed?  All these are run under ata
port lock and so is the command issue path, so even if the tag gets
reissued, it will have to wait till the completion path is done.

Thanks.

-- 
tejun

Re: [PATCH 1/2] libata: only call ->done once all per-tag ressources are released

[Christoph Hellwig]

On Sun, Oct 04, 2015 at 01:34:29PM -0400, Tejun Heo wrote:
> Hmmm... has this actually been observed?  All these are run under ata
> port lock and so is the command issue path, so even if the tag gets
> reissued, it will have to wait till the completion path is done.

No - I stumbled over this while trying to debug
https://lkml.org/lkml/2015/6/25/620 (which could use some libata experience,
btw), but it didn't help.  I still would like to see it fixes as similar
patterns in SCSI drivers have caused crashes.

Re: [PATCH 1/2] libata: only call ->done once all per-tag ressources are released

[Tejun Heo]

Hello, Christoph.

On Mon, Oct 05, 2015 at 08:01:17AM +0200, Christoph Hellwig wrote:
> On Sun, Oct 04, 2015 at 01:34:29PM -0400, Tejun Heo wrote:
> > Hmmm... has this actually been observed?  All these are run under ata
> > port lock and so is the command issue path, so even if the tag gets
> > reissued, it will have to wait till the completion path is done.
> 
> No - I stumbled over this while trying to debug
> https://lkml.org/lkml/2015/6/25/620 (which could use some libata experience,
> btw), but it didn't help.  I still would like to see it fixes as similar
> patterns in SCSI drivers have caused crashes.

So, given the locking, the bug doesn't exist.  I don't think switching
to a safer pattern is a bad thing but the patch needs updated
description and comment explanining that it isn't a bug fix and the
specific ordering isn't strictly necessary.

Thanks.

-- 
tejun

[PATCH 1/2 v2] libata: only call ->done once all per-tag ressources are released

[Christoph Hellwig]

When calling ->done before releasing resources we could run into a
race where the SCSI midlayer sends another command and races with
the resources beeing manipulated.  For libata this can't currently
happen as synchronization happens at a higher level, but I'd still
like to fix it to future proof libata and to avoid copy & paste
into SCSI drivers where this pattern has led to reproducible crashes.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/ata/libata-scsi.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 0d7f0da..8445620 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1757,6 +1757,15 @@ nothing_to_do:
 	return 1;
 }
 
+static void ata_qc_done(struct ata_queued_cmd *qc)
+{
+	struct scsi_cmnd *cmd = qc->scsicmd;
+	void (*done)(struct scsi_cmnd *) = qc->scsidone;
+
+	ata_qc_free(qc);
+	done(cmd);
+}
+
 static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
@@ -1793,9 +1802,7 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
 	if (need_sense && !ap->ops->error_handler)
 		ata_dump_status(ap->print_id, &qc->result_tf);
 
-	qc->scsidone(cmd);
-
-	ata_qc_free(qc);
+	ata_qc_done(qc);
 }
 
 /**
@@ -2594,8 +2601,7 @@ static void atapi_sense_complete(struct ata_queued_cmd *qc)
 		ata_gen_passthru_sense(qc);
 	}
 
-	qc->scsidone(qc->scsicmd);
-	ata_qc_free(qc);
+	ata_qc_done(qc);
 }
 
 /* is it pointless to prefer PIO for "safety reasons"? */
@@ -2690,8 +2696,7 @@ static void atapi_qc_complete(struct ata_queued_cmd *qc)
 			qc->dev->sdev->locked = 0;
 
 		qc->scsicmd->result = SAM_STAT_CHECK_CONDITION;
-		qc->scsidone(cmd);
-		ata_qc_free(qc);
+		ata_qc_done(qc);
 		return;
 	}
 
@@ -2735,8 +2740,7 @@ static void atapi_qc_complete(struct ata_queued_cmd *qc)
 		cmd->result = SAM_STAT_GOOD;
 	}
 
-	qc->scsidone(cmd);
-	ata_qc_free(qc);
+	ata_qc_done(qc);
 }
 /**
  *	atapi_xlat - Initialize PACKET taskfile
-- 
1.9.1

Re: [PATCH 1/2 v2] libata: only call ->done once all per-tag ressources are released

[Tejun Heo]

On Thu, Oct 08, 2015 at 10:25:41AM +0200, Christoph Hellwig wrote:
> When calling ->done before releasing resources we could run into a
> race where the SCSI midlayer sends another command and races with
> the resources beeing manipulated.  For libata this can't currently
> happen as synchronization happens at a higher level, but I'd still
> like to fix it to future proof libata and to avoid copy & paste
> into SCSI drivers where this pattern has led to reproducible crashes.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Applied to libata/for-4.4.

Thanks.

-- 
tejun

[PATCH 2/2] libata: cleanup ata_scsi_qc_complete

[Christoph Hellwig]

Remove an incorrect comment and untangle an if statement in
ata_scsi_qc_complete.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/ata/libata-scsi.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 8445620..7feceec 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1783,21 +1783,12 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
 	 * asc,ascq = ATA PASS-THROUGH INFORMATION AVAILABLE
 	 */
 	if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) &&
-	    ((cdb[2] & 0x20) || need_sense)) {
+	    ((cdb[2] & 0x20) || need_sense))
 		ata_gen_passthru_sense(qc);
-	} else {
-		if (!need_sense) {
-			cmd->result = SAM_STAT_GOOD;
-		} else {
-			/* TODO: decide which descriptor format to use
-			 * for 48b LBA devices and call that here
-			 * instead of the fixed desc, which is only
-			 * good for smaller LBA (and maybe CHS?)
-			 * devices.
-			 */
-			ata_gen_ata_sense(qc);
-		}
-	}
+	else if (need_sense)
+		ata_gen_ata_sense(qc);
+	else
+		cmd->result = SAM_STAT_GOOD;
 
 	if (need_sense && !ap->ops->error_handler)
 		ata_dump_status(ap->print_id, &qc->result_tf);
-- 
1.9.1

Re: [PATCH 2/2] libata: cleanup ata_scsi_qc_complete

[Tejun Heo]

On Sat, Oct 03, 2015 at 07:21:11PM +0200, Christoph Hellwig wrote:
> Remove an incorrect comment and untangle an if statement in
> ata_scsi_qc_complete.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Applied to libata/for-4.4.

Thanks.

-- 
tejun