| Message ID | 20220531002812.527368-2-damien.lemoal@opensource.wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | sd_zbc fixes | expand |
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);
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);
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
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(-)