Skip to contentSkip to conversation

Add NULL checking of return value from scsi_cmd_to_driver()

PATCH In review Li Zhong 3 participants 16 messages Sep 24, 2012 – Oct 2, 2012 SCSI

lore
Share & tools
lore

Li Zhong

author
#0 lore raw

PATCH 1/1 Add NULL checking of return value from scsi_cmd_to_driver()

Just noticed that after commit 919f797, it is possible that scsi_cmd_to_driver() returns NULL. This patch adds the NULL checking for drv returned from the above function.

Maybe it is not possible at run time, but from the code itself, we'd better have this check?

This patch actually is a delta of my previous patch
[https://lkml.org/lkml/2012/4/11/814] and commit 919f797

---

drivers/scsi/scsi.c +1 -1
index 2936b44..b9dd7f0 100644
@@ -832,7 +832,7 @@ void scsi_finish_command(struct scsi_cmnd *cmd)
         if (cmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
 		int old_good_bytes = good_bytes;
 		drv = scsi_cmd_to_driver(cmd);
-		if (drv->done)
+		if (drv && drv->done)
 			good_bytes = drv->done(cmd);
 		/*
 		 * USB may not give sense identifying bad sector and
maintainer
#1 lore raw
Quoted 6 lines · Li Zhong

Just noticed that after commit 919f797, it is possible that scsi_cmd_to_driver() returns NULL. This patch adds the NULL checking for drv returned from the above function.

Maybe it is not possible at run time, but from the code itself, we'd better have this check?

There's not much point having a check that never trips, unless it's an assert, in which case a NULL deref does that. All it does is add pointless instructions to the critical path. only REQ_TYPE_BLOCK_PC commands can be submitted without a driver, so the check above would seem to preclude that.

James

author
#2 lore raw
Quoted 5 lines · James Bottomley
Quoted 4 lines · Li Zhong

Just noticed that after commit 919f797, it is possible that scsi_cmd_to_driver() returns NULL. This patch adds the NULL checking for drv returned from the above function.

Quoted 8 lines
Quoted 2 lines

Maybe it is not possible at run time, but from the code itself, we'd better have this check?

There's not much point having a check that never trips, unless it's an assert, in which case a NULL deref does that. All it does is add pointless instructions to the critical path. only REQ_TYPE_BLOCK_PC commands can be submitted without a driver, so the check above would seem to preclude that.

Hi James,

Thank you, it sounds reasonable to me. Let's drop it.

Thanks, Zhong

Quoted 3 lines

James

maintainer
#3 lore raw
Quoted 15 lines · Li Zhong
Quoted 5 lines · James Bottomley
Quoted 4 lines · Li Zhong

Just noticed that after commit 919f797, it is possible that scsi_cmd_to_driver() returns NULL. This patch adds the NULL checking for drv returned from the above function.

Quoted 8 lines
Quoted 2 lines

Maybe it is not possible at run time, but from the code itself, we'd better have this check?

There's not much point having a check that never trips, unless it's an assert, in which case a NULL deref does that. All it does is add pointless instructions to the critical path. only REQ_TYPE_BLOCK_PC commands can be submitted without a driver, so the check above would seem to preclude that.

Quoted 3 lines

Hi James,

Thank you, it sounds reasonable to me. Let's drop it.

Well, there is another thing you might do: The path length of scsi_cmd_to_driver() increased a lot thanks to 18a4d0a22ed6 it might be worth getting it back to what it was (this looks to be doable with the same != REQ_TYPE_BLOCK_PC test in the error handler. Plus, I think it fixes a bug where you get different behaviours from REQ_TYPE_BLOCK_PC commands when a driver is and isn't attached (I've cc'd Martin to see what he thinks).

James

author
#4 lore raw
Quoted 20 lines · James Bottomley
Quoted 15 lines · Li Zhong
Quoted 5 lines · James Bottomley
Quoted 4 lines · Li Zhong

Just noticed that after commit 919f797, it is possible that scsi_cmd_to_driver() returns NULL. This patch adds the NULL checking for drv returned from the above function.

Quoted 8 lines
Quoted 2 lines

Maybe it is not possible at run time, but from the code itself, we'd better have this check?

There's not much point having a check that never trips, unless it's an assert, in which case a NULL deref does that. All it does is add pointless instructions to the critical path. only REQ_TYPE_BLOCK_PC commands can be submitted without a driver, so the check above would seem to preclude that.

Quoted 3 lines

Hi James,

Thank you, it sounds reasonable to me. Let's drop it.

Quoted 7 lines

Well, there is another thing you might do: The path length of scsi_cmd_to_driver() increased a lot thanks to 18a4d0a22ed6 it might be worth getting it back to what it was (this looks to be doable with the same != REQ_TYPE_BLOCK_PC test in the error handler. Plus, I think it fixes a bug where you get different behaviours from REQ_TYPE_BLOCK_PC commands when a driver is and isn't attached (I've cc'd Martin to see what he thinks).

Hi James,

Do you mean something like this:

=======

drivers/scsi/scsi_error.c +5 -3
index de2337f..c1b05a8 100644
@@ -789,7 +789,6 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 			     int cmnd_size, int timeout, unsigned sense_bytes)
 {
 	struct scsi_device *sdev = scmd->device;
-	struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
 	struct Scsi_Host *shost = sdev->host;
 	DECLARE_COMPLETION_ONSTACK(done);
 	unsigned long timeleft;
@@ -845,8 +844,11 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 
 	scsi_eh_restore_cmnd(scmd, &ses);
 
-	if (sdrv && sdrv->eh_action)
-		rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
+	if (scmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
+		struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
+		if (sdrv->eh_action)
+			rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
+	}
 
 	return rtn;
 }
include/scsi/scsi_cmnd.h +1 -10
index ac06cc5..377df4a 100644
@@ -134,16 +134,7 @@ struct scsi_cmnd {
 
 static inline struct scsi_driver *scsi_cmd_to_driver(struct scsi_cmnd *cmd)
 {
-	struct scsi_driver **sdp;
-
-	if (!cmd->request->rq_disk)
-		return NULL;
-
-	sdp = (struct scsi_driver **)cmd->request->rq_disk->private_data;
-	if (!sdp)
-		return NULL;
-
-	return *sdp;
+	return *(struct scsi_driver **)cmd->request->rq_disk->private_data;
 }
 
 extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t);
maintainer
#5 lore raw
Quoted 29 lines · Li Zhong
Quoted 20 lines · James Bottomley
Quoted 15 lines · Li Zhong
Quoted 5 lines · James Bottomley
Quoted 4 lines · Li Zhong

Just noticed that after commit 919f797, it is possible that scsi_cmd_to_driver() returns NULL. This patch adds the NULL checking for drv returned from the above function.

Quoted 8 lines
Quoted 2 lines

Maybe it is not possible at run time, but from the code itself, we'd better have this check?

There's not much point having a check that never trips, unless it's an assert, in which case a NULL deref does that. All it does is add pointless instructions to the critical path. only REQ_TYPE_BLOCK_PC commands can be submitted without a driver, so the check above would seem to preclude that.

Quoted 3 lines

Hi James,

Thank you, it sounds reasonable to me. Let's drop it.

Quoted 7 lines

Well, there is another thing you might do: The path length of scsi_cmd_to_driver() increased a lot thanks to 18a4d0a22ed6 it might be worth getting it back to what it was (this looks to be doable with the same != REQ_TYPE_BLOCK_PC test in the error handler. Plus, I think it fixes a bug where you get different behaviours from REQ_TYPE_BLOCK_PC commands when a driver is and isn't attached (I've cc'd Martin to see what he thinks).

Quoted 3 lines

Hi James,

Do you mean something like this:

Pretty much, yes.

James

Quoted 56 lines

=======

drivers/scsi/scsi_error.c +5 -3
index de2337f..c1b05a8 100644
@@ -789,7 +789,6 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 			     int cmnd_size, int timeout, unsigned sense_bytes)
 {
 	struct scsi_device *sdev = scmd->device;
-	struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
 	struct Scsi_Host *shost = sdev->host;
 	DECLARE_COMPLETION_ONSTACK(done);
 	unsigned long timeleft;
@@ -845,8 +844,11 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 
 	scsi_eh_restore_cmnd(scmd, &ses);
 
-	if (sdrv && sdrv->eh_action)
-		rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
+	if (scmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
+		struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
+		if (sdrv->eh_action)
+			rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
+	}
 
 	return rtn;
 }
include/scsi/scsi_cmnd.h +1 -11
index ac06cc5..377df4a 100644
@@ -134,16 +134,7 @@ struct scsi_cmnd {
 
 static inline struct scsi_driver *scsi_cmd_to_driver(struct scsi_cmnd *cmd)
 {
-	struct scsi_driver **sdp;
-
-	if (!cmd->request->rq_disk)
-		return NULL;
-
-	sdp = (struct scsi_driver **)cmd->request->rq_disk->private_data;
-	if (!sdp)
-		return NULL;
-
-	return *sdp;
+	return *(struct scsi_driver **)cmd->request->rq_disk->private_data;
 }
 
 extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t);
 
 
--

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

#6 lore raw
In reply to James Bottomley
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"James" == James Bottomley <[email protected]> writes:

James> Plus, I think it fixes a bug where you get different behaviours James> from REQ_TYPE_BLOCK_PC commands when a driver is and isn't James> attached (I've cc'd Martin to see what he thinks).

I'm fine with having the eh action be triggered for FS requests only, if that's what you're asking?

Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering
maintainer
#7 lore raw
Quoted 8 lines · Martin K. Petersen
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"James" == James Bottomley <[email protected]> writes:

James> Plus, I think it fixes a bug where you get different behaviours James> from REQ_TYPE_BLOCK_PC commands when a driver is and isn't James> attached (I've cc'd Martin to see what he thinks).

I'm fine with having the eh action be triggered for FS requests only, if that's what you're asking?

Sort of ... I was thinking do it for all non REQ_TYPE_BLOCK_PC commands (which includes flush and other strange types), but if you think it should only be for REQ_TYPE_FS, that's fine too.

James

author
#8 lore raw

PATCH 1/1 Short the path length of scsi_cmd_to_driver()

Hi James, Martin,

Here is the updated version, please help to review.

Thanks, Zhong

========
As suggested by James: this patch tries to short the path length of scsi_cmd_to_driver(). As only REQ_TYPE_BLOCK_PC commands can be submitted without a driver, so we could avoid the related NULL checking, as long as we make sure we don't use it for REQ_TYPE_BLOCK_PC type commands. Plus, this fixes a bug where you get different behaviors from REQ_TYPE_BLOCK_PC commands when a driver is and isn't attached.

And Martin pointed out that we could have eh action be triggered for REQ_TYPE_FS type only.

---
 drivers/scsi/scsi_error.c |    8 +++++---
 include/scsi/scsi_cmnd.h  |   12 ++----------
 2 files changed, 7 insertions(+), 13 deletions(-)
drivers/scsi/scsi_error.c +5 -3
index de2337f..4001559 100644
@@ -789,7 +789,6 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 			     int cmnd_size, int timeout, unsigned sense_bytes)
 {
 	struct scsi_device *sdev = scmd->device;
-	struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
 	struct Scsi_Host *shost = sdev->host;
 	DECLARE_COMPLETION_ONSTACK(done);
 	unsigned long timeleft;
@@ -845,8 +844,11 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 
 	scsi_eh_restore_cmnd(scmd, &ses);
 
-	if (sdrv && sdrv->eh_action)
-		rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
+	if (scmd->request->cmd_type == REQ_TYPE_FS) {
+		struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
+		if (sdrv->eh_action)
+			rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
+	}
 
 	return rtn;
 }
include/scsi/scsi_cmnd.h +2 -10
index ac06cc5..de5f5d8 100644
@@ -132,18 +132,10 @@ struct scsi_cmnd {
 	unsigned char tag;	/* SCSI-II queued command tag */
 };
 
+/* make sure not to use it with REQ_TYPE_BLOCK_PC commands */
 static inline struct scsi_driver *scsi_cmd_to_driver(struct scsi_cmnd *cmd)
 {
-	struct scsi_driver **sdp;
-
-	if (!cmd->request->rq_disk)
-		return NULL;
-
-	sdp = (struct scsi_driver **)cmd->request->rq_disk->private_data;
-	if (!sdp)
-		return NULL;
-
-	return *sdp;
+	return *(struct scsi_driver **)cmd->request->rq_disk->private_data;
 }
 
 extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t);
