| Message ID | 20220530014341.115427-3-damien.lemoal@opensource.wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | sd_zbc fixes | expand |
On 30/05/2022 03:43, Damien Le Moal wrote: > Make sure to always clear 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). > > This change also makes sure that the sdkp rev_mutex is never used while > not being initialized by gating sd_zbc_clear_zone_info() cleanup code > with a check on the zone_wp_update_buf field which is never NULL when > rev_mutex has been initialized. > > Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> > --- > drivers/scsi/sd_zbc.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c > index 5b9fad70aa88..6245205b1159 100644 > --- a/drivers/scsi/sd_zbc.c > +++ b/drivers/scsi/sd_zbc.c > @@ -788,6 +788,9 @@ static int sd_zbc_init_disk(struct scsi_disk *sdkp) > > static void sd_zbc_clear_zone_info(struct scsi_disk *sdkp) > { > + if (!sdkp->zone_wp_update_buf) > + return; > + > /* Serialize against revalidate zones */ > mutex_lock(&sdkp->rev_mutex); > > @@ -804,8 +807,7 @@ static void sd_zbc_clear_zone_info(struct scsi_disk *sdkp) > > void sd_zbc_release_disk(struct scsi_disk *sdkp) > { > - if (sd_is_zoned(sdkp)) > - sd_zbc_clear_zone_info(sdkp); > + sd_zbc_clear_zone_info(sdkp); > } Now sd_zbc_release_disk() has become a simple rename of sd_zbc_clear_zone_info(). I think it can go and we can use sd_zbc_clear_zone_info() in the callers instead.
On 5/30/22 16:48, Johannes Thumshirn wrote: > On 30/05/2022 03:43, Damien Le Moal wrote: >> Make sure to always clear 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). >> >> This change also makes sure that the sdkp rev_mutex is never used while >> not being initialized by gating sd_zbc_clear_zone_info() cleanup code >> with a check on the zone_wp_update_buf field which is never NULL when >> rev_mutex has been initialized. >> >> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> >> --- >> drivers/scsi/sd_zbc.c | 15 ++++++++++----- >> 1 file changed, 10 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c >> index 5b9fad70aa88..6245205b1159 100644 >> --- a/drivers/scsi/sd_zbc.c >> +++ b/drivers/scsi/sd_zbc.c >> @@ -788,6 +788,9 @@ static int sd_zbc_init_disk(struct scsi_disk *sdkp) >> >> static void sd_zbc_clear_zone_info(struct scsi_disk *sdkp) >> { >> + if (!sdkp->zone_wp_update_buf) >> + return; >> + >> /* Serialize against revalidate zones */ >> mutex_lock(&sdkp->rev_mutex); >> >> @@ -804,8 +807,7 @@ static void sd_zbc_clear_zone_info(struct scsi_disk *sdkp) >> >> void sd_zbc_release_disk(struct scsi_disk *sdkp) >> { >> - if (sd_is_zoned(sdkp)) >> - sd_zbc_clear_zone_info(sdkp); >> + sd_zbc_clear_zone_info(sdkp); >> } > > Now sd_zbc_release_disk() has become a simple rename of sd_zbc_clear_zone_info(). > I think it can go and we can use sd_zbc_clear_zone_info() in the callers instead. Yes, I thought of that, but I wanted to keep the name to make it clear that the "main" caller is scsi_disk_release(). But if you insist, we can get rid of it :)
diff --git a/drivers/scsi/sd_zbc.c b/drivers/scsi/sd_zbc.c index 5b9fad70aa88..6245205b1159 100644 --- a/drivers/scsi/sd_zbc.c +++ b/drivers/scsi/sd_zbc.c @@ -788,6 +788,9 @@ static int sd_zbc_init_disk(struct scsi_disk *sdkp) static void sd_zbc_clear_zone_info(struct scsi_disk *sdkp) { + if (!sdkp->zone_wp_update_buf) + return; + /* Serialize against revalidate zones */ mutex_lock(&sdkp->rev_mutex); @@ -804,8 +807,7 @@ static void sd_zbc_clear_zone_info(struct scsi_disk *sdkp) void sd_zbc_release_disk(struct scsi_disk *sdkp) { - if (sd_is_zoned(sdkp)) - sd_zbc_clear_zone_info(sdkp); + sd_zbc_clear_zone_info(sdkp); } static void sd_zbc_revalidate_zones_cb(struct gendisk *disk) @@ -914,12 +916,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, clear the disk zone information in + * case the device type changed. */ + sd_zbc_clear_zone_info(sdkp); return 0; + } /* READ16/WRITE16 is mandatory for ZBC disks */ sdkp->device->use_16_for_rw = 1;
Make sure to always clear 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). This change also makes sure that the sdkp rev_mutex is never used while not being initialized by gating sd_zbc_clear_zone_info() cleanup code with a check on the zone_wp_update_buf field which is never NULL when rev_mutex has been initialized. Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> --- drivers/scsi/sd_zbc.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)