Message ID | 1462962941-9429-1-git-send-email-hare@suse.de (mailing list archive) |
---|---|
State | Not Applicable, archived |
Delegated to: | Mike Snitzer |
Headers | show |
On Wed, May 11, 2016 at 12:35:41PM +0200, Hannes Reinecke wrote: > Ben Marzinski pointed out that filter_devnode() is used > without any lock or configuration settings in uev_trigger(), > and hence might be invalid when processing events during > reconfiguration. > So move it into the individual functions and handle it > with the vector lock held. ACK -Ben > > Signed-off-by: Hannes Reinecke <hare@suse.com> > --- > multipathd/main.c | 45 +++++++++++++++++++++++++++------------------ > 1 file changed, 27 insertions(+), 18 deletions(-) > > diff --git a/multipathd/main.c b/multipathd/main.c > index 58e8854..2c7486d 100644 > --- a/multipathd/main.c > +++ b/multipathd/main.c > @@ -580,6 +580,11 @@ uev_add_path (struct uevent *uev, struct vectors * vecs) > pthread_cleanup_push(cleanup_lock, &vecs->lock); > lock(vecs->lock); > pthread_testcancel(); > + if (filter_devnode(conf->blist_devnode, conf->elist_devnode, > + uev->kernel) > 0) { > + ret = 0; > + goto out_unlock; > + } > pp = find_path_by_dev(vecs->pathvec, uev->kernel); > if (pp) { > int r; > @@ -637,6 +642,7 @@ uev_add_path (struct uevent *uev, struct vectors * vecs) > free_path(pp); > ret = 1; > } > +out_unlock: > lock_cleanup_pop(vecs->lock); > return ret; > } > @@ -780,22 +786,23 @@ fail: > static int > uev_remove_path (struct uevent *uev, struct vectors * vecs) > { > - struct path *pp; > - int ret; > + struct path *pp = NULL; > + int ret = 0; > > condlog(2, "%s: remove path (uevent)", uev->kernel); > pthread_cleanup_push(cleanup_lock, &vecs->lock); > lock(vecs->lock); > pthread_testcancel(); > - pp = find_path_by_dev(vecs->pathvec, uev->kernel); > - if (pp) > - ret = ev_remove_path(pp, vecs); > - lock_cleanup_pop(vecs->lock); > - if (!pp) { > - /* Not an error; path might have been purged earlier */ > - condlog(0, "%s: path already removed", uev->kernel); > - return 0; > + if (filter_devnode(conf->blist_devnode, conf->elist_devnode, > + uev->kernel) == 0) { > + pp = find_path_by_dev(vecs->pathvec, uev->kernel); > + if (pp) > + ret = ev_remove_path(pp, vecs); > + else > + /* Not an error; path might have been purged earlier */ > + condlog(0, "%s: path already removed", uev->kernel); > } > + lock_cleanup_pop(vecs->lock); > return ret; > } > > @@ -905,7 +912,7 @@ uev_update_path (struct uevent *uev, struct vectors * vecs) > ro = uevent_get_disk_ro(uev); > > if (ro >= 0) { > - struct path * pp; > + struct path * pp = NULL; > struct multipath *mpp = NULL; > > condlog(2, "%s: update path write_protect to '%d' (uevent)", > @@ -918,6 +925,10 @@ uev_update_path (struct uevent *uev, struct vectors * vecs) > * need to be at the same indentation level, hence > * this slightly convoluted codepath. > */ > + if (filter_devnode(conf->blist_devnode, conf->elist_devnode, > + uev->kernel) > 0) { > + goto out_unlock; > + } > pp = find_path_by_dev(vecs->pathvec, uev->kernel); > if (pp) { > if (pp->initialized == INIT_REQUESTED_UDEV) { > @@ -937,11 +948,13 @@ uev_update_path (struct uevent *uev, struct vectors * vecs) > uev->kernel, mpp->alias, retval); > } > } > + out_unlock: > lock_cleanup_pop(vecs->lock); > if (!pp) { > - condlog(0, "%s: spurious uevent, path not found", > - uev->kernel); > - return 1; > + if (retval) > + condlog(0, "%s: spurious uevent, path not found", > + uev->kernel); > + return retval; > } > if (retval == 2) > return uev_add_path(uev, vecs); > @@ -1059,10 +1072,6 @@ uev_trigger (struct uevent * uev, void * trigger_data) > /* > * path add/remove event > */ > - if (filter_devnode(conf->blist_devnode, conf->elist_devnode, > - uev->kernel) > 0) > - goto out; > - > if (!strncmp(uev->action, "add", 3)) { > r = uev_add_path(uev, vecs); > goto out; > -- > 2.6.6 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
Merged On Wed, May 11, 2016 at 7:18 PM, Benjamin Marzinski <bmarzins@redhat.com> wrote: > On Wed, May 11, 2016 at 12:35:41PM +0200, Hannes Reinecke wrote: > > Ben Marzinski pointed out that filter_devnode() is used > > without any lock or configuration settings in uev_trigger(), > > and hence might be invalid when processing events during > > reconfiguration. > > So move it into the individual functions and handle it > > with the vector lock held. > > ACK > > -Ben > > > > > Signed-off-by: Hannes Reinecke <hare@suse.com> > > --- > > multipathd/main.c | 45 +++++++++++++++++++++++++++------------------ > > 1 file changed, 27 insertions(+), 18 deletions(-) > > > > diff --git a/multipathd/main.c b/multipathd/main.c > > index 58e8854..2c7486d 100644 > > --- a/multipathd/main.c > > +++ b/multipathd/main.c > > @@ -580,6 +580,11 @@ uev_add_path (struct uevent *uev, struct vectors * > vecs) > > pthread_cleanup_push(cleanup_lock, &vecs->lock); > > lock(vecs->lock); > > pthread_testcancel(); > > + if (filter_devnode(conf->blist_devnode, conf->elist_devnode, > > + uev->kernel) > 0) { > > + ret = 0; > > + goto out_unlock; > > + } > > pp = find_path_by_dev(vecs->pathvec, uev->kernel); > > if (pp) { > > int r; > > @@ -637,6 +642,7 @@ uev_add_path (struct uevent *uev, struct vectors * > vecs) > > free_path(pp); > > ret = 1; > > } > > +out_unlock: > > lock_cleanup_pop(vecs->lock); > > return ret; > > } > > @@ -780,22 +786,23 @@ fail: > > static int > > uev_remove_path (struct uevent *uev, struct vectors * vecs) > > { > > - struct path *pp; > > - int ret; > > + struct path *pp = NULL; > > + int ret = 0; > > > > condlog(2, "%s: remove path (uevent)", uev->kernel); > > pthread_cleanup_push(cleanup_lock, &vecs->lock); > > lock(vecs->lock); > > pthread_testcancel(); > > - pp = find_path_by_dev(vecs->pathvec, uev->kernel); > > - if (pp) > > - ret = ev_remove_path(pp, vecs); > > - lock_cleanup_pop(vecs->lock); > > - if (!pp) { > > - /* Not an error; path might have been purged earlier */ > > - condlog(0, "%s: path already removed", uev->kernel); > > - return 0; > > + if (filter_devnode(conf->blist_devnode, conf->elist_devnode, > > + uev->kernel) == 0) { > > + pp = find_path_by_dev(vecs->pathvec, uev->kernel); > > + if (pp) > > + ret = ev_remove_path(pp, vecs); > > + else > > + /* Not an error; path might have been purged > earlier */ > > + condlog(0, "%s: path already removed", > uev->kernel); > > } > > + lock_cleanup_pop(vecs->lock); > > return ret; > > } > > > > @@ -905,7 +912,7 @@ uev_update_path (struct uevent *uev, struct vectors > * vecs) > > ro = uevent_get_disk_ro(uev); > > > > if (ro >= 0) { > > - struct path * pp; > > + struct path * pp = NULL; > > struct multipath *mpp = NULL; > > > > condlog(2, "%s: update path write_protect to '%d' > (uevent)", > > @@ -918,6 +925,10 @@ uev_update_path (struct uevent *uev, struct vectors > * vecs) > > * need to be at the same indentation level, hence > > * this slightly convoluted codepath. > > */ > > + if (filter_devnode(conf->blist_devnode, > conf->elist_devnode, > > + uev->kernel) > 0) { > > + goto out_unlock; > > + } > > pp = find_path_by_dev(vecs->pathvec, uev->kernel); > > if (pp) { > > if (pp->initialized == INIT_REQUESTED_UDEV) { > > @@ -937,11 +948,13 @@ uev_update_path (struct uevent *uev, struct > vectors * vecs) > > uev->kernel, mpp->alias, retval); > > } > > } > > + out_unlock: > > lock_cleanup_pop(vecs->lock); > > if (!pp) { > > - condlog(0, "%s: spurious uevent, path not found", > > - uev->kernel); > > - return 1; > > + if (retval) > > + condlog(0, "%s: spurious uevent, path not > found", > > + uev->kernel); > > + return retval; > > } > > if (retval == 2) > > return uev_add_path(uev, vecs); > > @@ -1059,10 +1072,6 @@ uev_trigger (struct uevent * uev, void * > trigger_data) > > /* > > * path add/remove event > > */ > > - if (filter_devnode(conf->blist_devnode, conf->elist_devnode, > > - uev->kernel) > 0) > > - goto out; > > - > > if (!strncmp(uev->action, "add", 3)) { > > r = uev_add_path(uev, vecs); > > goto out; > > -- > > 2.6.6 > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
diff --git a/multipathd/main.c b/multipathd/main.c index 58e8854..2c7486d 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -580,6 +580,11 @@ uev_add_path (struct uevent *uev, struct vectors * vecs) pthread_cleanup_push(cleanup_lock, &vecs->lock); lock(vecs->lock); pthread_testcancel(); + if (filter_devnode(conf->blist_devnode, conf->elist_devnode, + uev->kernel) > 0) { + ret = 0; + goto out_unlock; + } pp = find_path_by_dev(vecs->pathvec, uev->kernel); if (pp) { int r; @@ -637,6 +642,7 @@ uev_add_path (struct uevent *uev, struct vectors * vecs) free_path(pp); ret = 1; } +out_unlock: lock_cleanup_pop(vecs->lock); return ret; } @@ -780,22 +786,23 @@ fail: static int uev_remove_path (struct uevent *uev, struct vectors * vecs) { - struct path *pp; - int ret; + struct path *pp = NULL; + int ret = 0; condlog(2, "%s: remove path (uevent)", uev->kernel); pthread_cleanup_push(cleanup_lock, &vecs->lock); lock(vecs->lock); pthread_testcancel(); - pp = find_path_by_dev(vecs->pathvec, uev->kernel); - if (pp) - ret = ev_remove_path(pp, vecs); - lock_cleanup_pop(vecs->lock); - if (!pp) { - /* Not an error; path might have been purged earlier */ - condlog(0, "%s: path already removed", uev->kernel); - return 0; + if (filter_devnode(conf->blist_devnode, conf->elist_devnode, + uev->kernel) == 0) { + pp = find_path_by_dev(vecs->pathvec, uev->kernel); + if (pp) + ret = ev_remove_path(pp, vecs); + else + /* Not an error; path might have been purged earlier */ + condlog(0, "%s: path already removed", uev->kernel); } + lock_cleanup_pop(vecs->lock); return ret; } @@ -905,7 +912,7 @@ uev_update_path (struct uevent *uev, struct vectors * vecs) ro = uevent_get_disk_ro(uev); if (ro >= 0) { - struct path * pp; + struct path * pp = NULL; struct multipath *mpp = NULL; condlog(2, "%s: update path write_protect to '%d' (uevent)", @@ -918,6 +925,10 @@ uev_update_path (struct uevent *uev, struct vectors * vecs) * need to be at the same indentation level, hence * this slightly convoluted codepath. */ + if (filter_devnode(conf->blist_devnode, conf->elist_devnode, + uev->kernel) > 0) { + goto out_unlock; + } pp = find_path_by_dev(vecs->pathvec, uev->kernel); if (pp) { if (pp->initialized == INIT_REQUESTED_UDEV) { @@ -937,11 +948,13 @@ uev_update_path (struct uevent *uev, struct vectors * vecs) uev->kernel, mpp->alias, retval); } } + out_unlock: lock_cleanup_pop(vecs->lock); if (!pp) { - condlog(0, "%s: spurious uevent, path not found", - uev->kernel); - return 1; + if (retval) + condlog(0, "%s: spurious uevent, path not found", + uev->kernel); + return retval; } if (retval == 2) return uev_add_path(uev, vecs); @@ -1059,10 +1072,6 @@ uev_trigger (struct uevent * uev, void * trigger_data) /* * path add/remove event */ - if (filter_devnode(conf->blist_devnode, conf->elist_devnode, - uev->kernel) > 0) - goto out; - if (!strncmp(uev->action, "add", 3)) { r = uev_add_path(uev, vecs); goto out;
Ben Marzinski pointed out that filter_devnode() is used without any lock or configuration settings in uev_trigger(), and hence might be invalid when processing events during reconfiguration. So move it into the individual functions and handle it with the vector lock held. Signed-off-by: Hannes Reinecke <hare@suse.com> --- multipathd/main.c | 45 +++++++++++++++++++++++++++------------------ 1 file changed, 27 insertions(+), 18 deletions(-)