Signature · 1 line
1.7.7.6
#9 lore raw

Re: [PATCH scsi] Short the path length of scsi_cmd_to_driver()

Quoted 6 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Li" == Li Zhong <[email protected]> writes:

drivers/scsi/scsi_error.c +0 -0
@@ -845,8 +844,11 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 
 	scsi_eh_restore_cmnd(scmd, &ses);
 

drivers/scsi/scsi_error.c static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd, · line 854

Quoted 10 lines
drivers/scsi/scsi_error.c +5 -2
-	if (sdrv && sdrv->eh_action)
-		rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
+	if (scmd->request->cmd_type == REQ_TYPE_FS) {
+		struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
+		if (sdrv->eh_action)
+			rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
+	}
 
 	return rtn;
 }

My only concern is whether our device lifetime rules guarantee that the ULD is always attached when we service an error handling command?

Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering
author
#10 lore raw

Re: [PATCH scsi] Short the path length of scsi_cmd_to_driver()

Quoted 14 lines · Martin K. Petersen
Quoted 14 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Li" == Li Zhong <[email protected]> writes:

drivers/scsi/scsi_error.c +5 -2
@@ -845,8 +844,11 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 
 	scsi_eh_restore_cmnd(scmd, &ses);
 
-	if (sdrv && sdrv->eh_action)
-		rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
+	if (scmd->request->cmd_type == REQ_TYPE_FS) {
+		struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
+		if (sdrv->eh_action)
+			rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
+	}
 
