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 <[email protected]>
> > ---
> > 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;
> > }
