Re: [PATCH v6 22/27] scsi: sd: add multipath disk class

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Empty `dev_release` callback `sd_mpath_disk_release` bypasses device/kobject memory management.
- [Medium] Struct `sd_mpath_disk` is incomplete and fails to manage the multipath gendisk as claimed.
--

--- Patch [22]: [PATCH v6 22/27] scsi: sd: add multipath disk class ---
commit f604e0ae8b45c7be3d202656fafcb45e6c81e7cf
Author: John Garry <[email protected]>

scsi: sd: add multipath disk class

Add a new class, sd_mpath_disk_class, which is the multipath version of
the scsi_disk class. Structure sd_mpath_disk is introduced to manage the
multipath gendisk.

> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 599e75f333343..242a15bc2c5bb 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
[ ... ]
> @@ -113,6 +114,39 @@ static mempool_t *sd_page_pool;
>  static mempool_t *sd_large_page_pool;
>  static atomic_t sd_large_page_pool_users = ATOMIC_INIT(0);
>  static struct lock_class_key sd_bio_compl_lkclass;
> +#ifdef CONFIG_SCSI_MULTIPATH
> +struct sd_mpath_disk {
> +	struct scsi_mpath_head		*scsi_mpath_head;
> +};

[Severity: Medium]
The commit message states that sd_mpath_disk is introduced to manage the
multipath gendisk. However, does this struct lack a struct gendisk pointer
and an embedded struct device? Without these, it doesn't appear capable of
fulfilling its documented purpose of managing the multipath disk.

> +
> +static void sd_mpath_disk_release(struct device *dev)
> +{
> +}

[Severity: High]
Is it safe to provide an empty dev_release callback here?

When a device is assigned to sd_mpath_disk_class and its reference count
reaches zero, this callback is invoked. Providing an empty release callback
circumvents proper resource cleanup, silencing kobject warnings while
guaranteeing that the enclosing structure is never actually freed.

Could this cause a memory leak of the device structure during teardown?

> +
> +static const struct class sd_mpath_disk_class = {
> +	.name = "scsi_mpath_disk",
> +	.dev_release = sd_mpath_disk_release,
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=22