Quoted 5 lines

drivers/scsi/scsi_error.c static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd, · line 854

Quoted 2 lines
drivers/scsi/scsi_error.c +0 -0
 	return rtn;
 }

My only concern is whether our device lifetime rules guarantee that the ULD is always attached when we service an error handling command?

Thank you, Martin, for the review.

I don't know much about scsi, it might take me some more time to have an answer to the above question.

For now, if I understand correctly, maybe we could only do the not-consistent behaviours bug fix? Or could we provide two versions of scsi_cmd_to_driver(), one with NULL checking for scsi_send_eh_cmnd(), one without the checking for scsi_finish_command()?

Thanks, Zhong

maintainer
#11 lore raw

Re: [PATCH scsi] Short the path length of scsi_cmd_to_driver()

In reply to Martin K. Petersen
Quoted 14 lines · Martin K. Petersen
Quoted 14 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Li" == Li Zhong <[email protected]> writes:

drivers/scsi/scsi_error.c +5 -2
@@ -845,8 +844,11 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 
 	scsi_eh_restore_cmnd(scmd, &ses);
 
-	if (sdrv && sdrv->eh_action)
-		rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
+	if (scmd->request->cmd_type == REQ_TYPE_FS) {
+		struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
+		if (sdrv->eh_action)
+			rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
+	}
 
