[PATCH v2 0/3] scsi: sd: fix probe error cleanup, special_vec leak and sd_done() sense gate

[PATCH v2 0/3] scsi: sd: fix probe error cleanup, special_vec leak and sd_done() sense gate

[Yang Xiuwei]

This series fixes three resource-handling bugs in drivers/scsi/sd.c:
sd_probe() error cleanup, special_vec mempool leak on prep failure, and
sd_done() sense handling.

Changes in v2:
- Drop patch 2/4 (probe cleanup refactor through out_put).
- Add Fixes tags per review on patches 2/3 and 3/3.
- Reword the commit message of patch 3/3.

Link: https://lore.kernel.org/all/20260623100159.4018066-1-yangxiuwei@kylinos.cn/

Yang Xiuwei (3):
  scsi: sd: fix error handling in sd_probe() after large pool creation
    failure
  scsi: sd: fix special_vec mempool leak when scsi_alloc_sgtables()
    fails
  scsi: sd: fix sd_done() sense handling condition

 drivers/scsi/sd.c | 55 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 35 insertions(+), 20 deletions(-)

-- 
2.25.1

[PATCH v2 1/3] scsi: sd: fix error handling in sd_probe() after large pool creation failure

[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")
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
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

Re: [PATCH v2 1/3] scsi: sd: fix error handling in sd_probe() after large pool creation failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Calling device_unregister() before put_disk() in the error path creates a use-after-free vulnerability during SCSI Error Handling.
--

--- Patch [1]: [PATCH v2 1/3] scsi: sd: fix error handling in sd_probe() after large pool creation failure ---
commit f2e6ec332a4785be7451dad3afaf04d9cb43c90c
Author: Yang Xiuwei <yangxiuwei@kylinos.cn>
scsi: sd: fix error handling in sd_probe() after large pool creation failure

This commit modifies the error handling path in sd_probe() when
sd_large_pool_create() fails. It replaces a goto with direct calls to
device_unregister() and put_disk() to prevent leaking sysfs entries and
freeing an already registered device.

> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 599e75f333343..d18693d390b2f 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);

[Severity: Critical]
Does this ordering create a use-after-free window?

By calling device_unregister() before put_disk(), the sdkp structure is
synchronously freed, but the request queue disk pointer remains set until
put_disk() runs.

If an asynchronous SCSI command times out precisely in this window, could
the SCSI error handler dereference the freed pointer?

drivers/scsi/sd.c:sd_eh_reset() {
        struct scsi_disk *sdkp = scsi_disk(scsi_cmd_to_rq(scmd)->q->disk);

        /* New SCSI EH run, reset gate variable */
        sdkp->ignore_medium_access_errors = false;
        ...
}

Would it be safer to call put_disk() before device_unregister() to clear
the queue disk pointer and prevent this concurrent access?

> +			goto out;
>  		}
>  	}
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707030333.22245-1-yangxiuwei@kylinos.cn?part=1

Re: [PATCH v2 1/3] scsi: sd: fix error handling in sd_probe() after large pool creation failure

[John Garry]

On 07/07/2026 04:03, 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")
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

This is the same as I sent:

Reviewed-by: John Garry <john.g.garry@oracle.com>

However, would it be simpler to always create this pool for LBS enabled 
(and not just when we probe some disk which has sector size > PAGE_SIZE)?

We only get LBS when we have THP, so not always. And we would waste 2x 
64K pages. But at least the code would be simpler.

On another topic, sd_large_page_pool_users does not need to be atomic as 
it is always read/written under the mutex (so can be a regular int).


> 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;
>   		}
>   	}
>   

Re: [PATCH v2 1/3] scsi: sd: fix error handling in sd_probe() after large pool creation failure

[Yang Xiuwei]

Hi John,

On Wed, 08 Jul 2026, John Garry wrote:
> However, would it be simpler to always create this pool for LBS enabled
> (and not just when we probe some disk which has sector size > PAGE_SIZE)?

Yeah — same idea as sd_page_pool at init. sd_probe already has quite a
few error paths; I had a go at consolidating them in v1 but dropped it.
Moving the pool to init seems like a cleaner approach.

> On another topic, sd_large_page_pool_users does not need to be atomic as
> it is always read/written under the mutex (so can be a regular int).

