[patch] libsas: dereferencing variable before check

[patch] libsas: dereferencing variable before check

[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 <error27@gmail.com>

diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 88f7446..98b1545 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -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);

Re: [patch] libsas: dereferencing variable before check

[Tejun Heo]

"Dan Carpenter" <error27@gmail.com> wrote:

>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 <error27@gmail.com>

Ooh.. right.

Asked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [patch] libsas: dereferencing variable before check

[James Bottomley]

On Thu, 2010-05-13 at 01:08 +0200, Tejun Heo wrote:
> 
> "Dan Carpenter" <error27@gmail.com> wrote:
> 
> >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 <error27@gmail.com>
> 
> Ooh.. right.
> 
> Asked-by: Tejun Heo <tj@kernel.org>

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

---

diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 88f7446..8c496b5 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -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);
diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index 8228350..53849f2 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -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)

Re: [patch] libsas: dereferencing variable before check

[Tejun Heo]

Hello,

On 05/13/2010 04:31 PM, James Bottomley wrote:
>> Ooh.. right.
>>
>> Asked-by: Tejun Heo <tj@kernel.org>

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

> 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.

-- 
tejun

Re: [patch] libsas: dereferencing variable before check

[Dan Carpenter]

On Thu, May 13, 2010 at 09:31:54AM -0500, James Bottomley wrote:
> On Thu, 2010-05-13 at 01:08 +0200, Tejun Heo wrote:
> > 
> > "Dan Carpenter" <error27@gmail.com> wrote:
> > 
> > >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 <error27@gmail.com>
> > 
> > Ooh.. right.
> > 
> > Asked-by: Tejun Heo <tj@kernel.org>
> 
> 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 <error27@gmail.com>

regards,
dan carpenter