Quoted 5 lines

drivers/scsi/scsi_error.c static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd, · line 854

Quoted 2 lines
drivers/scsi/scsi_error.c +0 -0
 	return rtn;
 }

My only concern is whether our device lifetime rules guarantee that the ULD is always attached when we service an error handling command?

Yes, they are. We can only get REQ_TYPE_FS through a filesystem, which must be mounted on a block device, which is provided by the ULD. You can't unmount with outstanding I/O (which people complain about when it goes into error handling, I'll admit), so the ULD has to stay bound.

James

#12 lore raw
In reply to James Bottomley
Quoted 4 lines
Quoted 4 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line

"James" == James Bottomley <[email protected]> writes:

I'm fine with having the eh action be triggered for FS requests only, if that's what you're asking?

James> Sort of ... I was thinking do it for all non REQ_TYPE_BLOCK_PC James> commands (which includes flush and other strange types), but if James> you think it should only be for REQ_TYPE_FS, that's fine too.

I'm ok either way.

Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering
maintainer
#13 lore raw
Quoted 10 lines · Martin K. Petersen
Quoted 4 lines
Quoted 4 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line

"James" == James Bottomley <[email protected]> writes:

I'm fine with having the eh action be triggered for FS requests only, if that's what you're asking?

James> Sort of ... I was thinking do it for all non REQ_TYPE_BLOCK_PC James> commands (which includes flush and other strange types), but if James> you think it should only be for REQ_TYPE_FS, that's fine too.

