* [PATCH v1 1/4] scsi: sd: fix error handling in sd_probe() after large pool creation failure
2026-06-23 10:01 [PATCH v1 0/4] scsi: sd: fix probe error cleanup, special_vec leak and sd_done() sense gate Yang Xiuwei
@ 2026-06-23 10:01 ` Yang Xiuwei
2026-06-26 21:52 ` Damien Le Moal
2026-06-23 10:01 ` [PATCH v1 2/4] scsi: sd: unify sd_probe() error cleanup through out_put Yang Xiuwei
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Yang Xiuwei @ 2026-06-23 10:01 UTC (permalink / raw)
To: martin.petersen, James.Bottomley
Cc: hare, tom.leiming, p.raghav, dlemoal, sw.prabhu6, linux-scsi,
Yang Xiuwei
After device_add(&sdkp->disk_dev) succeeds, sd_large_pool_create()
failure must unregister disk_dev and let scsi_disk_release() free
sdkp. Going through out_free_index kfree()s an already registered
device and leaks the sysfs entry.
Fixes: 7179e626b76e ("scsi: sd: Enable sector size > PAGE_SIZE in SCSI sd driver")
Signed-off-by: Yang Xiuwei <yangxiuwei@kylinos.cn>
---
drivers/scsi/sd.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 599e75f33334..d18693d390b2 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -4089,7 +4089,9 @@ static int sd_probe(struct scsi_device *sdp)
if (sdp->sector_size > PAGE_SIZE) {
if (sd_large_pool_create()) {
error = -ENOMEM;
- goto out_free_index;
+ device_unregister(&sdkp->disk_dev);
+ put_disk(gd);
+ goto out;
}
}
--
2.25.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v1 1/4] scsi: sd: fix error handling in sd_probe() after large pool creation failure
2026-06-23 10:01 ` [PATCH v1 1/4] scsi: sd: fix error handling in sd_probe() after large pool creation failure Yang Xiuwei
@ 2026-06-26 21:52 ` Damien Le Moal
0 siblings, 0 replies; 13+ messages in thread
From: Damien Le Moal @ 2026-06-26 21:52 UTC (permalink / raw)
To: Yang Xiuwei, martin.petersen, James.Bottomley
Cc: hare, tom.leiming, p.raghav, sw.prabhu6, linux-scsi
On 6/23/26 19:01, Yang Xiuwei wrote:
> After device_add(&sdkp->disk_dev) succeeds, sd_large_pool_create()
> failure must unregister disk_dev and let scsi_disk_release() free
> sdkp. Going through out_free_index kfree()s an already registered
> device and leaks the sysfs entry.
>
> Fixes: 7179e626b76e ("scsi: sd: Enable sector size > PAGE_SIZE in SCSI sd driver")
> Signed-off-by: Yang Xiuwei <yangxiuwei@kylinos.cn>
Looks OK to me.
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v1 2/4] scsi: sd: unify sd_probe() error cleanup through out_put
2026-06-23 10:01 [PATCH v1 0/4] scsi: sd: fix probe error cleanup, special_vec leak and sd_done() sense gate Yang Xiuwei
2026-06-23 10:01 ` [PATCH v1 1/4] scsi: sd: fix error handling in sd_probe() after large pool creation failure Yang Xiuwei
@ 2026-06-23 10:01 ` Yang Xiuwei
2026-06-26 21:54 ` Damien Le Moal
2026-06-23 10:01 ` [PATCH v1 3/4] scsi: sd: fix special_vec mempool leak when scsi_alloc_sgtables() fails Yang Xiuwei
2026-06-23 10:01 ` [PATCH v1 4/4] scsi: sd: fix sd_done() sense handling condition Yang Xiuwei
3 siblings, 1 reply; 13+ messages in thread
From: Yang Xiuwei @ 2026-06-23 10:01 UTC (permalink / raw)
To: martin.petersen, James.Bottomley
Cc: hare, tom.leiming, p.raghav, dlemoal, sw.prabhu6, linux-scsi,
Yang Xiuwei
After put_device() or device_unregister() has released sdkp through
scsi_disk_release(), set sdkp to NULL and fall through to out_put so
put_disk() and kfree() are handled in one place.
Suggested-by: Ming Lei <tom.leiming@gmail.com>
Signed-off-by: Yang Xiuwei <yangxiuwei@kylinos.cn>
---
drivers/scsi/sd.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index d18693d390b2..b096ea237f14 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -4060,8 +4060,8 @@ static int sd_probe(struct scsi_device *sdp)
error = device_add(&sdkp->disk_dev);
if (error) {
put_device(&sdkp->disk_dev);
- put_disk(gd);
- goto out;
+ sdkp = NULL;
+ goto out_put;
}
dev_set_drvdata(dev, sdkp);
@@ -4090,8 +4090,8 @@ static int sd_probe(struct scsi_device *sdp)
if (sd_large_pool_create()) {
error = -ENOMEM;
device_unregister(&sdkp->disk_dev);
- put_disk(gd);
- goto out;
+ sdkp = NULL;
+ goto out_put;
}
}
@@ -4109,11 +4109,11 @@ static int sd_probe(struct scsi_device *sdp)
error = device_add_disk(dev, gd, NULL);
if (error) {
- device_unregister(&sdkp->disk_dev);
- put_disk(gd);
if (sdp->sector_size > PAGE_SIZE)
sd_large_pool_destroy();
- goto out;
+ device_unregister(&sdkp->disk_dev);
+ sdkp = NULL;
+ goto out_put;
}
if (sdkp->security) {
--
2.25.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v1 2/4] scsi: sd: unify sd_probe() error cleanup through out_put
2026-06-23 10:01 ` [PATCH v1 2/4] scsi: sd: unify sd_probe() error cleanup through out_put Yang Xiuwei
@ 2026-06-26 21:54 ` Damien Le Moal
2026-06-29 1:16 ` Yang Xiuwei
0 siblings, 1 reply; 13+ messages in thread
From: Damien Le Moal @ 2026-06-26 21:54 UTC (permalink / raw)
To: Yang Xiuwei, martin.petersen, James.Bottomley
Cc: hare, tom.leiming, p.raghav, sw.prabhu6, linux-scsi
On 6/23/26 19:01, Yang Xiuwei wrote:
> After put_device() or device_unregister() has released sdkp through
> scsi_disk_release(), set sdkp to NULL and fall through to out_put so
> put_disk() and kfree() are handled in one place.
>
> Suggested-by: Ming Lei <tom.leiming@gmail.com>
> Signed-off-by: Yang Xiuwei <yangxiuwei@kylinos.cn>
> ---
> drivers/scsi/sd.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index d18693d390b2..b096ea237f14 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -4060,8 +4060,8 @@ static int sd_probe(struct scsi_device *sdp)
> error = device_add(&sdkp->disk_dev);
> if (error) {
> put_device(&sdkp->disk_dev);
> - put_disk(gd);
> - goto out;
> + sdkp = NULL;
> + goto out_put;
> }
>
> dev_set_drvdata(dev, sdkp);
> @@ -4090,8 +4090,8 @@ static int sd_probe(struct scsi_device *sdp)
> if (sd_large_pool_create()) {
> error = -ENOMEM;
> device_unregister(&sdkp->disk_dev);
> - put_disk(gd);
> - goto out;
> + sdkp = NULL;
> + goto out_put;
device_unregister() is called here and in the next error path too. So what about
a "goto out_unregister;" to avoid repeating this pattern ?
> }
> }
>
> @@ -4109,11 +4109,11 @@ static int sd_probe(struct scsi_device *sdp)
>
> error = device_add_disk(dev, gd, NULL);
> if (error) {
> - device_unregister(&sdkp->disk_dev);
> - put_disk(gd);
> if (sdp->sector_size > PAGE_SIZE)
> sd_large_pool_destroy();
> - goto out;
> + device_unregister(&sdkp->disk_dev);
> + sdkp = NULL;
> + goto out_put;
> }
>
> if (sdkp->security) {
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v1 2/4] scsi: sd: unify sd_probe() error cleanup through out_put
2026-06-26 21:54 ` Damien Le Moal
@ 2026-06-29 1:16 ` Yang Xiuwei
2026-06-29 5:03 ` Damien Le Moal
0 siblings, 1 reply; 13+ messages in thread
From: Yang Xiuwei @ 2026-06-29 1:16 UTC (permalink / raw)
To: dlemoal
Cc: James.Bottomley, hare, linux-scsi, martin.petersen, p.raghav,
sw.prabhu6, tom.leiming
---
Hi Damien,
On Sat, Jun 27, 2026 at 06:54:44AM +0900, Damien Le Moal wrote:
> device_unregister() is called here and in the next error path too. So what about
> a "goto out_unregister;" to avoid repeating this pattern ?
I tried that too, but out_unregister cannot fall through to out_free_index:
scsi_disk_release() already calls ida_free(). So it needs a second
goto out_put after unregister, which is why v1 kept the explicit paths.
If that out_unregister + goto out_put pattern is fine here, I will use
it in v2.
Thanks,
Yang Xiuwei
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v1 2/4] scsi: sd: unify sd_probe() error cleanup through out_put
2026-06-29 1:16 ` Yang Xiuwei
@ 2026-06-29 5:03 ` Damien Le Moal
0 siblings, 0 replies; 13+ messages in thread
From: Damien Le Moal @ 2026-06-29 5:03 UTC (permalink / raw)
To: Yang Xiuwei
Cc: James.Bottomley, hare, linux-scsi, martin.petersen, p.raghav,
sw.prabhu6, tom.leiming
On 6/29/26 10:16 AM, Yang Xiuwei wrote:
> ---
>
> Hi Damien,
>
> On Sat, Jun 27, 2026 at 06:54:44AM +0900, Damien Le Moal wrote:
>> device_unregister() is called here and in the next error path too. So what about
>> a "goto out_unregister;" to avoid repeating this pattern ?
>
> I tried that too, but out_unregister cannot fall through to out_free_index:
> scsi_disk_release() already calls ida_free(). So it needs a second
> goto out_put after unregister, which is why v1 kept the explicit paths.
Simple solution is best :) If changing the labels makes things more
complicated, then please feel free to ignore my comment.
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v1 3/4] scsi: sd: fix special_vec mempool leak when scsi_alloc_sgtables() fails
2026-06-23 10:01 [PATCH v1 0/4] scsi: sd: fix probe error cleanup, special_vec leak and sd_done() sense gate Yang Xiuwei
2026-06-23 10:01 ` [PATCH v1 1/4] scsi: sd: fix error handling in sd_probe() after large pool creation failure Yang Xiuwei
2026-06-23 10:01 ` [PATCH v1 2/4] scsi: sd: unify sd_probe() error cleanup through out_put Yang Xiuwei
@ 2026-06-23 10:01 ` Yang Xiuwei
2026-06-23 10:20 ` sashiko-bot
2026-06-26 21:57 ` Damien Le Moal
2026-06-23 10:01 ` [PATCH v1 4/4] scsi: sd: fix sd_done() sense handling condition Yang Xiuwei
3 siblings, 2 replies; 13+ messages in thread
From: Yang Xiuwei @ 2026-06-23 10:01 UTC (permalink / raw)
To: martin.petersen, James.Bottomley
Cc: hare, tom.leiming, p.raghav, dlemoal, sw.prabhu6, linux-scsi,
Yang Xiuwei
sd_set_special_bvec() allocates a special payload page for UNMAP and
WRITE SAME commands. If scsi_alloc_sgtables() fails afterward in
sd_setup_unmap_cmnd() or sd_setup_write_same{10,16}_cmnd(), the SCSI
midlayer does not call uninit_command() because RQF_DONTPREP is not
set yet, leaking the page.
Call sd_uninit_command() on error, and clear RQF_SPECIAL_PAYLOAD after
freeing the page.
Signed-off-by: Yang Xiuwei <yangxiuwei@kylinos.cn>
---
drivers/scsi/sd.c | 47 ++++++++++++++++++++++++++++++-----------------
1 file changed, 30 insertions(+), 17 deletions(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index b096ea237f14..6f05e7683df6 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -958,6 +958,21 @@ static unsigned char sd_setup_protect_cmnd(struct scsi_cmnd *scmd,
return protect;
}
+static void sd_uninit_command(struct scsi_cmnd *cmd)
+{
+ struct request *rq = scsi_cmd_to_rq(cmd);
+ struct scsi_device *sdp = cmd->device;
+
+ if (!(rq->rq_flags & RQF_SPECIAL_PAYLOAD))
+ return;
+
+ if (sdp->sector_size > PAGE_SIZE)
+ mempool_free(rq->special_vec.bv_page, sd_large_page_pool);
+ else
+ mempool_free(rq->special_vec.bv_page, sd_page_pool);
+ rq->rq_flags &= ~RQF_SPECIAL_PAYLOAD;
+}
+
static void *sd_set_special_bvec(struct scsi_cmnd *cmd, unsigned int data_len)
{
struct page *page;
@@ -990,6 +1005,7 @@ static blk_status_t sd_setup_unmap_cmnd(struct scsi_cmnd *cmd)
u32 nr_blocks = sectors_to_logical(sdp, blk_rq_sectors(rq));
unsigned int data_len = 24;
char *buf;
+ blk_status_t ret;
buf = sd_set_special_bvec(cmd, data_len);
if (!buf)
@@ -1008,7 +1024,10 @@ static blk_status_t sd_setup_unmap_cmnd(struct scsi_cmnd *cmd)
cmd->transfersize = data_len;
rq->timeout = SD_TIMEOUT;
- return scsi_alloc_sgtables(cmd);
+ ret = scsi_alloc_sgtables(cmd);
+ if (ret != BLK_STS_OK)
+ sd_uninit_command(cmd);
+ return ret;
}
static void sd_config_atomic(struct scsi_disk *sdkp, struct queue_limits *lim)
@@ -1079,6 +1098,7 @@ static blk_status_t sd_setup_write_same16_cmnd(struct scsi_cmnd *cmd,
u64 lba = sectors_to_logical(sdp, blk_rq_pos(rq));
u32 nr_blocks = sectors_to_logical(sdp, blk_rq_sectors(rq));
u32 data_len = sdp->sector_size;
+ blk_status_t ret;
if (!sd_set_special_bvec(cmd, data_len))
return BLK_STS_RESOURCE;
@@ -1094,7 +1114,10 @@ static blk_status_t sd_setup_write_same16_cmnd(struct scsi_cmnd *cmd,
cmd->transfersize = data_len;
rq->timeout = unmap ? SD_TIMEOUT : SD_WRITE_SAME_TIMEOUT;
- return scsi_alloc_sgtables(cmd);
+ ret = scsi_alloc_sgtables(cmd);
+ if (ret != BLK_STS_OK)
+ sd_uninit_command(cmd);
+ return ret;
}
static blk_status_t sd_setup_write_same10_cmnd(struct scsi_cmnd *cmd,
@@ -1106,6 +1129,7 @@ static blk_status_t sd_setup_write_same10_cmnd(struct scsi_cmnd *cmd,
u64 lba = sectors_to_logical(sdp, blk_rq_pos(rq));
u32 nr_blocks = sectors_to_logical(sdp, blk_rq_sectors(rq));
u32 data_len = sdp->sector_size;
+ blk_status_t ret;
if (!sd_set_special_bvec(cmd, data_len))
return BLK_STS_RESOURCE;
@@ -1121,7 +1145,10 @@ static blk_status_t sd_setup_write_same10_cmnd(struct scsi_cmnd *cmd,
cmd->transfersize = data_len;
rq->timeout = unmap ? SD_TIMEOUT : SD_WRITE_SAME_TIMEOUT;
- return scsi_alloc_sgtables(cmd);
+ ret = scsi_alloc_sgtables(cmd);
+ if (ret != BLK_STS_OK)
+ sd_uninit_command(cmd);
+ return ret;
}
static blk_status_t sd_setup_write_zeroes_cmnd(struct scsi_cmnd *cmd)
@@ -1550,20 +1577,6 @@ static blk_status_t sd_init_command(struct scsi_cmnd *cmd)
}
}
-static void sd_uninit_command(struct scsi_cmnd *SCpnt)
-{
- struct request *rq = scsi_cmd_to_rq(SCpnt);
- struct scsi_device *sdp = SCpnt->device;
- unsigned sector_size = sdp->sector_size;
-
- if (rq->rq_flags & RQF_SPECIAL_PAYLOAD) {
- if (sector_size > PAGE_SIZE)
- mempool_free(rq->special_vec.bv_page, sd_large_page_pool);
- else
- mempool_free(rq->special_vec.bv_page, sd_page_pool);
- }
-}
-
static bool sd_need_revalidate(struct gendisk *disk, struct scsi_disk *sdkp)
{
if (sdkp->device->removable || sdkp->write_prot) {
--
2.25.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v1 3/4] scsi: sd: fix special_vec mempool leak when scsi_alloc_sgtables() fails
2026-06-23 10:01 ` [PATCH v1 3/4] scsi: sd: fix special_vec mempool leak when scsi_alloc_sgtables() fails Yang Xiuwei
@ 2026-06-23 10:20 ` sashiko-bot
2026-06-26 21:57 ` Damien Le Moal
1 sibling, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-06-23 10:20 UTC (permalink / raw)
To: Yang Xiuwei; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Concurrent updates to `sdp->sector_size` can cause `sd_uninit_command()` to free a payload page to the wrong mempool.
--
commit ba587f7eceae0f0ec53afb6d48daeb8d25456542
Author: Yang Xiuwei <yangxiuwei@kylinos.cn>
scsi: sd: fix special_vec mempool leak when scsi_alloc_sgtables() fails
sd_set_special_bvec() allocates a special payload page for UNMAP and
WRITE SAME commands. If scsi_alloc_sgtables() fails afterward in
sd_setup_unmap_cmnd() or sd_setup_write_same{10,16}_cmnd(), the SCSI
midlayer does not call uninit_command() because RQF_DONTPREP is not
set yet, leaking the page.
Call sd_uninit_command() on error, and clear RQF_SPECIAL_PAYLOAD after
freeing the page.
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index b096ea237f14a..6f05e7683df6d 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -958,6 +958,21 @@ static unsigned char sd_setup_protect_cmnd(struct scsi_cmnd *scmd,
> return protect;
> }
>
> +static void sd_uninit_command(struct scsi_cmnd *cmd)
> +{
> + struct request *rq = scsi_cmd_to_rq(cmd);
> + struct scsi_device *sdp = cmd->device;
> +
> + if (!(rq->rq_flags & RQF_SPECIAL_PAYLOAD))
> + return;
> +
> + if (sdp->sector_size > PAGE_SIZE)
> + mempool_free(rq->special_vec.bv_page, sd_large_page_pool);
> + else
> + mempool_free(rq->special_vec.bv_page, sd_page_pool);
[Severity: High]
This is a pre-existing issue, but can checking sdp->sector_size here cause a
page to be freed into the wrong mempool if the sector size changes
concurrently?
In sd_set_special_bvec(), the page is allocated based on sdp->sector_size:
if (sector_size > PAGE_SIZE)
page = mempool_alloc(sd_large_page_pool, GFP_ATOMIC);
Concurrently, sd_revalidate_disk() calls sd_read_capacity(), which updates
sdkp->device->sector_size directly before the block layer queue is frozen:
sdkp->device->sector_size = sector_size;
If sdp->sector_size is modified and crosses the PAGE_SIZE boundary while an
UNMAP or WRITE SAME request is in flight, could sd_uninit_command() read the
new size and free the page to the wrong pool? This might be reachable by a
malicious USB device returning a different sector size upon a media change.
Since sd_large_page_pool uses high-order allocations, freeing an order-0 page
into it could corrupt the page allocator and cause subsequent allocations to
write out of bounds.
> + rq->rq_flags &= ~RQF_SPECIAL_PAYLOAD;
> +}
> +
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260623100159.4018066-1-yangxiuwei@kylinos.cn?part=3
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v1 3/4] scsi: sd: fix special_vec mempool leak when scsi_alloc_sgtables() fails
2026-06-23 10:01 ` [PATCH v1 3/4] scsi: sd: fix special_vec mempool leak when scsi_alloc_sgtables() fails Yang Xiuwei
2026-06-23 10:20 ` sashiko-bot
@ 2026-06-26 21:57 ` Damien Le Moal
1 sibling, 0 replies; 13+ messages in thread
From: Damien Le Moal @ 2026-06-26 21:57 UTC (permalink / raw)
To: Yang Xiuwei, martin.petersen, James.Bottomley
Cc: hare, tom.leiming, p.raghav, sw.prabhu6, linux-scsi
On 6/23/26 19:01, Yang Xiuwei wrote:
> sd_set_special_bvec() allocates a special payload page for UNMAP and
> WRITE SAME commands. If scsi_alloc_sgtables() fails afterward in
> sd_setup_unmap_cmnd() or sd_setup_write_same{10,16}_cmnd(), the SCSI
> midlayer does not call uninit_command() because RQF_DONTPREP is not
> set yet, leaking the page.
>
> Call sd_uninit_command() on error, and clear RQF_SPECIAL_PAYLOAD after
> freeing the page.
>
> Signed-off-by: Yang Xiuwei <yangxiuwei@kylinos.cn>
This needs a Fixes tag I think.
But otherwise looks OK to me.
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v1 4/4] scsi: sd: fix sd_done() sense handling condition
2026-06-23 10:01 [PATCH v1 0/4] scsi: sd: fix probe error cleanup, special_vec leak and sd_done() sense gate Yang Xiuwei
` (2 preceding siblings ...)
2026-06-23 10:01 ` [PATCH v1 3/4] scsi: sd: fix special_vec mempool leak when scsi_alloc_sgtables() fails Yang Xiuwei
@ 2026-06-23 10:01 ` Yang Xiuwei
2026-06-23 10:18 ` sashiko-bot
2026-06-26 22:07 ` Damien Le Moal
3 siblings, 2 replies; 13+ messages in thread
From: Yang Xiuwei @ 2026-06-23 10:01 UTC (permalink / raw)
To: martin.petersen, James.Bottomley
Cc: hare, tom.leiming, p.raghav, dlemoal, sw.prabhu6, linux-scsi,
Yang Xiuwei
Commit 464a00c9e0ad ("scsi: core: Kill DRIVER_SENSE") replaced
driver_byte(result) != DRIVER_SENSE with
!scsi_status_is_check_condition(result) but kept the old OR-shaped
gate. That lets CHECK CONDITION with invalid or deferred sense enter
the sense_key switch with an uninitialized or stale sshdr.
Only handle sshdr when CHECK CONDITION is indicated and the sense
data is valid and not deferred.
Fixes: 464a00c9e0ad ("scsi: core: Kill DRIVER_SENSE")
Signed-off-by: Yang Xiuwei <yangxiuwei@kylinos.cn>
---
drivers/scsi/sd.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 6f05e7683df6..110ea4d793f3 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2419,8 +2419,8 @@ static int sd_done(struct scsi_cmnd *SCpnt)
}
sdkp->medium_access_timed_out = 0;
- if (!scsi_status_is_check_condition(result) &&
- (!sense_valid || sense_deferred))
+ if (!scsi_status_is_check_condition(result) ||
+ !sense_valid || sense_deferred)
goto out;
switch (sshdr.sense_key) {
--
2.25.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v1 4/4] scsi: sd: fix sd_done() sense handling condition
2026-06-23 10:01 ` [PATCH v1 4/4] scsi: sd: fix sd_done() sense handling condition Yang Xiuwei
@ 2026-06-23 10:18 ` sashiko-bot
2026-06-26 22:07 ` Damien Le Moal
1 sibling, 0 replies; 13+ messages in thread
From: sashiko-bot @ 2026-06-23 10:18 UTC (permalink / raw)
To: Yang Xiuwei; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch introduces a logic regression by unconditionally bypassing valid sense data processing for any SCSI status other than SAM_STAT_CHECK_CONDITION.
--
commit 6e9e6fa97257d1c3c528406c9719d2273c7f5a65
Author: Yang Xiuwei <yangxiuwei@kylinos.cn>
scsi: sd: fix sd_done() sense handling condition
This commit modifies the conditional in sd_done() to avoid entering the
sense_key switch with an uninitialized or stale sshdr when a CHECK CONDITION
occurs with invalid or deferred sense data. It replaces the previous logic
with an OR condition.
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -2419,8 +2419,8 @@ static int sd_done(struct scsi_cmnd *SCpnt)
> }
> sdkp->medium_access_timed_out = 0;
>
> - if (!scsi_status_is_check_condition(result) &&
> - (!sense_valid || sense_deferred))
> + if (!scsi_status_is_check_condition(result) ||
> + !sense_valid || sense_deferred)
[Severity: High]
Does this logic change inadvertently bypass valid sense data processing for
any SCSI status other than SAM_STAT_CHECK_CONDITION?
For example, if a low-level driver completes an I/O with
SAM_STAT_COMMAND_TERMINATED alongside valid sense data like RECOVERED_ERROR,
!scsi_status_is_check_condition(result) will evaluate to true.
This would cause the code to immediately jump to the 'out' label, skipping
the sense key switch block. Because good_bytes is initialized to 0 for any
non-zero result, the command will incorrectly fail instead of being handled
as fully or partially completed.
Would it be better to solely check if (!sense_valid || sense_deferred) to
ensure sshdr is initialized without discarding valid sense data on other
statuses?
> goto out;
>
> switch (sshdr.sense_key) {
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260623100159.4018066-1-yangxiuwei@kylinos.cn?part=4
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v1 4/4] scsi: sd: fix sd_done() sense handling condition
2026-06-23 10:01 ` [PATCH v1 4/4] scsi: sd: fix sd_done() sense handling condition Yang Xiuwei
2026-06-23 10:18 ` sashiko-bot
@ 2026-06-26 22:07 ` Damien Le Moal
1 sibling, 0 replies; 13+ messages in thread
From: Damien Le Moal @ 2026-06-26 22:07 UTC (permalink / raw)
To: Yang Xiuwei, martin.petersen, James.Bottomley
Cc: hare, tom.leiming, p.raghav, sw.prabhu6, linux-scsi
On 6/23/26 19:01, Yang Xiuwei wrote:
> Commit 464a00c9e0ad ("scsi: core: Kill DRIVER_SENSE") replaced
> driver_byte(result) != DRIVER_SENSE with
> !scsi_status_is_check_condition(result) but kept the old OR-shaped
> gate. That lets CHECK CONDITION with invalid or deferred sense enter
> the sense_key switch with an uninitialized or stale sshdr.
>
> Only handle sshdr when CHECK CONDITION is indicated and the sense
> data is valid and not deferred.
>
> Fixes: 464a00c9e0ad ("scsi: core: Kill DRIVER_SENSE")
> Signed-off-by: Yang Xiuwei <yangxiuwei@kylinos.cn>
I think the fixes tag may be wrong, since the driver_byte(result) !=
DRIVER_SENSE test was actually equivalent to
!scsi_status_is_check_condition(result). So it looks like the problem actually
is even older than 464a00c9e0ad.
But I think this is fine.
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 13+ messages in thread