[dm-devel] dm-mpath: Fix setup_scsi_dh()
Bart Van Assche
bvanassche at acm.org
Mon Sep 17 14:51:50 UTC 2018
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.
More information about the dm-devel
mailing list