| Message ID | 1620775324-23984-2-git-send-email-bmarzins@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | Memory issues found by coverity | expand |
On Tue, 2021-05-11 at 18:22 -0500, Benjamin Marzinski wrote: > In ev_remove_path(), if update_mpp_paths() fails, we delete the > entire > map. However, since update_mpp_paths() happens before we call > set_path_removed(), pp->initialized isn't set to INIT_REMOVED, so > remove_map_and_stop_waiter() doesn't remove the path when in removes > the > map. But with the map removed, there's nothing to keep us from > removing > the path. > > Call set_path_removed() before update_mpp_paths() to avoid the odd > case > of ev_remove_path() removing the map but not the path. > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > --- > libmultipath/structs_vec.c | 3 +-- > multipathd/main.c | 13 ++++++++----- > 2 files changed, 9 insertions(+), 7 deletions(-) > > diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c > index d242c06b..432c0c63 100644 > --- a/libmultipath/structs_vec.c > +++ b/libmultipath/structs_vec.c > @@ -45,8 +45,7 @@ int update_mpp_paths(struct multipath *mpp, vector > pathvec) > > /* > * Avoid adding removed paths to the > map again > - * when we reload it. Such paths may > exist if > - * domap fails in ev_remove_path(). > + * when we reload it. I'd like to keep the remark about domap(). It's meant as a reminder for us and future developers how this situation is most likely to come to pass. Other than that, ACK. Regards, Martin -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Wed, May 12, 2021 at 09:11:01AM +0000, Martin Wilck wrote: > On Tue, 2021-05-11 at 18:22 -0500, Benjamin Marzinski wrote: > > In ev_remove_path(), if update_mpp_paths() fails, we delete the > > entire > > map. However, since update_mpp_paths() happens before we call > > set_path_removed(), pp->initialized isn't set to INIT_REMOVED, so > > remove_map_and_stop_waiter() doesn't remove the path when in removes > > the > > map. But with the map removed, there's nothing to keep us from > > removing > > the path. > > > > Call set_path_removed() before update_mpp_paths() to avoid the odd > > case > > of ev_remove_path() removing the map but not the path. > > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > > --- > > libmultipath/structs_vec.c | 3 +-- > > multipathd/main.c | 13 ++++++++----- > > 2 files changed, 9 insertions(+), 7 deletions(-) > > > > diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c > > index d242c06b..432c0c63 100644 > > --- a/libmultipath/structs_vec.c > > +++ b/libmultipath/structs_vec.c > > @@ -45,8 +45,7 @@ int update_mpp_paths(struct multipath *mpp, vector > > pathvec) > > > > /* > > * Avoid adding removed paths to the > > map again > > - * when we reload it. Such paths may > > exist if > > - * domap fails in ev_remove_path(). > > + * when we reload it. > > I'd like to keep the remark about domap(). It's meant as a reminder for > us and future developers how this situation is most likely to come to > pass. Sure. I just removed it, since we now call update_mpp_paths immediately after calling set_path_removed(), so it seemed more obvious that we will run into this situation than it did before, when it only happened if we first failed in ev_remove_path(). I'm fine with putting it back. > > Other than that, ACK. > > Regards, > Martin -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c index d242c06b..432c0c63 100644 --- a/libmultipath/structs_vec.c +++ b/libmultipath/structs_vec.c @@ -45,8 +45,7 @@ int update_mpp_paths(struct multipath *mpp, vector pathvec) /* * Avoid adding removed paths to the map again - * when we reload it. Such paths may exist if - * domap fails in ev_remove_path(). + * when we reload it. */ pp1 = find_path_by_devt(pathvec, pp->dev_t); if (pp1 && pp->initialized != INIT_REMOVED && diff --git a/multipathd/main.c b/multipathd/main.c index 102946bf..449ce384 100644 --- a/multipathd/main.c +++ b/multipathd/main.c @@ -1199,6 +1199,13 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map) * avoid referring to the map of an orphaned path */ if ((mpp = pp->mpp)) { + /* + * Mark the path as removed. In case of success, we + * will delete it for good. Otherwise, it will be deleted + * later, unless all attempts to reload this map fail. + */ + set_path_removed(pp); + /* * transform the mp->pg vector of vectors of paths * into a mp->params string to feed the device-mapper @@ -1210,13 +1217,9 @@ ev_remove_path (struct path *pp, struct vectors * vecs, int need_do_map) } /* - * Mark the path as removed. In case of success, we - * will delete it for good. Otherwise, it will be deleted - * later, unless all attempts to reload this map fail. - * Note: we have to explicitly remove pp from mpp->paths, + * we have to explicitly remove pp from mpp->paths, * update_mpp_paths() doesn't do that. */ - set_path_removed(pp); i = find_slot(mpp->paths, pp); if (i != -1) vector_del_slot(mpp->paths, i);
In ev_remove_path(), if update_mpp_paths() fails, we delete the entire map. However, since update_mpp_paths() happens before we call set_path_removed(), pp->initialized isn't set to INIT_REMOVED, so remove_map_and_stop_waiter() doesn't remove the path when in removes the map. But with the map removed, there's nothing to keep us from removing the path. Call set_path_removed() before update_mpp_paths() to avoid the odd case of ev_remove_path() removing the map but not the path. Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> --- libmultipath/structs_vec.c | 3 +-- multipathd/main.c | 13 ++++++++----- 2 files changed, 9 insertions(+), 7 deletions(-)