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

[Benjamin Marzinski <bmarzins@redhat.com>]

On Thu, Nov 27, 2025 at 04:26:51PM +0100, Martin Wilck wrote:
> 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?

we just know that it's not a SCSI device, so we clear
attached_handler_name.  I suppose we could add another check to error
out here if m->hw_handler_name is set. But if it is, the setup_scsi_dh()
call just below will fail anyways, so there's not much difference.

But if you think adding that extra check makes things clearer, I'm fine
with that.

-Ben
 
> 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;
> >  }