| Message ID | 20180917033354.30745-1-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
| Series | dm-mpath: Fix setup_scsi_dh() | expand |
[dropping stable@ cc and cc'ing linux-scsi instead] On Sun, Sep 16 2018 at 11:33pm -0400, Bart Van Assche <bvanassche@acm.org> wrote: > This patch fixes two bugs that got introduced recently in setup_scsi_dh(): > - Avoid that a memory leak occurs if attached_handler_name is not assigned > to m->hw_handler_name. I do see potential for leak, but I'd prefer to fix it with something like the patch at the end of this mail. > - Avoid that m->hw_handler_name becomes a dangling pointer if the > RETAIN_ATTACHED_HW_HANDLER flag is set and scsi_dh_attach() returns > -EBUSY. What is the concern about a dangling pointer? How does that manifest? Stale scsi_dh name stored in hw_handler_name? Pretty sure it gets freed and reassigned as needed (at the start of setup_scsi_dh). > --- > drivers/md/dm-mpath.c | 14 +++++++++----- > include/scsi/scsi_device.h | 9 +++++++++ > 2 files changed, 18 insertions(+), 5 deletions(-) > > diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c > index d94ba6f72ff5..0ba58a537182 100644 > --- a/drivers/md/dm-mpath.c > +++ b/drivers/md/dm-mpath.c > @@ -867,7 +870,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; > + struct scsi_device *sdev; > > /* we need at least a path arg */ > if (as->argc < 1) { > @@ -887,10 +890,11 @@ 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 (attached_handler_name || m->hw_handler_name) { > + sdev = scsi_device_from_queue(q); > + if (sdev) { > + put_device(&sdev->sdev_gendev); > 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, &ti->error); > if (r) { > dm_put_device(ti, p->path.dev); > goto bad; Just because it is a scsi device doesn't mean a scsi_dh needs to be established (though usually that _is_ the case). But bigger concern is I'd _really_ rather avoid dm-mpath instantiating 'struct scsi_device'. scsi_dh_attached_handler_name() provides a more opaque interface. Uncompiled and untested patch to fix leak follows: diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c index d94ba6f72ff5..688ac9e719a7 100644 --- a/drivers/md/dm-mpath.c +++ b/drivers/md/dm-mpath.c @@ -806,14 +806,14 @@ 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) + 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. @@ -830,7 +830,8 @@ static int setup_scsi_dh(struct block_device *bdev, struct multipath *m, * 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; + 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,8 @@ static struct pgpath *parse_path(struct dm_arg_set *as, struct path_selector *ps return p; bad: + if (attached_handler_name) + kfree(attached_handler_name); free_pgpath(p); return ERR_PTR(r); } -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 9/17/18 7:20 AM, Mike Snitzer wrote: >> - Avoid that m->hw_handler_name becomes a dangling pointer if the >> RETAIN_ATTACHED_HW_HANDLER flag is set and scsi_dh_attach() returns >> -EBUSY. > > What is the concern about a dangling pointer? How does that manifest? > Stale scsi_dh name stored in hw_handler_name? Pretty sure it gets freed > and reassigned as needed (at the start of setup_scsi_dh). Hello Mike, Thanks for having taken a look. Before commit e8f74a0f0011, if both MPATHF_RETAIN_ATTACHED_HW_HANDLER and m->hw_handler_name are set before setup_scsi_dh() is called and if scsi_dh_attach() returns -EBUSY, scsi_dh_attached_handler_name() was called twice and allocated memory twice for the handler name. Since commit e8f74a0f0011, in that scenario, the following code related to the handler name is executed: kfree(m->hw_handler_name); m->hw_handler_name = attached_handler_name; [ scsi_dh_attach() returns -EBUSY ] kfree(m->hw_handler_name); m->hw_handler_name = attached_handler_name; I think this sequence makes m->hw_handler_name a dangling pointer. > diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c > index d94ba6f72ff5..688ac9e719a7 100644 > --- a/drivers/md/dm-mpath.c > +++ b/drivers/md/dm-mpath.c > @@ -806,14 +806,14 @@ 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) > + 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. > @@ -830,7 +830,8 @@ static int setup_scsi_dh(struct block_device *bdev, struct multipath *m, > * 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; > + 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,8 @@ static struct pgpath *parse_path(struct dm_arg_set *as, struct path_selector *ps > > return p; > bad: > + if (attached_handler_name) > + kfree(attached_handler_name); > free_pgpath(p); > return ERR_PTR(r); > } Except that the if (attached_handler_name) should be removed from before the kfree() call, the above looks good to me. But since we can avoid changing the type of attached_handler_name from char * into char ** by moving the kfree() call into setup_scsi_dh(), I prefer to avoid to make that change. Thanks, Bart. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Mon, Sep 17 2018 at 10:51am -0400, Bart Van Assche <bvanassche@acm.org> wrote: > On 9/17/18 7:20 AM, Mike Snitzer wrote: > >>- Avoid that m->hw_handler_name becomes a dangling pointer if the > >> RETAIN_ATTACHED_HW_HANDLER flag is set and scsi_dh_attach() returns > >> -EBUSY. > > > >What is the concern about a dangling pointer? How does that manifest? > >Stale scsi_dh name stored in hw_handler_name? Pretty sure it gets freed > >and reassigned as needed (at the start of setup_scsi_dh). > > Hello Mike, > > Thanks for having taken a look. Before commit e8f74a0f0011, if both > MPATHF_RETAIN_ATTACHED_HW_HANDLER and m->hw_handler_name are set > before setup_scsi_dh() is called and if scsi_dh_attach() returns > -EBUSY, scsi_dh_attached_handler_name() was called twice and > allocated memory twice for the handler name. Since commit > e8f74a0f0011, in that scenario, the following code related to the > handler name is executed: > > kfree(m->hw_handler_name); > m->hw_handler_name = attached_handler_name; > [ scsi_dh_attach() returns -EBUSY ] > kfree(m->hw_handler_name); > m->hw_handler_name = attached_handler_name; > > I think this sequence makes m->hw_handler_name a dangling pointer. Ah I see. My patch happened to fix that up by resetting attached_handler_name to NULL and also checking it for NULL during 'goto retain'. > >diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c > >index d94ba6f72ff5..688ac9e719a7 100644 > >--- a/drivers/md/dm-mpath.c > >+++ b/drivers/md/dm-mpath.c > >@@ -806,14 +806,14 @@ 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) > >+ 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. > >@@ -830,7 +830,8 @@ static int setup_scsi_dh(struct block_device *bdev, struct multipath *m, > > * 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; > >+ 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,8 @@ static struct pgpath *parse_path(struct dm_arg_set *as, struct path_selector *ps > > return p; > > bad: > >+ if (attached_handler_name) > >+ kfree(attached_handler_name); > > free_pgpath(p); > > return ERR_PTR(r); > > } > > Except that the if (attached_handler_name) should be removed from > before the kfree() call, the above looks good to me. Yeah, sure, since kfree() checks for NULL. > But since we can avoid changing the type of attached_handler_name from > char * into char ** by moving the kfree() call into setup_scsi_dh(), I > prefer to avoid to make that change. Moving kfree() into setup_scsi_dh() would require use of a common goto for cleanup (something parse_path() already has with 'goto bad;'). But having kfree() in parse_path() is cleaner/symmetric since call to scsi_dh_attached_handler_name() -- and associated memory allocation -- occurs in parse_path(). If you're OK with this, I'll get a proper patch staged based on your header and obviously add a Reported-by attributed to you. Thanks, Mike -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On 9/17/18 8:11 AM, Mike Snitzer wrote: > Moving kfree() into setup_scsi_dh() would require use of a common goto > for cleanup (something parse_path() already has with 'goto bad;'). > > But having kfree() in parse_path() is cleaner/symmetric since call to > scsi_dh_attached_handler_name() -- and associated memory allocation -- > occurs in parse_path(). > > If you're OK with this, I'll get a proper patch staged based on your > header and obviously add a Reported-by attributed to you. That sounds fine to me. Thanks! Bart. -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c index d94ba6f72ff5..0ba58a537182 100644 --- a/drivers/md/dm-mpath.c +++ b/drivers/md/dm-mpath.c @@ -22,6 +22,7 @@ #include <linux/time.h> #include <linux/workqueue.h> #include <linux/delay.h> +#include <scsi/scsi_device.h> #include <scsi/scsi_dh.h> #include <linux/atomic.h> #include <linux/blk-mq.h> @@ -806,13 +807,15 @@ 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) + char **error) { struct request_queue *q = bdev_get_queue(bdev); + const char *attached_handler_name; int r; if (test_bit(MPATHF_RETAIN_ATTACHED_HW_HANDLER, &m->flags)) { retain: + attached_handler_name = scsi_dh_attached_handler_name(q, GFP_KERNEL); if (attached_handler_name) { /* * Clear any hw_handler_params associated with a @@ -867,7 +870,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; + struct scsi_device *sdev; /* we need at least a path arg */ if (as->argc < 1) { @@ -887,10 +890,11 @@ 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 (attached_handler_name || m->hw_handler_name) { + sdev = scsi_device_from_queue(q); + if (sdev) { + put_device(&sdev->sdev_gendev); 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, &ti->error); if (r) { dm_put_device(ti, p->path.dev); goto bad; diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index 202f4d6a4342..1c839089c711 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -2,6 +2,7 @@ #ifndef _SCSI_SCSI_DEVICE_H #define _SCSI_SCSI_DEVICE_H +#include <linux/kconfig.h> #include <linux/list.h> #include <linux/spinlock.h> #include <linux/workqueue.h> @@ -335,7 +336,15 @@ extern void scsi_remove_device(struct scsi_device *); extern int scsi_unregister_device_handler(struct scsi_device_handler *scsi_dh); void scsi_attach_vpd(struct scsi_device *sdev); +#if IS_ENABLED(CONFIG_SCSI) extern struct scsi_device *scsi_device_from_queue(struct request_queue *q); +#else +static inline struct scsi_device * +scsi_device_from_queue(struct request_queue *q) +{ + return NULL; +} +#endif extern int __must_check scsi_device_get(struct scsi_device *); extern void scsi_device_put(struct scsi_device *); extern struct scsi_device *scsi_device_lookup(struct Scsi_Host *,
This patch fixes two bugs that got introduced recently in setup_scsi_dh(): - Avoid that a memory leak occurs if attached_handler_name is not assigned to m->hw_handler_name. - Avoid that m->hw_handler_name becomes a dangling pointer if the RETAIN_ATTACHED_HW_HANDLER flag is set and scsi_dh_attach() returns -EBUSY. Fixes: e8f74a0f0011 ("dm mpath: eliminate need to use scsi_device_from_queue") Signed-off-by: Bart Van Assche <bvanassche@acm.org> Cc: Martin K. Petersen <martin.petersen@oracle.com> Cc: <stable@vger.kernel.org> --- drivers/md/dm-mpath.c | 14 +++++++++----- include/scsi/scsi_device.h | 9 +++++++++ 2 files changed, 18 insertions(+), 5 deletions(-)