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

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

[Benjamin Marzinski]

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, success where
there is no attached device handler, or when the path device is not
a scsi device at all.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---

Changes from v1:
If scsi_dh_attached_handler_name() returns that the path device is not
a scsi device (-ENODEV) but m->hw_hander_name is set, print an error
message and clear it, like the code does when a hardware handler is set
on a bio-based multipath device.

 drivers/md/dm-mpath.c  | 13 +++++++++++++
 drivers/scsi/scsi_dh.c |  8 +++++---
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index c18358271618..65a3e93385c0 100644
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -950,6 +950,19 @@ 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) {
+			if (m->hw_handler_name) {
+				DMERR("hardware handlers are only allowed for scsi devices");
+				kfree(m->hw_handler_name);
+				m->hw_handler_name = NULL;
+			}
+			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

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

[Martin Wilck]

On Fri, 2025-12-05 at 20:00 -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, success where
> there is no attached device handler, or when the path device is not
> a scsi device at all.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

Reviewed-by: Martin Wilck <mwilck@suse.com>

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

[kernel test robot]

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20251206010015.1595225-1-bmarzins@redhat.com>
References: <20251206010015.1595225-1-bmarzins@redhat.com>
TO: Benjamin Marzinski <bmarzins@redhat.com>
TO: "James E . J . Bottomley" <James.Bottomley@HansenPartnership.com>
TO: "Martin K . Petersen" <martin.petersen@oracle.com>
TO: Mikulas Patocka <mpatocka@redhat.com>
TO: Mike Snitzer <snitzer@kernel.org>
CC: linux-scsi@vger.kernel.org
CC: dm-devel@lists.linux.dev
CC: Martin Wilck <mwilck@suse.com>

Hi Benjamin,

kernel test robot noticed the following build warnings:

[auto build test WARNING on jejb-scsi/for-next]
[also build test WARNING on mkp-scsi/for-next device-mapper-dm/for-next linus/master v6.18 next-20251208]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Benjamin-Marzinski/scsi-scsi_dh-Return-error-pointer-in-scsi_dh_attached_handler_name/20251206-090603
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jejb/scsi.git for-next
patch link:    https://lore.kernel.org/r/20251206010015.1595225-1-bmarzins%40redhat.com
patch subject: [PATCH v2] scsi: scsi_dh: Return error pointer in scsi_dh_attached_handler_name
:::::: branch date: 3 days ago
:::::: commit date: 3 days ago
config: i386-randconfig-141-20251208 (https://download.01.org/0day-ci/archive/20251209/202512090433.p58QdjEJ-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202512090433.p58QdjEJ-lkp@intel.com/

smatch warnings:
drivers/md/dm-mpath.c:997 parse_path() warn: passing zero to 'ERR_PTR'

vim +/ERR_PTR +997 drivers/md/dm-mpath.c

848b8aefd44df9 Mike Snitzer        2017-12-10  936  
848b8aefd44df9 Mike Snitzer        2017-12-10  937  static struct pgpath *parse_path(struct dm_arg_set *as, struct path_selector *ps,
848b8aefd44df9 Mike Snitzer        2017-12-10  938  				 struct dm_target *ti)
848b8aefd44df9 Mike Snitzer        2017-12-10  939  {
848b8aefd44df9 Mike Snitzer        2017-12-10  940  	int r;
848b8aefd44df9 Mike Snitzer        2017-12-10  941  	struct pgpath *p;
848b8aefd44df9 Mike Snitzer        2017-12-10  942  	struct multipath *m = ti->private;
e8f74a0f00113d Mike Snitzer        2018-03-12  943  	struct request_queue *q;
b592211c33f745 Mike Snitzer        2018-09-17  944  	const char *attached_handler_name = NULL;
848b8aefd44df9 Mike Snitzer        2017-12-10  945  
848b8aefd44df9 Mike Snitzer        2017-12-10  946  	/* we need at least a path arg */
848b8aefd44df9 Mike Snitzer        2017-12-10  947  	if (as->argc < 1) {
848b8aefd44df9 Mike Snitzer        2017-12-10  948  		ti->error = "no device given";
848b8aefd44df9 Mike Snitzer        2017-12-10  949  		return ERR_PTR(-EINVAL);
848b8aefd44df9 Mike Snitzer        2017-12-10  950  	}
848b8aefd44df9 Mike Snitzer        2017-12-10  951  
848b8aefd44df9 Mike Snitzer        2017-12-10  952  	p = alloc_pgpath();
848b8aefd44df9 Mike Snitzer        2017-12-10  953  	if (!p)
848b8aefd44df9 Mike Snitzer        2017-12-10  954  		return ERR_PTR(-ENOMEM);
848b8aefd44df9 Mike Snitzer        2017-12-10  955  
848b8aefd44df9 Mike Snitzer        2017-12-10  956  	r = dm_get_device(ti, dm_shift_arg(as), dm_table_get_mode(ti->table),
848b8aefd44df9 Mike Snitzer        2017-12-10  957  			  &p->path.dev);
848b8aefd44df9 Mike Snitzer        2017-12-10  958  	if (r) {
848b8aefd44df9 Mike Snitzer        2017-12-10  959  		ti->error = "error getting device";
2bfd2e1337f0d8 Chandra Seetharaman 2009-08-03  960  		goto bad;
2bfd2e1337f0d8 Chandra Seetharaman 2009-08-03  961  	}
848b8aefd44df9 Mike Snitzer        2017-12-10  962  
e8f74a0f00113d Mike Snitzer        2018-03-12  963  	q = bdev_get_queue(p->path.dev->bdev);
e8f74a0f00113d Mike Snitzer        2018-03-12  964  	attached_handler_name = scsi_dh_attached_handler_name(q, GFP_KERNEL);
1d31c0d9c94dbd Benjamin Marzinski  2025-12-05  965  	if (IS_ERR(attached_handler_name)) {
1d31c0d9c94dbd Benjamin Marzinski  2025-12-05  966  		if (PTR_ERR(attached_handler_name) == -ENODEV) {
1d31c0d9c94dbd Benjamin Marzinski  2025-12-05  967  			if (m->hw_handler_name) {
1d31c0d9c94dbd Benjamin Marzinski  2025-12-05  968  				DMERR("hardware handlers are only allowed for scsi devices");
1d31c0d9c94dbd Benjamin Marzinski  2025-12-05  969  				kfree(m->hw_handler_name);
1d31c0d9c94dbd Benjamin Marzinski  2025-12-05  970  				m->hw_handler_name = NULL;
1d31c0d9c94dbd Benjamin Marzinski  2025-12-05  971  			}
1d31c0d9c94dbd Benjamin Marzinski  2025-12-05  972  			attached_handler_name = NULL;
1d31c0d9c94dbd Benjamin Marzinski  2025-12-05  973  		} else {
1d31c0d9c94dbd Benjamin Marzinski  2025-12-05  974  			r = PTR_ERR(attached_handler_name);
1d31c0d9c94dbd Benjamin Marzinski  2025-12-05  975  			goto bad;
1d31c0d9c94dbd Benjamin Marzinski  2025-12-05  976  		}
1d31c0d9c94dbd Benjamin Marzinski  2025-12-05  977  	}
e457edf0b21c87 Mike Snitzer        2018-03-29  978  	if (attached_handler_name || m->hw_handler_name) {
848b8aefd44df9 Mike Snitzer        2017-12-10  979  		INIT_DELAYED_WORK(&p->activate_path, activate_path_work);
b592211c33f745 Mike Snitzer        2018-09-17  980  		r = setup_scsi_dh(p->path.dev->bdev, m, &attached_handler_name, &ti->error);
940bc471780b00 Martin Wilck        2019-04-29  981  		kfree(attached_handler_name);
848b8aefd44df9 Mike Snitzer        2017-12-10  982  		if (r) {
848b8aefd44df9 Mike Snitzer        2017-12-10  983  			dm_put_device(ti, p->path.dev);
848b8aefd44df9 Mike Snitzer        2017-12-10  984  			goto bad;
2bfd2e1337f0d8 Chandra Seetharaman 2009-08-03  985  		}
ae11b1b36da726 Hannes Reinecke     2008-07-17  986  	}
ae11b1b36da726 Hannes Reinecke     2008-07-17  987  
^1da177e4c3f41 Linus Torvalds      2005-04-16  988  	r = ps->type->add_path(ps, &p->path, as->argc, as->argv, &ti->error);
^1da177e4c3f41 Linus Torvalds      2005-04-16  989  	if (r) {
^1da177e4c3f41 Linus Torvalds      2005-04-16  990  		dm_put_device(ti, p->path.dev);
^1da177e4c3f41 Linus Torvalds      2005-04-16  991  		goto bad;
^1da177e4c3f41 Linus Torvalds      2005-04-16  992  	}
^1da177e4c3f41 Linus Torvalds      2005-04-16  993  
^1da177e4c3f41 Linus Torvalds      2005-04-16  994  	return p;
^1da177e4c3f41 Linus Torvalds      2005-04-16  995   bad:
^1da177e4c3f41 Linus Torvalds      2005-04-16  996  	free_pgpath(p);
01460f3520c100 Benjamin Marzinski  2008-10-10 @997  	return ERR_PTR(r);
^1da177e4c3f41 Linus Torvalds      2005-04-16  998  }
^1da177e4c3f41 Linus Torvalds      2005-04-16  999  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Martin K. Petersen]

Benjamin,

> 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, success where
> there is no attached device handler, or when the path device is not a
> scsi device at all.

Applied to 6.19/scsi-staging, thanks!

-- 
Martin K. Petersen