I'm ok either way.

OK, let's mirror the code in scsi.c then, so != REQ_TYPE_BLOCK_PC.

James

author
#14 lore raw

PATCH v2 1/1 Short the path length of scsi_cmd_to_driver()

As suggested by James: this patch tries to short the path length of scsi_cmd_to_driver(). As only REQ_TYPE_BLOCK_PC commands can be submitted without a driver, so we could avoid the related NULL checking, as long as we make sure we don't use it for REQ_TYPE_BLOCK_PC type commands. Plus, this fixes a bug where you get different behaviors from REQ_TYPE_BLOCK_PC commands when a driver is and isn't attached.

v2: mirror code != REQ_TYPE_BLOCK_PC in scsi.c, rather than == REQ_TYPE_FS

---
 drivers/scsi/scsi_error.c |    8 +++++---
 include/scsi/scsi_cmnd.h  |   12 ++----------
 2 files changed, 7 insertions(+), 13 deletions(-)
drivers/scsi/scsi_error.c +5 -3
index de2337f..c1b05a8 100644
@@ -789,7 +789,6 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 			     int cmnd_size, int timeout, unsigned sense_bytes)
 {
 	struct scsi_device *sdev = scmd->device;
-	struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
 	struct Scsi_Host *shost = sdev->host;
 	DECLARE_COMPLETION_ONSTACK(done);
 	unsigned long timeleft;
@@ -845,8 +844,11 @@ static int scsi_send_eh_cmnd(struct scsi_cmnd *scmd, unsigned char *cmnd,
 
 	scsi_eh_restore_cmnd(scmd, &ses);
 
-	if (sdrv && sdrv->eh_action)
-		rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
+	if (scmd->request->cmd_type != REQ_TYPE_BLOCK_PC) {
+		struct scsi_driver *sdrv = scsi_cmd_to_driver(scmd);
+		if (sdrv->eh_action)
+			rtn = sdrv->eh_action(scmd, cmnd, cmnd_size, rtn);
+	}
 
 	return rtn;
 }
include/scsi/scsi_cmnd.h +2 -10
index ac06cc5..de5f5d8 100644
@@ -132,18 +132,10 @@ struct scsi_cmnd {
 	unsigned char tag;	/* SCSI-II queued command tag */
 };
 
+/* make sure not to use it with REQ_TYPE_BLOCK_PC commands */
 static inline struct scsi_driver *scsi_cmd_to_driver(struct scsi_cmnd *cmd)
 {
-	struct scsi_driver **sdp;
-
-	if (!cmd->request->rq_disk)
-		return NULL;
-
-	sdp = (struct scsi_driver **)cmd->request->rq_disk->private_data;
-	if (!sdp)
-		return NULL;
-
-	return *sdp;
+	return *(struct scsi_driver **)cmd->request->rq_disk->private_data;
 }
 
 extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t);
Signature · 1 line
1.7.7.6
Acked-by on v2 1/1
#15 lore raw

Re: [PATCH v2 scsi] Short the path length of scsi_cmd_to_driver()

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

"Li" == Li Zhong <[email protected]> writes:

Li> As suggested by James: this patch tries to short the path length of Li> scsi_cmd_to_driver(). As only REQ_TYPE_BLOCK_PC commands can be Li> submitted without a driver, so we could avoid the related NULL Li> checking, as long as we make sure we don't use it for Li> REQ_TYPE_BLOCK_PC type commands. Plus, this fixes a bug where you Li> get different behaviors from REQ_TYPE_BLOCK_PC commands when a Li> driver is and isn't attached.

Signature · 1 line
Martin K. Petersen	Oracle Linux Engineering