dm mpath: fix attached_handler_name leak and dangling hw_handler_name pointer

Commit e8f74a0f0011 ("dm mpath: eliminate need to use
scsi_device_from_queue") introduced 2 regressions:
1) memory leak occurs if attached_handler_name is not assigned to
   m->hw_handler_name
2) m->hw_handler_name can become a dangling pointer if the
   RETAIN_ATTACHED_HW_HANDLER flag is set and scsi_dh_attach() returns
   -EBUSY.

Fix both of these by clearing 'attached_handler_name' pointer passed to
setup_scsi_dh() after it is assigned to m->hw_handler_name.  And if
setup_scsi_dh() doesn't consume 'attached_handler_name' parse_path()
will kfree() it.

Fixes: e8f74a0f0011 ("dm mpath: eliminate need to use scsi_device_from_queue")
Cc: stable@vger.kernel.org # 4.16+
Reported-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Mike Snitzer <snitzer@redhat.com>

diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
index d94ba6f..419362c 100644 (file)
--- a/drivers/md/dm-mpath.c
+++ b/drivers/md/dm-mpath.c
@@ -806,19 +806,19 @@ static int parse_path_selector(struct dm_arg_set *as, struct priority_group *pg,
 }
 
 static int setup_scsi_dh(struct block_device *bdev, struct multipath *m,
-                        const char *attached_handler_name, char **error)
+                        const char **attached_handler_name, char **error)
 {
        struct request_queue *q = bdev_get_queue(bdev);
        int r;
 
        if (test_bit(MPATHF_RETAIN_ATTACHED_HW_HANDLER, &m->flags)) {
 retain:
-               if (attached_handler_name) {
+               if (*attached_handler_name) {
                        /*
                         * Clear any hw_handler_params associated with a
                         * handler that isn't already attached.
                         */
-                       if (m->hw_handler_name && strcmp(attached_handler_name, m->hw_handler_name)) {
+                       if (m->hw_handler_name && strcmp(*attached_handler_name, m->hw_handler_name)) {
                                kfree(m->hw_handler_params);
                                m->hw_handler_params = NULL;
                        }
@@ -830,7 +830,8 @@ retain:
                         * handler instead of the original table passed in.
                         */
                        kfree(m->hw_handler_name);
-                       m->hw_handler_name = attached_handler_name;
+                       m->hw_handler_name = *attached_handler_name;
+                       *attached_handler_name = NULL;
                }
        }
 
@@ -867,7 +868,7 @@ static struct pgpath *parse_path(struct dm_arg_set *as, struct path_selector *ps
        struct pgpath *p;
        struct multipath *m = ti->private;
        struct request_queue *q;
-       const char *attached_handler_name;
+       const char *attached_handler_name = NULL;
 
        /* we need at least a path arg */
        if (as->argc < 1) {
@@ -890,7 +891,7 @@ static struct pgpath *parse_path(struct dm_arg_set *as, struct path_selector *ps
        attached_handler_name = scsi_dh_attached_handler_name(q, GFP_KERNEL);
        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);
+               r = setup_scsi_dh(p->path.dev->bdev, m, &attached_handler_name, &ti->error);
                if (r) {
                        dm_put_device(ti, p->path.dev);
                        goto bad;
@@ -905,6 +906,7 @@ static struct pgpath *parse_path(struct dm_arg_set *as, struct path_selector *ps
 
        return p;
  bad:
+       kfree(attached_handler_name);
        free_pgpath(p);
        return ERR_PTR(r);
 }