* [PATCH v2 0/2] sd_zbc fixes
@ 2022-05-31 0:28 Damien Le Moal
2022-05-31 0:28 ` [PATCH v2 1/2] scsi: sd: Fix potential NULL pointer dereference Damien Le Moal
2022-05-31 0:28 ` [PATCH v2 2/2] scsi: sd_zbc: prevent zone information memory leak Damien Le Moal
0 siblings, 2 replies; 6+ messages in thread
From: Damien Le Moal @ 2022-05-31 0:28 UTC (permalink / raw)
To: linux-scsi, Martin K . Petersen; +Cc: Dongliang Mu
A couple of patches to fix 2 issues with the zbc code:
* A potential NULL pointer dereference in sd_is_zoned(), if that
function is called when sdkp->device is not yet set (e.g. if an error
happen early in sd_probe()).
* Make sure that sdkp zone information memory is never leaked.
Changes from v1:
* Added reviewed-by and tested-by tags in patch 1
* Changed patch 2 to rename sd_zbc_clear_zone_info() to
sd_zbc_free_zone_info() and remove sd_zbc_release_disk().
Damien Le Moal (2):
scsi: sd: Fix potential NULL pointer dereference
scsi: sd_zbc: prevent zone information memory leak
drivers/scsi/sd.c | 4 ++--
drivers/scsi/sd.h | 7 ++++---
drivers/scsi/sd_zbc.c | 26 +++++++++++++-------------
3 files changed, 19 insertions(+), 18 deletions(-)
--
2.36.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/2] scsi: sd: Fix potential NULL pointer dereference
2022-05-31 0:28 [PATCH v2 0/2] sd_zbc fixes Damien Le Moal
@ 2022-05-31 0:28 ` Damien Le Moal
2022-05-31 8:08 ` Christoph Hellwig
2022-05-31 0:28 ` [PATCH v2 2/2] scsi: sd_zbc: prevent zone information memory leak Damien Le Moal
1 sibling, 1 reply; 6+ messages in thread
From: Damien Le Moal @ 2022-05-31 0:28 UTC (permalink / raw)
To: linux-scsi, Martin K . Petersen; +Cc: Dongliang Mu
If sd_probe() sees an error before sdkp->device is initialized,
sd_zbc_release_disk() is called, which causes a NULL pointer dereference
when sd_is_zoned() is called. Avoid this by also testing if a scsi disk
device pointer is set in sd_is_zoned().
Reported-by: Dongliang Mu <mudongliangabcd@gmail.com>
Fixes: 89d947561077 ("sd: Implement support for ZBC device")
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Tested-by: Dongliang Mu <mudongliangabcd@gmail.com>
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
drivers/scsi/sd.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index 2abad54fd23f..b90b96e8834e 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -236,7 +236,8 @@ static inline void sd_dif_config_host(struct scsi_disk *disk)
static inline int sd_is_zoned(struct scsi_disk *sdkp)
{
- return sdkp->zoned == 1 || sdkp->device->type == TYPE_ZBC;
+ return sdkp->zoned == 1 ||
+ (sdkp->device && sdkp->device->type == TYPE_ZBC);
}
#ifdef CONFIG_BLK_DEV_ZONED
--
2.36.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] scsi: sd_zbc: prevent zone information memory leak
2022-05-31 0:28 [PATCH v2 0/2] sd_zbc fixes Damien Le Moal
2022-05-31 0:28 ` [PATCH v2 1/2] scsi: sd: Fix potential NULL pointer dereference Damien Le Moal
@ 2022-05-31 0:28 ` Damien Le Moal
2022-05-31 7:57 ` Johannes Thumshirn
1 sibling, 1 reply; 6+ messages in thread
From: Damien Le Moal @ 2022-05-31 0:28 UTC (permalink / raw)
To: linux-scsi, Martin K . Petersen; +Cc: Dongliang Mu
Make sure to always free a scsi disk zone information, even for regular
disks. This ensures that there is no memory leak, even in the case of a
zoned disk changing type to a regular disk (e.g. with a reformat using
the FORMAT WITH PRESET command or other vendor proprietary command).
To do this, rename sd_zbc_clear_zone_info() to sd_zbc_free_zone_info()
and remove sd_zbc_release_disk(). A call to sd_zbc_free_zone_info() is
added to sd_zbc_read_zones() for drives for which sd_is_zoned() returns
false. Furthermore, sd_zbc_free_zone_info() code make s sure that the
sdkp rev_mutex is never used while not being initialized by gating the
cleanup code with a a check on the zone_wp_update_buf field as it is
never NULL when rev_mutex has been initialized.
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
drivers/scsi/sd.c | 4 ++--
drivers/scsi/sd.h | 4 ++--
drivers/scsi/sd_zbc.c | 26 +++++++++++++-------------
3 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 749316462075..5cc6cddd25d4 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3542,7 +3542,7 @@ static int sd_probe(struct device *dev)
out_put:
put_disk(gd);
out_free:
- sd_zbc_release_disk(sdkp);
+ sd_zbc_free_zone_info(sdkp);
kfree(sdkp);
out:
scsi_autopm_put_device(sdp);
@@ -3579,7 +3579,7 @@ static void scsi_disk_release(struct device *dev)
struct scsi_disk *sdkp = to_scsi_disk(dev);
ida_free(&sd_index_ida, sdkp->index);
- sd_zbc_release_disk(sdkp);
+ sd_zbc_free_zone_info(sdkp);
put_device(&sdkp->device->sdev_gendev);
free_opal_dev(sdkp->opal_dev);
diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h
index b90b96e8834e..6a8969eae5bc 100644
--- a/drivers/scsi/sd.h
+++ b/drivers/scsi/sd.h
@@ -242,7 +242,7 @@ static inline int sd_is_zoned(struct scsi_disk *sdkp)
#ifdef CONFIG_BLK_DEV_ZONED
-void sd_zbc_release_disk(struct scsi_disk *sdkp);
+void sd_zbc_free_zone_info(struct scsi_disk *sdkp);
int sd_zbc_read_zones(struct scsi_disk *sdkp, u8 buf[SD_BUF_SIZE]);
int sd_zbc_revalidate_zones(struct scsi_disk *sdkp);
blk_status_t sd_zbc_setup_zone_mgmt_cmnd(struct scsi_cmnd *cmd,
@@ -257,7 +257,7 @@ blk_status_t sd_zbc_prepare_zone_append(struct scsi_cmnd *cmd, sector_t *lba,
#else /* CONFIG_BLK_DEV_ZONED */
-static inline void sd_zbc_release_disk(struct scsi_disk *sdkp) {}
+static inline void sd_zbc_free_zone_info(struct scsi_disk *sdkp) {}
static inline int sd_zbc_read_zones(struct scsi_disk *sdkp, u8 buf[SD_BUF_SIZE])
{
diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c
index 5b9fad70aa88..6acc4f406eb8 100644
--- a/drivers/scsi/sd_zbc.c
+++ b/drivers/scsi/sd_zbc.c
@@ -786,8 +786,11 @@ static int sd_zbc_init_disk(struct scsi_disk *sdkp)
return 0;
}
-static void sd_zbc_clear_zone_info(struct scsi_disk *sdkp)
+void sd_zbc_free_zone_info(struct scsi_disk *sdkp)
{
+ if (!sdkp->zone_wp_update_buf)
+ return;
+
/* Serialize against revalidate zones */
mutex_lock(&sdkp->rev_mutex);
@@ -802,12 +805,6 @@ static void sd_zbc_clear_zone_info(struct scsi_disk *sdkp)
mutex_unlock(&sdkp->rev_mutex);
}
-void sd_zbc_release_disk(struct scsi_disk *sdkp)
-{
- if (sd_is_zoned(sdkp))
- sd_zbc_clear_zone_info(sdkp);
-}
-
static void sd_zbc_revalidate_zones_cb(struct gendisk *disk)
{
struct scsi_disk *sdkp = scsi_disk(disk);
@@ -914,12 +911,15 @@ int sd_zbc_read_zones(struct scsi_disk *sdkp, u8 buf[SD_BUF_SIZE])
u32 zone_blocks = 0;
int ret;
- if (!sd_is_zoned(sdkp))
+ if (!sd_is_zoned(sdkp)) {
/*
- * Device managed or normal SCSI disk,
- * no special handling required
+ * Device managed or normal SCSI disk, no special handling
+ * required. Nevertheless, free the disk zone information in
+ * case the device type changed.
*/
+ sd_zbc_free_zone_info(sdkp);
return 0;
+ }
/* READ16/WRITE16 is mandatory for ZBC disks */
sdkp->device->use_16_for_rw = 1;
@@ -928,11 +928,11 @@ int sd_zbc_read_zones(struct scsi_disk *sdkp, u8 buf[SD_BUF_SIZE])
if (!blk_queue_is_zoned(q)) {
/*
* This can happen for a host aware disk with partitions.
- * The block device zone information was already cleared
- * by blk_queue_set_zoned(). Only clear the scsi disk zone
+ * The block device zone model was already cleared by
+ * blk_queue_set_zoned(). Only free the scsi disk zone
* information and exit early.
*/
- sd_zbc_clear_zone_info(sdkp);
+ sd_zbc_free_zone_info(sdkp);
return 0;
}
--
2.36.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] scsi: sd_zbc: prevent zone information memory leak
2022-05-31 0:28 ` [PATCH v2 2/2] scsi: sd_zbc: prevent zone information memory leak Damien Le Moal
@ 2022-05-31 7:57 ` Johannes Thumshirn
0 siblings, 0 replies; 6+ messages in thread
From: Johannes Thumshirn @ 2022-05-31 7:57 UTC (permalink / raw)
To: Damien Le Moal, linux-scsi, Martin K . Petersen; +Cc: Dongliang Mu
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] scsi: sd: Fix potential NULL pointer dereference
2022-05-31 0:28 ` [PATCH v2 1/2] scsi: sd: Fix potential NULL pointer dereference Damien Le Moal
@ 2022-05-31 8:08 ` Christoph Hellwig
2022-05-31 8:39 ` Damien Le Moal
0 siblings, 1 reply; 6+ messages in thread
From: Christoph Hellwig @ 2022-05-31 8:08 UTC (permalink / raw)
To: Damien Le Moal; +Cc: linux-scsi, Martin K . Petersen, Dongliang Mu
On Tue, May 31, 2022 at 09:28:11AM +0900, Damien Le Moal wrote:
> If sd_probe() sees an error before sdkp->device is initialized,
> sd_zbc_release_disk() is called, which causes a NULL pointer dereference
> when sd_is_zoned() is called. Avoid this by also testing if a scsi disk
> device pointer is set in sd_is_zoned().
Wouldn't a fix like the one below make more sense? Until
sd_revalidate_disk and thus sd_zbc_revalidate_zones is called none of
the zone information is filled out, and thus we don't need to clear it.
But at that point we've already initialized the device and thus the
release will handler deal with all the real cleanup:
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 749316462075e..dabdc0eeb3dca 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -3542,7 +3542,6 @@ static int sd_probe(struct device *dev)
out_put:
put_disk(gd);
out_free:
- sd_zbc_release_disk(sdkp);
kfree(sdkp);
out:
scsi_autopm_put_device(sdp);
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] scsi: sd: Fix potential NULL pointer dereference
2022-05-31 8:08 ` Christoph Hellwig
@ 2022-05-31 8:39 ` Damien Le Moal
0 siblings, 0 replies; 6+ messages in thread
From: Damien Le Moal @ 2022-05-31 8:39 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: linux-scsi, Martin K . Petersen, Dongliang Mu
On 5/31/22 17:08, Christoph Hellwig wrote:
> On Tue, May 31, 2022 at 09:28:11AM +0900, Damien Le Moal wrote:
>> If sd_probe() sees an error before sdkp->device is initialized,
>> sd_zbc_release_disk() is called, which causes a NULL pointer dereference
>> when sd_is_zoned() is called. Avoid this by also testing if a scsi disk
>> device pointer is set in sd_is_zoned().
>
> Wouldn't a fix like the one below make more sense? Until
> sd_revalidate_disk and thus sd_zbc_revalidate_zones is called none of
> the zone information is filled out, and thus we don't need to clear it.
Indeed, very good point. Will send a v3 with that instead of the current fix.
>
> But at that point we've already initialized the device and thus the
> release will handler deal with all the real cleanup:
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 749316462075e..dabdc0eeb3dca 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -3542,7 +3542,6 @@ static int sd_probe(struct device *dev)
> out_put:
> put_disk(gd);
> out_free:
> - sd_zbc_release_disk(sdkp);
> kfree(sdkp);
> out:
> scsi_autopm_put_device(sdp);
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-05-31 8:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-31 0:28 [PATCH v2 0/2] sd_zbc fixes Damien Le Moal
2022-05-31 0:28 ` [PATCH v2 1/2] scsi: sd: Fix potential NULL pointer dereference Damien Le Moal
2022-05-31 8:08 ` Christoph Hellwig
2022-05-31 8:39 ` Damien Le Moal
2022-05-31 0:28 ` [PATCH v2 2/2] scsi: sd_zbc: prevent zone information memory leak Damien Le Moal
2022-05-31 7:57 ` Johannes Thumshirn
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).