Re: [PATCH] scsi: scsi_dh: Return error pointer in scsi_dh_attached_handler_name

[Martin Wilck <mwilck@suse.com>]

On Fri, 2025-11-21 at 18:48 -0500, Benjamin Marzinski wrote:
> If scsi_dh_attached_handler_name() fails to allocate the handler
> name,
> dm-multipath (its only caller) assumes there is no attached device
> handler, and sets the device up incorrectly. Return an error pointer
> instead, so multipath can distinguish between failure and success
> where
> there is no attached device handler.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  drivers/md/dm-mpath.c  | 8 ++++++++
>  drivers/scsi/scsi_dh.c | 8 +++++---
>  2 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
> index c18358271618..063dc526fe04 100644
> --- a/drivers/md/dm-mpath.c
> +++ b/drivers/md/dm-mpath.c
> @@ -950,6 +950,14 @@ static struct pgpath *parse_path(struct
> dm_arg_set *as, struct path_selector *ps
>  
>  	q = bdev_get_queue(p->path.dev->bdev);
>  	attached_handler_name = scsi_dh_attached_handler_name(q,
> GFP_KERNEL);
> +	if (IS_ERR(attached_handler_name)) {
> +		if (PTR_ERR(attached_handler_name) == -ENODEV)
> +			attached_handler_name = NULL;

What's the point of continuing here if we know that the SCSI device
doesn't exist?

Thanks,
Martin

> +		else {
> +			r = PTR_ERR(attached_handler_name);
> +			goto bad;
> +		}
> +	}
>  	if (attached_handler_name || m->hw_handler_name) {
>  		INIT_DELAYED_WORK(&p->activate_path,
> activate_path_work);
>  		r = setup_scsi_dh(p->path.dev->bdev, m,
> &attached_handler_name, &ti->error);
> diff --git a/drivers/scsi/scsi_dh.c b/drivers/scsi/scsi_dh.c
> index 7b56e00c7df6..b9d805317814 100644
> --- a/drivers/scsi/scsi_dh.c
> +++ b/drivers/scsi/scsi_dh.c
> @@ -353,7 +353,8 @@ EXPORT_SYMBOL_GPL(scsi_dh_attach);
>   *      that may have a device handler attached
>   * @gfp - the GFP mask used in the kmalloc() call when allocating
> memory
>   *
> - * Returns name of attached handler, NULL if no handler is attached.
> + * Returns name of attached handler, NULL if no handler is attached,
> or
> + * and error pointer if an error occurred.
>   * Caller must take care to free the returned string.
>   */
>  const char *scsi_dh_attached_handler_name(struct request_queue *q,
> gfp_t gfp)
> @@ -363,10 +364,11 @@ const char
> *scsi_dh_attached_handler_name(struct request_queue *q, gfp_t gfp)
>  
>  	sdev = scsi_device_from_queue(q);
>  	if (!sdev)
> -		return NULL;
> +		return ERR_PTR(-ENODEV);
>  
>  	if (sdev->handler)
> -		handler_name = kstrdup(sdev->handler->name, gfp);
> +		handler_name = kstrdup(sdev->handler->name, gfp) ? :
> +			       ERR_PTR(-ENOMEM);
>  	put_device(&sdev->sdev_gendev);
>  	return handler_name;
>  }