* [PATCH] scsi: scsi_dh: Return error pointer in scsi_dh_attached_handler_name
@ 2025-11-21 23:48 Benjamin Marzinski
2025-11-27 15:26 ` Martin Wilck
0 siblings, 1 reply; 4+ messages in thread
From: Benjamin Marzinski @ 2025-11-21 23:48 UTC (permalink / raw)
To: James E . J . Bottomley, Martin K . Petersen, Mikulas Patocka,
Mike Snitzer
Cc: linux-scsi, dm-devel, Martin Wilck
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;
+ 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;
}
--
2.50.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] scsi: scsi_dh: Return error pointer in scsi_dh_attached_handler_name
2025-11-21 23:48 [PATCH] scsi: scsi_dh: Return error pointer in scsi_dh_attached_handler_name Benjamin Marzinski
@ 2025-11-27 15:26 ` Martin Wilck
2025-12-02 1:00 ` Benjamin Marzinski
0 siblings, 1 reply; 4+ messages in thread
From: Martin Wilck @ 2025-11-27 15:26 UTC (permalink / raw)
To: Benjamin Marzinski, James E . J . Bottomley, Martin K . Petersen,
Mikulas Patocka, Mike Snitzer
Cc: linux-scsi, dm-devel
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;
> }
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] scsi: scsi_dh: Return error pointer in scsi_dh_attached_handler_name
2025-11-27 15:26 ` Martin Wilck
@ 2025-12-02 1:00 ` Benjamin Marzinski
2025-12-02 10:01 ` Martin Wilck
0 siblings, 1 reply; 4+ messages in thread
From: Benjamin Marzinski @ 2025-12-02 1:00 UTC (permalink / raw)
To: Martin Wilck
Cc: James E . J . Bottomley, Martin K . Petersen, Mikulas Patocka,
Mike Snitzer, linux-scsi, dm-devel
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;
> > }
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] scsi: scsi_dh: Return error pointer in scsi_dh_attached_handler_name
2025-12-02 1:00 ` Benjamin Marzinski
@ 2025-12-02 10:01 ` Martin Wilck
0 siblings, 0 replies; 4+ messages in thread
From: Martin Wilck @ 2025-12-02 10:01 UTC (permalink / raw)
To: Benjamin Marzinski
Cc: James E . J . Bottomley, Martin K . Petersen, Mikulas Patocka,
Mike Snitzer, linux-scsi, dm-devel
On Mon, 2025-12-01 at 20:00 -0500, Benjamin Marzinski wrote:
> 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.
Yes, I think so. We know it makes no sense to call setup_scsi_dh(), wo
we shouldn't try.
Martin
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-12-02 10:01 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-21 23:48 [PATCH] scsi: scsi_dh: Return error pointer in scsi_dh_attached_handler_name Benjamin Marzinski
2025-11-27 15:26 ` Martin Wilck
2025-12-02 1:00 ` Benjamin Marzinski
2025-12-02 10:01 ` Martin Wilck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox