Skip to contentSkip to conversation

libsas: dereferencing variable before check

PATCH In review Dan Carpenter 3 participants 5 messages May 12, 2010 – May 13, 2010

Subsystems (2)
SCSI SCSI libsas
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

PATCH 1/1 libsas: dereferencing variable before check

The "qc->scsicmd" could be null so I moved the dereference inside the check. This was introduced by 70b25f890: "[SCSI] fix locking around blk_abort_request()"

Signed-off-by Dan Carpenter <[email protected]>
drivers/scsi/libsas/sas_ata.c +2 -1
index 88f7446..98b1545 100644
@@ -395,12 +395,13 @@ int sas_ata_init_host_and_port(struct domain_device *found_dev,
 void sas_ata_task_abort(struct sas_task *task)
 {
 	struct ata_queued_cmd *qc = task->uldd_task;
-	struct request_queue *q = qc->scsicmd->device->request_queue;
+	struct request_queue *q;
 	struct completion *waiting;
 	unsigned long flags;
 
 	/* Bounce SCSI-initiated commands to the SCSI EH */
 	if (qc->scsicmd) {
+		q = qc->scsicmd->device->request_queue;
 		spin_lock_irqsave(q->queue_lock, flags);
 		blk_abort_request(qc->scsicmd->request);
 		spin_unlock_irqrestore(q->queue_lock, flags);
#2 lore raw
Quoted 7 lines · Tejun Heo
Quoted 4 lines · Dan Carpenter

The "qc->scsicmd" could be null so I moved the dereference inside the check. This was introduced by 70b25f890: "[SCSI] fix locking around blk_abort_request()"

Quoted 5 lines
Quoted 1 line
Signed-off-by Dan Carpenter <[email protected]>

Ooh.. right.

Asked-by Tejun Heo <[email protected]>

Well, oops, yes. Two patches to fix one bug is a bit overboard, I think, plus some code rearrangement to fix the locality of the problem and ensure it doesn't happen again might be useful. What about this.

James

---

drivers/scsi/libsas/sas_ata.c +3 -2
index 88f7446..8c496b5 100644
@@ -395,12 +395,13 @@ int sas_ata_init_host_and_port(struct domain_device *found_dev,
 void sas_ata_task_abort(struct sas_task *task)
 {
 	struct ata_queued_cmd *qc = task->uldd_task;
-	struct request_queue *q = qc->scsicmd->device->request_queue;
 	struct completion *waiting;
-	unsigned long flags;
 
 	/* Bounce SCSI-initiated commands to the SCSI EH */
 	if (qc->scsicmd) {
+		struct request_queue *q = qc->scsicmd->device->request_queue;
+		unsigned long flags;
+
 		spin_lock_irqsave(q->queue_lock, flags);
 		blk_abort_request(qc->scsicmd->request);
 		spin_unlock_irqrestore(q->queue_lock, flags);
drivers/scsi/libsas/sas_scsi_host.c +8 -8
index 8228350..53849f2 100644
@@ -1030,8 +1030,6 @@ int __sas_task_abort(struct sas_task *task)
 void sas_task_abort(struct sas_task *task)
 {
 	struct scsi_cmnd *sc = task->uldd_task;
-	struct request_queue *q = sc->device->request_queue;
-	unsigned long flags;
 
 	/* Escape for libsas internal commands */
 	if (!sc) {

@@ -1043,13 +1041,15 @@ void sas_task_abort(struct sas_task *task)
 
 	if (dev_is_sata(task->dev)) {
 		sas_ata_task_abort(task);
-		return;
-	}
+	} else {
+		struct request_queue *q = sc->device->request_queue;
+		unsigned long flags;
 
-	spin_lock_irqsave(q->queue_lock, flags);
-	blk_abort_request(sc->request);
-	spin_unlock_irqrestore(q->queue_lock, flags);
-	scsi_schedule_eh(sc->device->host);
+		spin_lock_irqsave(q->queue_lock, flags);
+		blk_abort_request(sc->request);
+		spin_unlock_irqrestore(q->queue_lock, flags);
+		scsi_schedule_eh(sc->device->host);
+	}
 }
 
 int sas_slave_alloc(struct scsi_device *scsi_dev)
#3 lore raw

Hello,

Quoted 3 lines · James Bottomley
Quoted 3 lines

Ooh.. right.

Asked-by Tejun Heo <[email protected]>

That was my first mail from android and of course I typed Asked instead of Acked. :-)

Quoted 3 lines

Well, oops, yes. Two patches to fix one bug is a bit overboard, I think, plus some code rearrangement to fix the locality of the problem and ensure it doesn't happen again might be useful. What about this.

Yeap, as long as the silly bug is fixed.

Thank you.

Signature · 1 line
tejun
author Acked-by
#4 lore raw
In reply to James Bottomley
Quoted 10 lines · James Bottomley
Quoted 9 lines · Tejun Heo
Quoted 5 lines · Dan Carpenter

The "qc->scsicmd" could be null so I moved the dereference inside the check. This was introduced by 70b25f890: "[SCSI] fix locking around blk_abort_request()"

Signed-off-by Dan Carpenter <[email protected]>
Quoted 8 lines
Quoted 3 lines

Ooh.. right.

Asked-by Tejun Heo <[email protected]>

Well, oops, yes. Two patches to fix one bug is a bit overboard, I think, plus some code rearrangement to fix the locality of the problem and ensure it doesn't happen again might be useful. What about this.

Looks good.

Acked-by Dan Carpenter <[email protected]>

regards,
dan carpenter