Agreed on the atomic; an init-time pool would drop the users counter as
well.

Thanks,
Yang Xiuwei

Re: [PATCH v2 1/3] scsi: sd: fix error handling in sd_probe() after large pool creation failure

[John Garry]

On 7/8/26 08:22, Yang Xiuwei wrote:
> On Wed, 08 Jul 2026, John Garry wrote:
>> However, would it be simpler to always create this pool for LBS enabled
>> (and not just when we probe some disk which has sector size > PAGE_SIZE)?
> Yeah — same idea as sd_page_pool at init. sd_probe already has quite a
> few error paths; I had a go at consolidating them in v1 but dropped it.
> Moving the pool to init seems like a cleaner approach.

Sure, but - as I said - it may waste memory if we (likely) have no disks 
with sector size > page size. Maybe it's better as is (to alloc in probe 
path).

[PATCH v2 2/3] scsi: sd: fix special_vec mempool leak when scsi_alloc_sgtables() fails

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

Fixes: 81d926e8b552 ("sd: split sd_setup_discard_cmnd")
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
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 d18693d390b2..8fed1cda9ac8 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

Re: [PATCH v2 2/3] scsi: sd: fix special_vec mempool leak when scsi_alloc_sgtables() fails

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The special payload page can be freed into the wrong mempool if the device sector size changes while the command is in flight.
--

--- Patch [2]: [PATCH v2 2/3] scsi: sd: fix special_vec mempool leak when scsi_alloc_sgtables() fails ---
commit 39421d4b78837e533a8958996d1edabedfa78d22
Author: Yang Xiuwei <yangxiuwei@kylinos.cn>

scsi: sd: fix special_vec mempool leak when scsi_alloc_sgtables() fails

This commit fixes a mempool leak when scsi_alloc_sgtables() fails in
sd_setup_unmap_cmnd() or sd_setup_write_same{10,16}_cmnd(). It ensures
sd_uninit_command() is called on error, clearing RQF_SPECIAL_PAYLOAD
and freeing the payload page.

> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index d18693d390b2f..8fed1cda9ac8c 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 isn't a bug introduced by this patch, but can this free the page to the
wrong mempool if the device sector size changes while the command is in flight?

sd_set_special_bvec() allocates the page based on the sector size at the time
of submission:

    if (sector_size > PAGE_SIZE)
        page = mempool_alloc(sd_large_page_pool, GFP_ATOMIC);
    else
        page = mempool_alloc(sd_page_pool, GFP_ATOMIC);

If sd_read_capacity() updates sdp->sector_size dynamically during a concurrent
rescan before the request queue is frozen, this cleanup path might read the
updated sector size. This could result in freeing an order-0 page into the
large page pool or vice versa, causing memory corruption upon subsequent
allocations from the pool.

> +	rq->rq_flags &= ~RQF_SPECIAL_PAYLOAD;
> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707030333.22245-1-yangxiuwei@kylinos.cn?part=2

Re: [PATCH v2 2/3] scsi: sd: fix special_vec mempool leak when scsi_alloc_sgtables() fails

[John Garry]

On 07/07/2026 04:03, 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.
> 
> Fixes: 81d926e8b552 ("sd: split sd_setup_discard_cmnd")
> Reviewed-by: Damien Le Moal<dlemoal@kernel.org>
> Signed-off-by: Yang Xiuwei<yangxiuwei@kylinos.cn>

Reviewed-by: John Garry <john.g.garry@oracle.com>

[PATCH v2 3/3] scsi: sd: fix sd_done() sense handling condition

[Yang Xiuwei]

Only enter the sense_key switch when the command returned CHECK
CONDITION with valid, non-deferred sense. The old condition let
deferred or invalid sense fall through and mis-handle the I/O.

Fixes: 03aba2f79594 ("[SCSI] sd/scsi_lib simplify sd_rw_intr and scsi_io_completion")
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
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 8fed1cda9ac8..a1b21ea14e54 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

Re: [PATCH v2 3/3] scsi: sd: fix sd_done() sense handling condition

[Bart Van Assche]

On 7/6/26 8:03 PM, Yang Xiuwei wrote:
> Only enter the sense_key switch when the command returned CHECK
> CONDITION with valid, non-deferred sense. The old condition let
> deferred or invalid sense fall through and mis-handle the I/O.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>