* [PATCH v5 01/13] scsi: core: need to check the result of scsi_execute in scsi_report_opcode
2019-10-18 8:24 [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr zhengbin
@ 2019-10-18 8:24 ` zhengbin
2019-10-18 8:24 ` [PATCH v5 02/13] scsi: core: need to check the result of scsi_execute in scsi_test_unit_ready zhengbin
` (13 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: zhengbin @ 2019-10-18 8:24 UTC (permalink / raw)
To: bvanassche, jejb, martin.petersen, linux-scsi
Cc: yi.zhang, yanaijie, zhengbin13
Like sd_pr_command, before use sshdr, we need to check the result
of scsi_execute.
Signed-off-by: zhengbin <zhengbin13@huawei.com>
---
drivers/scsi/scsi.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 4f76841..15d1fe9 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -509,7 +509,7 @@ int scsi_report_opcode(struct scsi_device *sdev, unsigned char *buffer,
result = scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buffer, len,
&sshdr, 30 * HZ, 3, NULL);
- if (result && scsi_sense_valid(&sshdr) &&
+ if (driver_byte(result) == DRIVER_SENSE && scsi_sense_valid(&sshdr) &&
sshdr.sense_key == ILLEGAL_REQUEST &&
(sshdr.asc == 0x20 || sshdr.asc == 0x24) && sshdr.ascq == 0x00)
return -EINVAL;
--
2.7.4
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v5 02/13] scsi: core: need to check the result of scsi_execute in scsi_test_unit_ready
2019-10-18 8:24 [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr zhengbin
2019-10-18 8:24 ` [PATCH v5 01/13] scsi: core: need to check the result of scsi_execute in scsi_report_opcode zhengbin
@ 2019-10-18 8:24 ` zhengbin
2019-10-18 8:24 ` [PATCH v5 03/13] scsi: core: need to check the result of scsi_execute in scsi_report_lun_scan zhengbin
` (12 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: zhengbin @ 2019-10-18 8:24 UTC (permalink / raw)
To: bvanassche, jejb, martin.petersen, linux-scsi
Cc: yi.zhang, yanaijie, zhengbin13
Like sd_pr_command, before use sshdr, we need to check the result
of scsi_execute.
Signed-off-by: zhengbin <zhengbin13@huawei.com>
---
drivers/scsi/scsi_lib.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 5447738..2970190 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -2216,6 +2216,9 @@ scsi_test_unit_ready(struct scsi_device *sdev, int timeout, int retries,
do {
result = scsi_execute_req(sdev, cmd, DMA_NONE, NULL, 0, sshdr,
timeout, 1, NULL);
+ if (driver_byte(result) != DRIVER_SENSE)
+ break;
+
if (sdev->removable && scsi_sense_valid(sshdr) &&
sshdr->sense_key == UNIT_ATTENTION)
sdev->changed = 1;
--
2.7.4
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v5 03/13] scsi: core: need to check the result of scsi_execute in scsi_report_lun_scan
2019-10-18 8:24 [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr zhengbin
2019-10-18 8:24 ` [PATCH v5 01/13] scsi: core: need to check the result of scsi_execute in scsi_report_opcode zhengbin
2019-10-18 8:24 ` [PATCH v5 02/13] scsi: core: need to check the result of scsi_execute in scsi_test_unit_ready zhengbin
@ 2019-10-18 8:24 ` zhengbin
2019-10-18 8:24 ` [PATCH v5 04/13] scsi: sr: need to check the result of scsi_execute in sr_get_events zhengbin
` (11 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: zhengbin @ 2019-10-18 8:24 UTC (permalink / raw)
To: bvanassche, jejb, martin.petersen, linux-scsi
Cc: yi.zhang, yanaijie, zhengbin13
Like sd_pr_command, before use sshdr, we need to check the result
of scsi_execute.
Signed-off-by: zhengbin <zhengbin13@huawei.com>
---
drivers/scsi/scsi_scan.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 058079f..781f82a 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1385,7 +1385,8 @@ static int scsi_report_lun_scan(struct scsi_target *starget, blist_flags_t bflag
retries, result));
if (result == 0)
break;
- else if (scsi_sense_valid(&sshdr)) {
+ else if (driver_byte(result) == DRIVER_SENSE &&
+ scsi_sense_valid(&sshdr)) {
if (sshdr.sense_key != UNIT_ATTENTION)
break;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v5 04/13] scsi: sr: need to check the result of scsi_execute in sr_get_events
2019-10-18 8:24 [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr zhengbin
` (2 preceding siblings ...)
2019-10-18 8:24 ` [PATCH v5 03/13] scsi: core: need to check the result of scsi_execute in scsi_report_lun_scan zhengbin
@ 2019-10-18 8:24 ` zhengbin
2019-10-18 8:24 ` [PATCH v5 05/13] scsi: sr: need to check the result of scsi_execute in sr_do_ioctl zhengbin
` (10 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: zhengbin @ 2019-10-18 8:24 UTC (permalink / raw)
To: bvanassche, jejb, martin.petersen, linux-scsi
Cc: yi.zhang, yanaijie, zhengbin13
Like sd_pr_command, before use sshdr, we need to check the result
of scsi_execute.
Signed-off-by: zhengbin <zhengbin13@huawei.com>
---
drivers/scsi/sr.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 4664fdf..5c5476b 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -205,7 +205,8 @@ static unsigned int sr_get_events(struct scsi_device *sdev)
result = scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buf, sizeof(buf),
&sshdr, SR_TIMEOUT, MAX_RETRIES, NULL);
- if (scsi_sense_valid(&sshdr) && sshdr.sense_key == UNIT_ATTENTION)
+ if (driver_byte(result) == DRIVER_SENSE && scsi_sense_valid(&sshdr) &&
+ sshdr.sense_key == UNIT_ATTENTION)
return DISK_EVENT_MEDIA_CHANGE;
if (result || be16_to_cpu(eh->data_len) < sizeof(*med))
--
2.7.4
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v5 05/13] scsi: sr: need to check the result of scsi_execute in sr_do_ioctl
2019-10-18 8:24 [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr zhengbin
` (3 preceding siblings ...)
2019-10-18 8:24 ` [PATCH v5 04/13] scsi: sr: need to check the result of scsi_execute in sr_get_events zhengbin
@ 2019-10-18 8:24 ` zhengbin
2019-10-18 8:24 ` [PATCH v5 06/13] scsi: scsi_dh_emc: need to check the result of scsi_execute in send_trespass_cmd zhengbin
` (9 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: zhengbin @ 2019-10-18 8:24 UTC (permalink / raw)
To: bvanassche, jejb, martin.petersen, linux-scsi
Cc: yi.zhang, yanaijie, zhengbin13
Like sd_pr_command, before use sshdr, we need to check the result
of scsi_execute, also need to check whether sshdr is valid.
Signed-off-by: zhengbin <zhengbin13@huawei.com>
---
drivers/scsi/sr_ioctl.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/scsi/sr_ioctl.c b/drivers/scsi/sr_ioctl.c
index ffcf902..7672fc1 100644
--- a/drivers/scsi/sr_ioctl.c
+++ b/drivers/scsi/sr_ioctl.c
@@ -206,6 +206,12 @@ int sr_do_ioctl(Scsi_CD *cd, struct packet_command *cgc)
/* Minimal error checking. Ignore cases we know about, and report the rest. */
if (driver_byte(result) != 0) {
+ if (driver_byte(result) != DRIVER_SENSE ||
+ !scsi_sense_valid(sshdr)) {
+ err = -EIO;
+ goto out;
+ }
+
switch (sshdr->sense_key) {
case UNIT_ATTENTION:
SDev->changed = 1;
--
2.7.4
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v5 06/13] scsi: scsi_dh_emc: need to check the result of scsi_execute in send_trespass_cmd
2019-10-18 8:24 [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr zhengbin
` (4 preceding siblings ...)
2019-10-18 8:24 ` [PATCH v5 05/13] scsi: sr: need to check the result of scsi_execute in sr_do_ioctl zhengbin
@ 2019-10-18 8:24 ` zhengbin
2019-10-18 8:24 ` [PATCH v5 07/13] scsi: scsi_dh_rdac: need to check the result of scsi_execute in send_mode_select zhengbin
` (8 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: zhengbin @ 2019-10-18 8:24 UTC (permalink / raw)
To: bvanassche, jejb, martin.petersen, linux-scsi
Cc: yi.zhang, yanaijie, zhengbin13
Like sd_pr_command, before use sshdr, we need to check the result
of scsi_execute.
Signed-off-by: zhengbin <zhengbin13@huawei.com>
---
drivers/scsi/device_handler/scsi_dh_emc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/device_handler/scsi_dh_emc.c b/drivers/scsi/device_handler/scsi_dh_emc.c
index caa685c..cf49378 100644
--- a/drivers/scsi/device_handler/scsi_dh_emc.c
+++ b/drivers/scsi/device_handler/scsi_dh_emc.c
@@ -267,7 +267,8 @@ static int send_trespass_cmd(struct scsi_device *sdev,
&sshdr, CLARIION_TIMEOUT * HZ, CLARIION_RETRIES,
req_flags, 0, NULL);
if (err) {
- if (scsi_sense_valid(&sshdr))
+ if (driver_byte(err) == DRIVER_SENSE &&
+ scsi_sense_valid(&sshdr))
res = trespass_endio(sdev, &sshdr);
else {
sdev_printk(KERN_INFO, sdev,
--
2.7.4
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v5 07/13] scsi: scsi_dh_rdac: need to check the result of scsi_execute in send_mode_select
2019-10-18 8:24 [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr zhengbin
` (5 preceding siblings ...)
2019-10-18 8:24 ` [PATCH v5 06/13] scsi: scsi_dh_emc: need to check the result of scsi_execute in send_trespass_cmd zhengbin
@ 2019-10-18 8:24 ` zhengbin
2019-10-18 8:24 ` [PATCH v5 08/13] scsi: scsi_dh_hp_sw: need to check the result of scsi_execute in hp_sw_tur,hp_sw_start_stop zhengbin
` (7 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: zhengbin @ 2019-10-18 8:24 UTC (permalink / raw)
To: bvanassche, jejb, martin.petersen, linux-scsi
Cc: yi.zhang, yanaijie, zhengbin13
Like sd_pr_command, before use sshdr, we need to check the result
of scsi_execute.
Signed-off-by: zhengbin <zhengbin13@huawei.com>
---
drivers/scsi/device_handler/scsi_dh_rdac.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/device_handler/scsi_dh_rdac.c b/drivers/scsi/device_handler/scsi_dh_rdac.c
index 5efc959..2a43985 100644
--- a/drivers/scsi/device_handler/scsi_dh_rdac.c
+++ b/drivers/scsi/device_handler/scsi_dh_rdac.c
@@ -531,6 +531,7 @@ static void send_mode_select(struct work_struct *work)
struct scsi_device *sdev = ctlr->ms_sdev;
struct rdac_dh_data *h = sdev->handler_data;
int err = SCSI_DH_OK, retry_cnt = RDAC_RETRY_COUNT;
+ int result;
struct rdac_queue_data *tmp, *qdata;
LIST_HEAD(list);
unsigned char cdb[MAX_COMMAND_SIZE];
@@ -555,9 +556,10 @@ static void send_mode_select(struct work_struct *work)
(char *) h->ctlr->array_name, h->ctlr->index,
(retry_cnt == RDAC_RETRY_COUNT) ? "queueing" : "retrying");
- if (scsi_execute(sdev, cdb, DMA_TO_DEVICE, &h->ctlr->mode_select,
- data_size, NULL, &sshdr, RDAC_TIMEOUT * HZ,
- RDAC_RETRIES, req_flags, 0, NULL)) {
+ result = scsi_execute(sdev, cdb, DMA_TO_DEVICE, &h->ctlr->mode_select,
+ data_size, NULL, &sshdr, RDAC_TIMEOUT * HZ,
+ RDAC_RETRIES, req_flags, 0, NULL);
+ if (driver_byte(result) == DRIVER_SENSE) {
err = mode_select_handle_sense(sdev, &sshdr);
if (err == SCSI_DH_RETRY && retry_cnt--)
goto retry;
--
2.7.4
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v5 08/13] scsi: scsi_dh_hp_sw: need to check the result of scsi_execute in hp_sw_tur,hp_sw_start_stop
2019-10-18 8:24 [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr zhengbin
` (6 preceding siblings ...)
2019-10-18 8:24 ` [PATCH v5 07/13] scsi: scsi_dh_rdac: need to check the result of scsi_execute in send_mode_select zhengbin
@ 2019-10-18 8:24 ` zhengbin
2019-10-21 9:08 ` [PATCH v5 08/13] scsi: scsi_dh_hp_sw: need to check the result of scsi_execute in hp_sw_tur, hp_sw_start_stop kbuild test robot
2019-10-18 8:24 ` [PATCH v5 09/13] scsi: scsi_dh_alua: need to check the result of scsi_execute in alua_rtpg,alua_stpg zhengbin
` (6 subsequent siblings)
14 siblings, 1 reply; 26+ messages in thread
From: zhengbin @ 2019-10-18 8:24 UTC (permalink / raw)
To: bvanassche, jejb, martin.petersen, linux-scsi
Cc: yi.zhang, yanaijie, zhengbin13
Like sd_pr_command, before use sshdr, we need to check the result
of scsi_execute.
Signed-off-by: zhengbin <zhengbin13@huawei.com>
---
drivers/scsi/device_handler/scsi_dh_hp_sw.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/device_handler/scsi_dh_hp_sw.c b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
index 8acd4bb..be8c139 100644
--- a/drivers/scsi/device_handler/scsi_dh_hp_sw.c
+++ b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
@@ -90,7 +90,8 @@ static int hp_sw_tur(struct scsi_device *sdev, struct hp_sw_dh_data *h)
res = scsi_execute(sdev, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
HP_SW_TIMEOUT, HP_SW_RETRIES, req_flags, 0, NULL);
if (res) {
- if (scsi_sense_valid(&sshdr))
+ if (driver_byte(res) == DRIVER_SENSE &&
+ scsi_sense_valid(&sshdr))
ret = tur_done(sdev, h, &sshdr);
else {
sdev_printk(KERN_WARNING, sdev,
@@ -128,7 +129,8 @@ static int hp_sw_start_stop(struct hp_sw_dh_data *h)
res = scsi_execute(sdev, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
HP_SW_TIMEOUT, HP_SW_RETRIES, req_flags, 0, NULL);
if (res) {
- if (!scsi_sense_valid(&sshdr)) {
+ if (driver_byte(result) != DRIVER_SENSE ||
+ !scsi_sense_valid(&sshdr)) {
sdev_printk(KERN_WARNING, sdev,
"%s: sending start_stop_unit failed, "
"no sense available\n", HP_SW_NAME);
--
2.7.4
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v5 08/13] scsi: scsi_dh_hp_sw: need to check the result of scsi_execute in hp_sw_tur,hp_sw_start_stop
2019-10-18 8:24 ` [PATCH v5 08/13] scsi: scsi_dh_hp_sw: need to check the result of scsi_execute in hp_sw_tur,hp_sw_start_stop zhengbin
@ 2019-10-21 9:08 ` kbuild test robot
0 siblings, 0 replies; 26+ messages in thread
From: kbuild test robot @ 2019-10-21 9:08 UTC (permalink / raw)
To: zhengbin
Cc: kbuild-all, bvanassche, jejb, martin.petersen, linux-scsi,
yi.zhang, yanaijie, zhengbin13
[-- Attachment #1: Type: text/plain, Size: 6881 bytes --]
Hi zhengbin,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on mkp-scsi/for-next]
[cannot apply to v5.4-rc4 next-20191018]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/zhengbin/scsi-core-fix-uninit-value-access-of-variable-sshdr/20191021-160007
base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 7.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=sh
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from drivers/scsi/device_handler/scsi_dh_hp_sw.c:13:0:
drivers/scsi/device_handler/scsi_dh_hp_sw.c: In function 'hp_sw_start_stop':
>> drivers/scsi/device_handler/scsi_dh_hp_sw.c:132:19: error: 'result' undeclared (first use in this function); did you mean 'res'?
if (driver_byte(result) != DRIVER_SENSE ||
^
include/scsi/scsi.h:213:32: note: in definition of macro 'driver_byte'
#define driver_byte(result) (((result) >> 24) & 0xff)
^~~~~~
drivers/scsi/device_handler/scsi_dh_hp_sw.c:132:19: note: each undeclared identifier is reported only once for each function it appears in
if (driver_byte(result) != DRIVER_SENSE ||
^
include/scsi/scsi.h:213:32: note: in definition of macro 'driver_byte'
#define driver_byte(result) (((result) >> 24) & 0xff)
^~~~~~
vim +132 drivers/scsi/device_handler/scsi_dh_hp_sw.c
> 13 #include <scsi/scsi.h>
14 #include <scsi/scsi_dbg.h>
15 #include <scsi/scsi_eh.h>
16 #include <scsi/scsi_dh.h>
17
18 #define HP_SW_NAME "hp_sw"
19
20 #define HP_SW_TIMEOUT (60 * HZ)
21 #define HP_SW_RETRIES 3
22
23 #define HP_SW_PATH_UNINITIALIZED -1
24 #define HP_SW_PATH_ACTIVE 0
25 #define HP_SW_PATH_PASSIVE 1
26
27 struct hp_sw_dh_data {
28 int path_state;
29 int retries;
30 int retry_cnt;
31 struct scsi_device *sdev;
32 };
33
34 static int hp_sw_start_stop(struct hp_sw_dh_data *);
35
36 /*
37 * tur_done - Handle TEST UNIT READY return status
38 * @sdev: sdev the command has been sent to
39 * @errors: blk error code
40 *
41 * Returns SCSI_DH_DEV_OFFLINED if the sdev is on the passive path
42 */
43 static int tur_done(struct scsi_device *sdev, struct hp_sw_dh_data *h,
44 struct scsi_sense_hdr *sshdr)
45 {
46 int ret = SCSI_DH_IO;
47
48 switch (sshdr->sense_key) {
49 case UNIT_ATTENTION:
50 ret = SCSI_DH_IMM_RETRY;
51 break;
52 case NOT_READY:
53 if (sshdr->asc == 0x04 && sshdr->ascq == 2) {
54 /*
55 * LUN not ready - Initialization command required
56 *
57 * This is the passive path
58 */
59 h->path_state = HP_SW_PATH_PASSIVE;
60 ret = SCSI_DH_OK;
61 break;
62 }
63 /* Fallthrough */
64 default:
65 sdev_printk(KERN_WARNING, sdev,
66 "%s: sending tur failed, sense %x/%x/%x\n",
67 HP_SW_NAME, sshdr->sense_key, sshdr->asc,
68 sshdr->ascq);
69 break;
70 }
71 return ret;
72 }
73
74 /*
75 * hp_sw_tur - Send TEST UNIT READY
76 * @sdev: sdev command should be sent to
77 *
78 * Use the TEST UNIT READY command to determine
79 * the path state.
80 */
81 static int hp_sw_tur(struct scsi_device *sdev, struct hp_sw_dh_data *h)
82 {
83 unsigned char cmd[6] = { TEST_UNIT_READY };
84 struct scsi_sense_hdr sshdr;
85 int ret = SCSI_DH_OK, res;
86 u64 req_flags = REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
87 REQ_FAILFAST_DRIVER;
88
89 retry:
90 res = scsi_execute(sdev, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
91 HP_SW_TIMEOUT, HP_SW_RETRIES, req_flags, 0, NULL);
92 if (res) {
93 if (driver_byte(res) == DRIVER_SENSE &&
94 scsi_sense_valid(&sshdr))
95 ret = tur_done(sdev, h, &sshdr);
96 else {
97 sdev_printk(KERN_WARNING, sdev,
98 "%s: sending tur failed with %x\n",
99 HP_SW_NAME, res);
100 ret = SCSI_DH_IO;
101 }
102 } else {
103 h->path_state = HP_SW_PATH_ACTIVE;
104 ret = SCSI_DH_OK;
105 }
106 if (ret == SCSI_DH_IMM_RETRY)
107 goto retry;
108
109 return ret;
110 }
111
112 /*
113 * hp_sw_start_stop - Send START STOP UNIT command
114 * @sdev: sdev command should be sent to
115 *
116 * Sending START STOP UNIT activates the SP.
117 */
118 static int hp_sw_start_stop(struct hp_sw_dh_data *h)
119 {
120 unsigned char cmd[6] = { START_STOP, 0, 0, 0, 1, 0 };
121 struct scsi_sense_hdr sshdr;
122 struct scsi_device *sdev = h->sdev;
123 int res, rc = SCSI_DH_OK;
124 int retry_cnt = HP_SW_RETRIES;
125 u64 req_flags = REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
126 REQ_FAILFAST_DRIVER;
127
128 retry:
129 res = scsi_execute(sdev, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
130 HP_SW_TIMEOUT, HP_SW_RETRIES, req_flags, 0, NULL);
131 if (res) {
> 132 if (driver_byte(result) != DRIVER_SENSE ||
133 !scsi_sense_valid(&sshdr)) {
134 sdev_printk(KERN_WARNING, sdev,
135 "%s: sending start_stop_unit failed, "
136 "no sense available\n", HP_SW_NAME);
137 return SCSI_DH_IO;
138 }
139 switch (sshdr.sense_key) {
140 case NOT_READY:
141 if (sshdr.asc == 0x04 && sshdr.ascq == 3) {
142 /*
143 * LUN not ready - manual intervention required
144 *
145 * Switch-over in progress, retry.
146 */
147 if (--retry_cnt)
148 goto retry;
149 rc = SCSI_DH_RETRY;
150 break;
151 }
152 /* fall through */
153 default:
154 sdev_printk(KERN_WARNING, sdev,
155 "%s: sending start_stop_unit failed, "
156 "sense %x/%x/%x\n", HP_SW_NAME,
157 sshdr.sense_key, sshdr.asc, sshdr.ascq);
158 rc = SCSI_DH_IO;
159 }
160 }
161 return rc;
162 }
163
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 52246 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v5 08/13] scsi: scsi_dh_hp_sw: need to check the result of scsi_execute in hp_sw_tur, hp_sw_start_stop
@ 2019-10-21 9:08 ` kbuild test robot
0 siblings, 0 replies; 26+ messages in thread
From: kbuild test robot @ 2019-10-21 9:08 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 7078 bytes --]
Hi zhengbin,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on mkp-scsi/for-next]
[cannot apply to v5.4-rc4 next-20191018]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/zhengbin/scsi-core-fix-uninit-value-access-of-variable-sshdr/20191021-160007
base: https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 7.4.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=7.4.0 make.cross ARCH=sh
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from drivers/scsi/device_handler/scsi_dh_hp_sw.c:13:0:
drivers/scsi/device_handler/scsi_dh_hp_sw.c: In function 'hp_sw_start_stop':
>> drivers/scsi/device_handler/scsi_dh_hp_sw.c:132:19: error: 'result' undeclared (first use in this function); did you mean 'res'?
if (driver_byte(result) != DRIVER_SENSE ||
^
include/scsi/scsi.h:213:32: note: in definition of macro 'driver_byte'
#define driver_byte(result) (((result) >> 24) & 0xff)
^~~~~~
drivers/scsi/device_handler/scsi_dh_hp_sw.c:132:19: note: each undeclared identifier is reported only once for each function it appears in
if (driver_byte(result) != DRIVER_SENSE ||
^
include/scsi/scsi.h:213:32: note: in definition of macro 'driver_byte'
#define driver_byte(result) (((result) >> 24) & 0xff)
^~~~~~
vim +132 drivers/scsi/device_handler/scsi_dh_hp_sw.c
> 13 #include <scsi/scsi.h>
14 #include <scsi/scsi_dbg.h>
15 #include <scsi/scsi_eh.h>
16 #include <scsi/scsi_dh.h>
17
18 #define HP_SW_NAME "hp_sw"
19
20 #define HP_SW_TIMEOUT (60 * HZ)
21 #define HP_SW_RETRIES 3
22
23 #define HP_SW_PATH_UNINITIALIZED -1
24 #define HP_SW_PATH_ACTIVE 0
25 #define HP_SW_PATH_PASSIVE 1
26
27 struct hp_sw_dh_data {
28 int path_state;
29 int retries;
30 int retry_cnt;
31 struct scsi_device *sdev;
32 };
33
34 static int hp_sw_start_stop(struct hp_sw_dh_data *);
35
36 /*
37 * tur_done - Handle TEST UNIT READY return status
38 * @sdev: sdev the command has been sent to
39 * @errors: blk error code
40 *
41 * Returns SCSI_DH_DEV_OFFLINED if the sdev is on the passive path
42 */
43 static int tur_done(struct scsi_device *sdev, struct hp_sw_dh_data *h,
44 struct scsi_sense_hdr *sshdr)
45 {
46 int ret = SCSI_DH_IO;
47
48 switch (sshdr->sense_key) {
49 case UNIT_ATTENTION:
50 ret = SCSI_DH_IMM_RETRY;
51 break;
52 case NOT_READY:
53 if (sshdr->asc == 0x04 && sshdr->ascq == 2) {
54 /*
55 * LUN not ready - Initialization command required
56 *
57 * This is the passive path
58 */
59 h->path_state = HP_SW_PATH_PASSIVE;
60 ret = SCSI_DH_OK;
61 break;
62 }
63 /* Fallthrough */
64 default:
65 sdev_printk(KERN_WARNING, sdev,
66 "%s: sending tur failed, sense %x/%x/%x\n",
67 HP_SW_NAME, sshdr->sense_key, sshdr->asc,
68 sshdr->ascq);
69 break;
70 }
71 return ret;
72 }
73
74 /*
75 * hp_sw_tur - Send TEST UNIT READY
76 * @sdev: sdev command should be sent to
77 *
78 * Use the TEST UNIT READY command to determine
79 * the path state.
80 */
81 static int hp_sw_tur(struct scsi_device *sdev, struct hp_sw_dh_data *h)
82 {
83 unsigned char cmd[6] = { TEST_UNIT_READY };
84 struct scsi_sense_hdr sshdr;
85 int ret = SCSI_DH_OK, res;
86 u64 req_flags = REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
87 REQ_FAILFAST_DRIVER;
88
89 retry:
90 res = scsi_execute(sdev, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
91 HP_SW_TIMEOUT, HP_SW_RETRIES, req_flags, 0, NULL);
92 if (res) {
93 if (driver_byte(res) == DRIVER_SENSE &&
94 scsi_sense_valid(&sshdr))
95 ret = tur_done(sdev, h, &sshdr);
96 else {
97 sdev_printk(KERN_WARNING, sdev,
98 "%s: sending tur failed with %x\n",
99 HP_SW_NAME, res);
100 ret = SCSI_DH_IO;
101 }
102 } else {
103 h->path_state = HP_SW_PATH_ACTIVE;
104 ret = SCSI_DH_OK;
105 }
106 if (ret == SCSI_DH_IMM_RETRY)
107 goto retry;
108
109 return ret;
110 }
111
112 /*
113 * hp_sw_start_stop - Send START STOP UNIT command
114 * @sdev: sdev command should be sent to
115 *
116 * Sending START STOP UNIT activates the SP.
117 */
118 static int hp_sw_start_stop(struct hp_sw_dh_data *h)
119 {
120 unsigned char cmd[6] = { START_STOP, 0, 0, 0, 1, 0 };
121 struct scsi_sense_hdr sshdr;
122 struct scsi_device *sdev = h->sdev;
123 int res, rc = SCSI_DH_OK;
124 int retry_cnt = HP_SW_RETRIES;
125 u64 req_flags = REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
126 REQ_FAILFAST_DRIVER;
127
128 retry:
129 res = scsi_execute(sdev, cmd, DMA_NONE, NULL, 0, NULL, &sshdr,
130 HP_SW_TIMEOUT, HP_SW_RETRIES, req_flags, 0, NULL);
131 if (res) {
> 132 if (driver_byte(result) != DRIVER_SENSE ||
133 !scsi_sense_valid(&sshdr)) {
134 sdev_printk(KERN_WARNING, sdev,
135 "%s: sending start_stop_unit failed, "
136 "no sense available\n", HP_SW_NAME);
137 return SCSI_DH_IO;
138 }
139 switch (sshdr.sense_key) {
140 case NOT_READY:
141 if (sshdr.asc == 0x04 && sshdr.ascq == 3) {
142 /*
143 * LUN not ready - manual intervention required
144 *
145 * Switch-over in progress, retry.
146 */
147 if (--retry_cnt)
148 goto retry;
149 rc = SCSI_DH_RETRY;
150 break;
151 }
152 /* fall through */
153 default:
154 sdev_printk(KERN_WARNING, sdev,
155 "%s: sending start_stop_unit failed, "
156 "sense %x/%x/%x\n", HP_SW_NAME,
157 sshdr.sense_key, sshdr.asc, sshdr.ascq);
158 rc = SCSI_DH_IO;
159 }
160 }
161 return rc;
162 }
163
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 52246 bytes --]
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v5 09/13] scsi: scsi_dh_alua: need to check the result of scsi_execute in alua_rtpg,alua_stpg
2019-10-18 8:24 [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr zhengbin
` (7 preceding siblings ...)
2019-10-18 8:24 ` [PATCH v5 08/13] scsi: scsi_dh_hp_sw: need to check the result of scsi_execute in hp_sw_tur,hp_sw_start_stop zhengbin
@ 2019-10-18 8:24 ` zhengbin
2019-10-18 8:24 ` [PATCH v5 10/13] scsi: scsi_transport_spi: need to check whether sshdr is valid in spi_execute zhengbin
` (5 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: zhengbin @ 2019-10-18 8:24 UTC (permalink / raw)
To: bvanassche, jejb, martin.petersen, linux-scsi
Cc: yi.zhang, yanaijie, zhengbin13
Like sd_pr_command, before use sshdr, we need to check the result
of scsi_execute.
submit_rtpg/submit_stpg
scsi_execute
Signed-off-by: zhengbin <zhengbin13@huawei.com>
---
drivers/scsi/device_handler/scsi_dh_alua.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
index f32da0c..97c38ed 100644
--- a/drivers/scsi/device_handler/scsi_dh_alua.c
+++ b/drivers/scsi/device_handler/scsi_dh_alua.c
@@ -548,7 +548,8 @@ static int alua_rtpg(struct scsi_device *sdev, struct alua_port_group *pg)
kfree(buff);
return SCSI_DH_OK;
}
- if (!scsi_sense_valid(&sense_hdr)) {
+ if (driver_byte(retval) != DRIVER_SENSE ||
+ !scsi_sense_valid(&sense_hdr)) {
sdev_printk(KERN_INFO, sdev,
"%s: rtpg failed, result %d\n",
ALUA_DH_NAME, retval);
@@ -773,7 +774,8 @@ static unsigned alua_stpg(struct scsi_device *sdev, struct alua_port_group *pg)
retval = submit_stpg(sdev, pg->group_id, &sense_hdr);
if (retval) {
- if (!scsi_sense_valid(&sense_hdr)) {
+ if (driver_byte(retval) != DRIVER_SENSE ||
+ !scsi_sense_valid(&sense_hdr)) {
sdev_printk(KERN_INFO, sdev,
"%s: stpg failed, result %d",
ALUA_DH_NAME, retval);
--
2.7.4
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v5 10/13] scsi: scsi_transport_spi: need to check whether sshdr is valid in spi_execute
2019-10-18 8:24 [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr zhengbin
` (8 preceding siblings ...)
2019-10-18 8:24 ` [PATCH v5 09/13] scsi: scsi_dh_alua: need to check the result of scsi_execute in alua_rtpg,alua_stpg zhengbin
@ 2019-10-18 8:24 ` zhengbin
2019-10-18 8:24 ` [PATCH v5 11/13] scsi: cxlflash: need to check whether sshdr is valid in read_cap16 zhengbin
` (4 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: zhengbin @ 2019-10-18 8:24 UTC (permalink / raw)
To: bvanassche, jejb, martin.petersen, linux-scsi
Cc: yi.zhang, yanaijie, zhengbin13
Like sd_pr_command, before use sshdr, we need to check whether
sshdr is valid.
Signed-off-by: zhengbin <zhengbin13@huawei.com>
---
drivers/scsi/scsi_transport_spi.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/scsi/scsi_transport_spi.c b/drivers/scsi/scsi_transport_spi.c
index f866106..428f9b8 100644
--- a/drivers/scsi/scsi_transport_spi.c
+++ b/drivers/scsi/scsi_transport_spi.c
@@ -124,6 +124,7 @@ static int spi_execute(struct scsi_device *sdev, const void *cmd,
REQ_FAILFAST_DRIVER,
0, NULL);
if (driver_byte(result) != DRIVER_SENSE ||
+ !scsi_sense_valid(sshdr) ||
sshdr->sense_key != UNIT_ATTENTION)
break;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v5 11/13] scsi: cxlflash: need to check whether sshdr is valid in read_cap16
2019-10-18 8:24 [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr zhengbin
` (9 preceding siblings ...)
2019-10-18 8:24 ` [PATCH v5 10/13] scsi: scsi_transport_spi: need to check whether sshdr is valid in spi_execute zhengbin
@ 2019-10-18 8:24 ` zhengbin
2019-10-18 8:24 ` [PATCH v5 12/13] scsi: ufs: need to check whether sshdr is valid in ufshcd_set_dev_pwr_mode zhengbin
` (3 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: zhengbin @ 2019-10-18 8:24 UTC (permalink / raw)
To: bvanassche, jejb, martin.petersen, linux-scsi
Cc: yi.zhang, yanaijie, zhengbin13
Like sd_pr_command, before use sshdr, we need to check whether
sshdr is valid.
Signed-off-by: zhengbin <zhengbin13@huawei.com>
---
drivers/scsi/cxlflash/superpipe.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/cxlflash/superpipe.c b/drivers/scsi/cxlflash/superpipe.c
index 593669a..1e90ff3 100644
--- a/drivers/scsi/cxlflash/superpipe.c
+++ b/drivers/scsi/cxlflash/superpipe.c
@@ -369,7 +369,7 @@ static int read_cap16(struct scsi_device *sdev, struct llun_info *lli)
goto out;
}
- if (driver_byte(result) == DRIVER_SENSE) {
+ if (driver_byte(result) == DRIVER_SENSE && scsi_sense_valid(&sshdr)) {
result &= ~(0xFF<<24); /* DRIVER_SENSE is not an error */
if (result & SAM_STAT_CHECK_CONDITION) {
switch (sshdr.sense_key) {
--
2.7.4
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v5 12/13] scsi: ufs: need to check whether sshdr is valid in ufshcd_set_dev_pwr_mode
2019-10-18 8:24 [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr zhengbin
` (10 preceding siblings ...)
2019-10-18 8:24 ` [PATCH v5 11/13] scsi: cxlflash: need to check whether sshdr is valid in read_cap16 zhengbin
@ 2019-10-18 8:24 ` zhengbin
2019-10-18 8:24 ` [PATCH v5 13/13] scsi: ch: need to check whether sshdr is valid in ch_do_scsi zhengbin
` (2 subsequent siblings)
14 siblings, 0 replies; 26+ messages in thread
From: zhengbin @ 2019-10-18 8:24 UTC (permalink / raw)
To: bvanassche, jejb, martin.petersen, linux-scsi
Cc: yi.zhang, yanaijie, zhengbin13
Like sd_pr_command, before use sshdr, we need to check whether
sshdr is valid.
Signed-off-by: zhengbin <zhengbin13@huawei.com>
---
drivers/scsi/ufs/ufshcd.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index c28c144..5afc426 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -7600,7 +7600,8 @@ static int ufshcd_set_dev_pwr_mode(struct ufs_hba *hba,
sdev_printk(KERN_WARNING, sdp,
"START_STOP failed for power mode: %d, result %x\n",
pwr_mode, ret);
- if (driver_byte(ret) == DRIVER_SENSE)
+ if (driver_byte(ret) == DRIVER_SENSE &&
+ scsi_sense_valid(&sshdr))
scsi_print_sense_hdr(sdp, NULL, &sshdr);
}
--
2.7.4
^ permalink raw reply related [flat|nested] 26+ messages in thread* [PATCH v5 13/13] scsi: ch: need to check whether sshdr is valid in ch_do_scsi
2019-10-18 8:24 [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr zhengbin
` (11 preceding siblings ...)
2019-10-18 8:24 ` [PATCH v5 12/13] scsi: ufs: need to check whether sshdr is valid in ufshcd_set_dev_pwr_mode zhengbin
@ 2019-10-18 8:24 ` zhengbin
2019-10-18 9:41 ` [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr Hannes Reinecke
2019-10-18 12:33 ` Damien Le Moal
14 siblings, 0 replies; 26+ messages in thread
From: zhengbin @ 2019-10-18 8:24 UTC (permalink / raw)
To: bvanassche, jejb, martin.petersen, linux-scsi
Cc: yi.zhang, yanaijie, zhengbin13
Like sd_pr_command, before use sshdr, we need to check whether
sshdr is valid.
Signed-off-by: zhengbin <zhengbin13@huawei.com>
---
drivers/scsi/ch.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/ch.c b/drivers/scsi/ch.c
index 76751d6..dba6fe2 100644
--- a/drivers/scsi/ch.c
+++ b/drivers/scsi/ch.c
@@ -200,7 +200,7 @@ ch_do_scsi(scsi_changer *ch, unsigned char *cmd, int cmd_len,
buflength, &sshdr, timeout * HZ,
MAX_RETRIES, NULL);
- if (driver_byte(result) == DRIVER_SENSE) {
+ if (driver_byte(result) == DRIVER_SENSE && scsi_sense_valid(&sshdr)) {
if (debug)
scsi_print_sense_hdr(ch->device, ch->name, &sshdr);
errno = ch_find_errno(&sshdr);
@@ -212,7 +212,9 @@ ch_do_scsi(scsi_changer *ch, unsigned char *cmd, int cmd_len,
goto retry;
break;
}
- }
+ } else if (result)
+ errno = -EIO;
+
return errno;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr
2019-10-18 8:24 [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr zhengbin
` (12 preceding siblings ...)
2019-10-18 8:24 ` [PATCH v5 13/13] scsi: ch: need to check whether sshdr is valid in ch_do_scsi zhengbin
@ 2019-10-18 9:41 ` Hannes Reinecke
2019-10-18 13:43 ` Martin K. Petersen
2019-10-18 12:33 ` Damien Le Moal
14 siblings, 1 reply; 26+ messages in thread
From: Hannes Reinecke @ 2019-10-18 9:41 UTC (permalink / raw)
To: zhengbin, bvanassche, jejb, martin.petersen, linux-scsi
Cc: yi.zhang, yanaijie
On 10/18/19 10:24 AM, zhengbin wrote:
> v1->v2: modify the comments suggested by Bart
> v2->v3: fix bug in sr_do_ioctl
> v3->v4: let "fix bug in sr_do_ioctl" be a separate patch
> v4->v5: fix uninit-value access bug in callers, not in __scsi_execute
>
> zhengbin (13):
> scsi: core: need to check the result of scsi_execute in
> scsi_report_opcode
> scsi: core: need to check the result of scsi_execute in
> scsi_test_unit_ready
> scsi: core: need to check the result of scsi_execute in
> scsi_report_lun_scan
> scsi: sr: need to check the result of scsi_execute in sr_get_events
> scsi: sr: need to check the result of scsi_execute in sr_do_ioctl
> scsi: scsi_dh_emc: need to check the result of scsi_execute in
> send_trespass_cmd
> scsi: scsi_dh_rdac: need to check the result of scsi_execute in
> send_mode_select
> scsi: scsi_dh_hp_sw: need to check the result of scsi_execute in
> hp_sw_tur,hp_sw_start_stop
> scsi: scsi_dh_alua: need to check the result of scsi_execute in
> alua_rtpg,alua_stpg
> scsi: scsi_transport_spi: need to check whether sshdr is valid in
> spi_execute
> scsi: cxlflash: need to check whether sshdr is valid in read_cap16
> scsi: ufs: need to check whether sshdr is valid in
> ufshcd_set_dev_pwr_mode
> scsi: ch: need to check whether sshdr is valid in ch_do_scsi
>
> drivers/scsi/ch.c | 6 ++++--
> drivers/scsi/cxlflash/superpipe.c | 2 +-
> drivers/scsi/device_handler/scsi_dh_alua.c | 6 ++++--
> drivers/scsi/device_handler/scsi_dh_emc.c | 3 ++-
> drivers/scsi/device_handler/scsi_dh_hp_sw.c | 6 ++++--
> drivers/scsi/device_handler/scsi_dh_rdac.c | 8 +++++---
> drivers/scsi/scsi.c | 2 +-
> drivers/scsi/scsi_lib.c | 3 +++
> drivers/scsi/scsi_scan.c | 3 ++-
> drivers/scsi/scsi_transport_spi.c | 1 +
> drivers/scsi/sr.c | 3 ++-
> drivers/scsi/sr_ioctl.c | 6 ++++++
> drivers/scsi/ufs/ufshcd.c | 3 ++-
> 13 files changed, 37 insertions(+), 15 deletions(-)
>
> --
> 2.7.4
>
Nope.
The one thing which I patently don't like is the ambivalence between
DRIVER_SENSE and scsi_sense_valid().
What shall we do if only _one_ of them is set?
IE what would be the correct way of action if DRIVER_SENSE is not set,
but we have a valid sense code?
Or the other way around?
But more important, from a quick glance not all drivers set the
DRIVER_SENSE bit; so for things like hpsa or smartpqi the sense code is
never evaluated after this patchset.
I _really_ would prefer to ditch the 'DRIVER_SENSE' bit, and rely on
scsi_sense_valid() only.
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@suse.de +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr
2019-10-18 9:41 ` [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr Hannes Reinecke
@ 2019-10-18 13:43 ` Martin K. Petersen
2019-10-18 14:05 ` Hannes Reinecke
` (3 more replies)
0 siblings, 4 replies; 26+ messages in thread
From: Martin K. Petersen @ 2019-10-18 13:43 UTC (permalink / raw)
To: Hannes Reinecke
Cc: zhengbin, bvanassche, jejb, martin.petersen, linux-scsi,
yi.zhang, yanaijie, Johannes Thumshirn
Hannes,
> The one thing which I patently don't like is the ambivalence between
> DRIVER_SENSE and scsi_sense_valid(). What shall we do if only _one_
> of them is set? IE what would be the correct way of action if
> DRIVER_SENSE is not set, but we have a valid sense code? Or the other
> way around?
I agree, it's a mess.
(Sorry, zhengbin, you opened a can of worms. This is some of our oldest
and most arcane code in SCSI)
> But more important, from a quick glance not all drivers set the
> DRIVER_SENSE bit; so for things like hpsa or smartpqi the sense code is
> never evaluated after this patchset.
And yet we appear to have several code paths where sense evaluation is
contingent on DRIVER_SENSE. So no matter what, behavior might
change if we enforce consistent semantics. *sigh*
> I _really_ would prefer to ditch the 'DRIVER_SENSE' bit, and rely on
> scsi_sense_valid() only.
I would really like to get rid of DRIVER_* completely. Except for
DRIVER_SENSE, few are actually in use:
DRIVER_OK: 0
DRIVER_BUSY: 0
DRIVER_SOFT: 0
DRIVER_MEDIA: 0
DRIVER_ERROR: 6
DRIVER_INVALID: 4
DRIVER_TIMEOUT: 1
DRIVER_SENSE: 58
Johannes: Whatever happened to your efforts at cleaning all this up? Do
you have a patch series or a working tree we could use as starting
point?
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr
2019-10-18 13:43 ` Martin K. Petersen
@ 2019-10-18 14:05 ` Hannes Reinecke
2019-10-18 23:17 ` [RFC] scsi: Avoid sign extension when setting command result bytes, was " Finn Thain
` (2 subsequent siblings)
3 siblings, 0 replies; 26+ messages in thread
From: Hannes Reinecke @ 2019-10-18 14:05 UTC (permalink / raw)
To: Martin K. Petersen
Cc: zhengbin, bvanassche, jejb, linux-scsi, yi.zhang, yanaijie,
Johannes Thumshirn
On 10/18/19 3:43 PM, Martin K. Petersen wrote:
>
> Hannes,
>
>> The one thing which I patently don't like is the ambivalence between
>> DRIVER_SENSE and scsi_sense_valid(). What shall we do if only _one_
>> of them is set? IE what would be the correct way of action if
>> DRIVER_SENSE is not set, but we have a valid sense code? Or the other
>> way around?
>
> I agree, it's a mess.
>
> (Sorry, zhengbin, you opened a can of worms. This is some of our oldest
> and most arcane code in SCSI)
>
>> But more important, from a quick glance not all drivers set the
>> DRIVER_SENSE bit; so for things like hpsa or smartpqi the sense code is
>> never evaluated after this patchset.
>
> And yet we appear to have several code paths where sense evaluation is
> contingent on DRIVER_SENSE. So no matter what, behavior might
> change if we enforce consistent semantics. *sigh*
>
>> I _really_ would prefer to ditch the 'DRIVER_SENSE' bit, and rely on
>> scsi_sense_valid() only.
>
> I would really like to get rid of DRIVER_* completely. Except for
> DRIVER_SENSE, few are actually in use:
>
> DRIVER_OK: 0
> DRIVER_BUSY: 0
> DRIVER_SOFT: 0
> DRIVER_MEDIA: 0
> DRIVER_ERROR: 6
Three less now :-)
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@suse.de +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer
^ permalink raw reply [flat|nested] 26+ messages in thread* [RFC] scsi: Avoid sign extension when setting command result bytes, was Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr
2019-10-18 13:43 ` Martin K. Petersen
2019-10-18 14:05 ` Hannes Reinecke
@ 2019-10-18 23:17 ` Finn Thain
2019-10-21 1:49 ` zhengbin (A)
2019-10-21 6:57 ` Johannes Thumshirn
3 siblings, 0 replies; 26+ messages in thread
From: Finn Thain @ 2019-10-18 23:17 UTC (permalink / raw)
To: Martin K. Petersen
Cc: Hannes Reinecke, zhengbin, bvanassche, jejb, linux-scsi,
yi.zhang, yanaijie, Johannes Thumshirn
On Fri, 18 Oct 2019, Martin K. Petersen wrote:
>
> (Sorry, zhengbin, you opened a can of worms. This is some of our oldest
> and most arcane code in SCSI)
>
A call to set_host_byte(cmd, x) or set_msg_byte(cmd, x) when x & 0x80 is
set clobbers the most significant bytes in cmd->result.
Avoid this implicit sign extension when shifting bits by using an
unsigned formal parameter.
This is a theoretical bug, found by inspection, so I'm sending an untested
RFC patch.
I didn't try to find possible callers that might pass a negative parameter
and neither did I check whether the clobber might be intentional...
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 76ed5e4acd38..ae814fa68bb8 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -306,17 +306,17 @@ static inline struct scsi_data_buffer *scsi_prot(struct scsi_cmnd *cmd)
#define scsi_for_each_prot_sg(cmd, sg, nseg, __i) \
for_each_sg(scsi_prot_sglist(cmd), sg, nseg, __i)
-static inline void set_msg_byte(struct scsi_cmnd *cmd, char status)
+static inline void set_msg_byte(struct scsi_cmnd *cmd, unsigned char status)
{
cmd->result = (cmd->result & 0xffff00ff) | (status << 8);
}
-static inline void set_host_byte(struct scsi_cmnd *cmd, char status)
+static inline void set_host_byte(struct scsi_cmnd *cmd, unsigned char status)
{
cmd->result = (cmd->result & 0xff00ffff) | (status << 16);
}
-static inline void set_driver_byte(struct scsi_cmnd *cmd, char status)
+static inline void set_driver_byte(struct scsi_cmnd *cmd, unsigned char status)
{
cmd->result = (cmd->result & 0x00ffffff) | (status << 24);
}
^ permalink raw reply related [flat|nested] 26+ messages in thread* Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr
2019-10-18 13:43 ` Martin K. Petersen
2019-10-18 14:05 ` Hannes Reinecke
2019-10-18 23:17 ` [RFC] scsi: Avoid sign extension when setting command result bytes, was " Finn Thain
@ 2019-10-21 1:49 ` zhengbin (A)
2019-10-21 13:06 ` Hannes Reinecke
2019-10-21 6:57 ` Johannes Thumshirn
3 siblings, 1 reply; 26+ messages in thread
From: zhengbin (A) @ 2019-10-21 1:49 UTC (permalink / raw)
To: Martin K. Petersen, Hannes Reinecke
Cc: bvanassche, jejb, linux-scsi, yi.zhang, yanaijie, Johannes Thumshirn
On 2019/10/18 21:43, Martin K. Petersen wrote:
> Hannes,
>
>> The one thing which I patently don't like is the ambivalence between
>> DRIVER_SENSE and scsi_sense_valid(). What shall we do if only _one_
>> of them is set? IE what would be the correct way of action if
>> DRIVER_SENSE is not set, but we have a valid sense code? Or the other
>> way around?
> I agree, it's a mess.
>
> (Sorry, zhengbin, you opened a can of worms. This is some of our oldest
> and most arcane code in SCSI)
>
>> But more important, from a quick glance not all drivers set the
>> DRIVER_SENSE bit; so for things like hpsa or smartpqi the sense code is
>> never evaluated after this patchset.
> And yet we appear to have several code paths where sense evaluation is
> contingent on DRIVER_SENSE. So no matter what, behavior might
> change if we enforce consistent semantics. *sigh*
So what should we do to prevent unit-value access of sshdr?
1. still init sshdr in __scsi_execute?
@@ -255,6 +255,13 @@ int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
struct scsi_request *rq;
int ret = DRIVER_ERROR << 24;
+ /*
+ * Zero-initialize sshdr for those callers that check the *sshdr
+ * contents even if no sense data is available.
+ */
+ if (sshdr)
+ memset(sshdr, 0, sizeof(struct scsi_sense_hdr));
+
req = blk_get_request(sdev->request_queue,
data_direction == DMA_TO_DEVICE ?
REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT);
2. init sshdr in callers of scsi_execute, instead of check the result of scsi_execute and check
whether sshdr is valid? for example:
@@ -506,6 +506,7 @@ int scsi_report_opcode(struct scsi_device *sdev, unsigned char *buffer,
put_unaligned_be32(len, &cmd[6]);
memset(buffer, 0, len);
+ memset(&sshdr, 0 ,sizeof(sshdr));
result = scsi_execute_req(sdev, cmd, DMA_FROM_DEVICE, buffer, len,
&sshdr, 30 * HZ, 3, NULL)
Besides, should sd_pr_command init sshdr?? cause sd_pr_command have checked the result of scsi_execute
and check whether sshdr is valid.
3. get rid of DRIVER_* completely? even this, we should init sshdr first.
sshdr is just 8 bytes, init it does not affect performance
My advice is 2.
>
>> I _really_ would prefer to ditch the 'DRIVER_SENSE' bit, and rely on
>> scsi_sense_valid() only.
> I would really like to get rid of DRIVER_* completely. Except for
> DRIVER_SENSE, few are actually in use:
>
> DRIVER_OK: 0
> DRIVER_BUSY: 0
> DRIVER_SOFT: 0
> DRIVER_MEDIA: 0
> DRIVER_ERROR: 6
> DRIVER_INVALID: 4
> DRIVER_TIMEOUT: 1
> DRIVER_SENSE: 58
>
> Johannes: Whatever happened to your efforts at cleaning all this up? Do
> you have a patch series or a working tree we could use as starting
> point?
>
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr
2019-10-21 1:49 ` zhengbin (A)
@ 2019-10-21 13:06 ` Hannes Reinecke
2019-10-22 1:59 ` zhengbin (A)
0 siblings, 1 reply; 26+ messages in thread
From: Hannes Reinecke @ 2019-10-21 13:06 UTC (permalink / raw)
To: zhengbin (A), Martin K. Petersen
Cc: bvanassche, jejb, linux-scsi, yi.zhang, yanaijie, Johannes Thumshirn
On 10/21/19 3:49 AM, zhengbin (A) wrote:
>
> On 2019/10/18 21:43, Martin K. Petersen wrote:
>> Hannes,
>>
>>> The one thing which I patently don't like is the ambivalence between
>>> DRIVER_SENSE and scsi_sense_valid(). What shall we do if only _one_
>>> of them is set? IE what would be the correct way of action if
>>> DRIVER_SENSE is not set, but we have a valid sense code? Or the other
>>> way around?
>> I agree, it's a mess.
>>
>> (Sorry, zhengbin, you opened a can of worms. This is some of our oldest
>> and most arcane code in SCSI)
>>
>>> But more important, from a quick glance not all drivers set the
>>> DRIVER_SENSE bit; so for things like hpsa or smartpqi the sense code is
>>> never evaluated after this patchset.
>> And yet we appear to have several code paths where sense evaluation is
>> contingent on DRIVER_SENSE. So no matter what, behavior might
>> change if we enforce consistent semantics. *sigh*
>
> So what should we do to prevent unit-value access of sshdr?
>
Where do you see it?
From my reading, __scsi_execute() is clearing sshdr by way of
__scsi_execute()
-> scsi_normalize_sense()
-> memset(sshdr)
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@suse.de +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr
2019-10-21 13:06 ` Hannes Reinecke
@ 2019-10-22 1:59 ` zhengbin (A)
2019-10-23 6:51 ` zhengbin (A)
0 siblings, 1 reply; 26+ messages in thread
From: zhengbin (A) @ 2019-10-22 1:59 UTC (permalink / raw)
To: Hannes Reinecke, Martin K. Petersen
Cc: bvanassche, jejb, linux-scsi, yi.zhang, yanaijie, Johannes Thumshirn
On 2019/10/21 21:06, Hannes Reinecke wrote:
> On 10/21/19 3:49 AM, zhengbin (A) wrote:
>> On 2019/10/18 21:43, Martin K. Petersen wrote:
>>> Hannes,
>>>
>>>> The one thing which I patently don't like is the ambivalence between
>>>> DRIVER_SENSE and scsi_sense_valid(). What shall we do if only _one_
>>>> of them is set? IE what would be the correct way of action if
>>>> DRIVER_SENSE is not set, but we have a valid sense code? Or the other
>>>> way around?
>>> I agree, it's a mess.
>>>
>>> (Sorry, zhengbin, you opened a can of worms. This is some of our oldest
>>> and most arcane code in SCSI)
>>>
>>>> But more important, from a quick glance not all drivers set the
>>>> DRIVER_SENSE bit; so for things like hpsa or smartpqi the sense code is
>>>> never evaluated after this patchset.
>>> And yet we appear to have several code paths where sense evaluation is
>>> contingent on DRIVER_SENSE. So no matter what, behavior might
>>> change if we enforce consistent semantics. *sigh*
>> So what should we do to prevent unit-value access of sshdr?
>>
> Where do you see it?
> >From my reading, __scsi_execute() is clearing sshdr by way of
>
> __scsi_execute()
> -> scsi_normalize_sense()
> -> memset(sshdr)
__scsi_execute
req = blk_get_request(sdev->request_queue,
data_direction == DMA_TO_DEVICE ?
REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT);
if (IS_ERR(req))
return ret; -->just return
rq = scsi_req(req);
if (bufflen && blk_rq_map_kern(sdev->request_queue, req,
buffer, bufflen, GFP_NOIO))
goto out; -->just goto out
>
> Cheers,
>
> Hannes
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr
2019-10-22 1:59 ` zhengbin (A)
@ 2019-10-23 6:51 ` zhengbin (A)
0 siblings, 0 replies; 26+ messages in thread
From: zhengbin (A) @ 2019-10-23 6:51 UTC (permalink / raw)
To: Hannes Reinecke, Martin K. Petersen
Cc: bvanassche, jejb, linux-scsi, yi.zhang, yanaijie, Johannes Thumshirn
On 2019/10/22 9:59, zhengbin (A) wrote:
> On 2019/10/21 21:06, Hannes Reinecke wrote:
>> On 10/21/19 3:49 AM, zhengbin (A) wrote:
>>> On 2019/10/18 21:43, Martin K. Petersen wrote:
>>>> Hannes,
>>>>
>>>>> The one thing which I patently don't like is the ambivalence between
>>>>> DRIVER_SENSE and scsi_sense_valid(). What shall we do if only _one_
>>>>> of them is set? IE what would be the correct way of action if
>>>>> DRIVER_SENSE is not set, but we have a valid sense code? Or the other
>>>>> way around?
>>>> I agree, it's a mess.
>>>>
>>>> (Sorry, zhengbin, you opened a can of worms. This is some of our oldest
>>>> and most arcane code in SCSI)
>>>>
>>>>> But more important, from a quick glance not all drivers set the
>>>>> DRIVER_SENSE bit; so for things like hpsa or smartpqi the sense code is
>>>>> never evaluated after this patchset.
>>>> And yet we appear to have several code paths where sense evaluation is
>>>> contingent on DRIVER_SENSE. So no matter what, behavior might
>>>> change if we enforce consistent semantics. *sigh*
>>> So what should we do to prevent unit-value access of sshdr?
>>>
>> Where do you see it?
>> >From my reading, __scsi_execute() is clearing sshdr by way of
>>
>> __scsi_execute()
>> -> scsi_normalize_sense()
>> -> memset(sshdr)
> __scsi_execute
>
> req = blk_get_request(sdev->request_queue,
> data_direction == DMA_TO_DEVICE ?
> REQ_OP_SCSI_OUT : REQ_OP_SCSI_IN, BLK_MQ_REQ_PREEMPT);
> if (IS_ERR(req))
> return ret; -->just return
> rq = scsi_req(req);
>
> if (bufflen && blk_rq_map_kern(sdev->request_queue, req,
> buffer, bufflen, GFP_NOIO))
> goto out; -->just goto out
may be we should init sshdr in __scsi_execute? which is the simplest way, and do not lose anyone.
If we init sshdr in the callers, maybe we will lose some function.
+ /*
+ * Zero-initialize sshdr for those callers that check the *sshdr
+ * contents even if no sense data is available.
+ */
+ if (sshdr)
+ memset(sshdr, 0, sizeof(struct scsi_sense_hdr));
+
>
>> Cheers,
>>
>> Hannes
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr
2019-10-18 13:43 ` Martin K. Petersen
` (2 preceding siblings ...)
2019-10-21 1:49 ` zhengbin (A)
@ 2019-10-21 6:57 ` Johannes Thumshirn
3 siblings, 0 replies; 26+ messages in thread
From: Johannes Thumshirn @ 2019-10-21 6:57 UTC (permalink / raw)
To: Martin K. Petersen, Hannes Reinecke
Cc: zhengbin, bvanassche, jejb, linux-scsi, yi.zhang, yanaijie
On 18/10/2019 15:43, Martin K. Petersen wrote:
[...]
> Johannes: Whatever happened to your efforts at cleaning all this up? Do
> you have a patch series or a working tree we could use as starting
> point?
I'll have to look. I think it's still floating around in some git tree.
Let me search for it.
Johannes
--
Johannes Thumshirn SUSE Labs Filesystems
jthumshirn@suse.de +49 911 74053 689
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5
90409 Nürnberg
Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr
2019-10-18 8:24 [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr zhengbin
` (13 preceding siblings ...)
2019-10-18 9:41 ` [PATCH v5 00/13] scsi: core: fix uninit-value access of variable sshdr Hannes Reinecke
@ 2019-10-18 12:33 ` Damien Le Moal
14 siblings, 0 replies; 26+ messages in thread
From: Damien Le Moal @ 2019-10-18 12:33 UTC (permalink / raw)
To: zhengbin, bvanassche, jejb, martin.petersen, linux-scsi
Cc: yi.zhang, yanaijie
On 2019/10/18 17:17, zhengbin wrote:
> v1->v2: modify the comments suggested by Bart
> v2->v3: fix bug in sr_do_ioctl
> v3->v4: let "fix bug in sr_do_ioctl" be a separate patch
> v4->v5: fix uninit-value access bug in callers, not in __scsi_execute
An explanation text of the series would be great...
What about defining a little helper function for checking
driver_byte(err) == DRIVER_SENSE && scsi_sense_valid(&sshdr)
instead of having that hard-coded everywhere ? That would make the code
a lot cleaner and more readable.
Something like:
static inline bool scsi_driver_sense_valid(int result,
struct scsi_sense_hdr *sshdr)
{
return driver_byte(result) == DRIVER_SENSE &&
scsi_sense_valid(sshdr);
}
>
> zhengbin (13):
> scsi: core: need to check the result of scsi_execute in
> scsi_report_opcode
> scsi: core: need to check the result of scsi_execute in
> scsi_test_unit_ready
> scsi: core: need to check the result of scsi_execute in
> scsi_report_lun_scan
> scsi: sr: need to check the result of scsi_execute in sr_get_events
> scsi: sr: need to check the result of scsi_execute in sr_do_ioctl
> scsi: scsi_dh_emc: need to check the result of scsi_execute in
> send_trespass_cmd
> scsi: scsi_dh_rdac: need to check the result of scsi_execute in
> send_mode_select
> scsi: scsi_dh_hp_sw: need to check the result of scsi_execute in
> hp_sw_tur,hp_sw_start_stop
> scsi: scsi_dh_alua: need to check the result of scsi_execute in
> alua_rtpg,alua_stpg
> scsi: scsi_transport_spi: need to check whether sshdr is valid in
> spi_execute
> scsi: cxlflash: need to check whether sshdr is valid in read_cap16
> scsi: ufs: need to check whether sshdr is valid in
> ufshcd_set_dev_pwr_mode
> scsi: ch: need to check whether sshdr is valid in ch_do_scsi
>
> drivers/scsi/ch.c | 6 ++++--
> drivers/scsi/cxlflash/superpipe.c | 2 +-
> drivers/scsi/device_handler/scsi_dh_alua.c | 6 ++++--
> drivers/scsi/device_handler/scsi_dh_emc.c | 3 ++-
> drivers/scsi/device_handler/scsi_dh_hp_sw.c | 6 ++++--
> drivers/scsi/device_handler/scsi_dh_rdac.c | 8 +++++---
> drivers/scsi/scsi.c | 2 +-
> drivers/scsi/scsi_lib.c | 3 +++
> drivers/scsi/scsi_scan.c | 3 ++-
> drivers/scsi/scsi_transport_spi.c | 1 +
> drivers/scsi/sr.c | 3 ++-
> drivers/scsi/sr_ioctl.c | 6 ++++++
> drivers/scsi/ufs/ufshcd.c | 3 ++-
> 13 files changed, 37 insertions(+), 15 deletions(-)
>
> --
> 2.7.4
>
>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 26+ messages in thread