| Message ID | 20230901180235.23980-5-mwilck@suse.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | christophe varoqui |
| Headers | show |
| Series | multipath-tools: user-friendly names rework | expand |
On Fri, Sep 01, 2023 at 08:02:17PM +0200, mwilck@suse.com wrote: > From: Martin Wilck <mwilck@suse.com> > > If the bindings file is changed in a way that multipathd can't handle > (e.g. by swapping the aliases of two maps), multipathd must not try > to re-use an alias that is already used by another map. Creating > or renaming a map with such an alias will fail. We already avoid > this for some cases, but not for all. Fix it. > > Signed-off-by: Martin Wilck <mwilck@suse.com> > Cc: David Bond <dbond@suse.com> > --- > libmultipath/alias.c | 36 +++++++++++++++++++++++++++++++----- > tests/alias.c | 2 +- > 2 files changed, 32 insertions(+), 6 deletions(-) > > diff --git a/libmultipath/alias.c b/libmultipath/alias.c > index 9b9b789..f7834d1 100644 > --- a/libmultipath/alias.c > +++ b/libmultipath/alias.c > @@ -120,7 +120,7 @@ static bool alias_already_taken(const char *alias, const char *map_wwid) > if (dm_get_uuid(alias, wwid, sizeof(wwid)) == 0 && > strncmp(map_wwid, wwid, sizeof(wwid)) == 0) > return false; > - condlog(3, "%s: alias '%s' already taken, but not in bindings file. reselecting alias", > + condlog(3, "%s: alias '%s' already taken, reselecting alias", > map_wwid, alias); > return true; > } > @@ -363,28 +363,54 @@ char *get_user_friendly_alias(const char *wwid, const char *file, const char *al > * allocated correctly > */ > if (strcmp(buff, wwid) == 0) { I'm confused about this. We should only have alias_old set if there already exists a device that matches this WWID, right? That's what I though alias_old means. Am I missing some way that alias_old could get set to something other than the alias of an already existing device with a matching WWID? Otherwise, once we verify that that this mapping also exists in the bindings file, we should be fine to use it? > - alias = strdup(alias_old); > + if (!alias_already_taken(alias_old, wwid)) > + alias = strdup(alias_old); > + else > + condlog(0, "ERROR: old alias [%s] for wwid [%s] is used by other map", > + alias_old, wwid); > goto out; > + > } else { > condlog(0, "alias %s already bound to wwid %s, cannot reuse", > alias_old, buff); > - goto new_alias; extra semicolon. > + goto new_alias; ; > } > } > > + /* > + * Look for an existing alias in the bindings file. > + * Pass prefix = NULL, so lookup_binding() won't try to allocate a new id. > + */ There's no point in saving the return value here. We don't use if for anything. > id = lookup_binding(f, wwid, &alias, NULL, 0); > if (alias) { > - condlog(3, "Use existing binding [%s] for WWID [%s]", > - alias, wwid); > + if (alias_already_taken(alias, wwid)) { > + free(alias); > + alias = NULL; > + } else > + condlog(3, "Use existing binding [%s] for WWID [%s]", > + alias, wwid); > goto out; > } > > /* allocate the existing alias in the bindings file */ > id = scan_devname(alias_old, prefix); Again, unless I'm overlooking something, I don't think we need to check if the alias is already taken here. Since we know that a device already exists with alias_old and the correct WWID, as long as alias_old is a valid user_friendly_name we can just use it. > + if (id > 0 && id_already_taken(id, prefix, wwid)) { > + condlog(0, "ERROR: old alias [%s] for wwid [%s] is used by other map", > + alias_old, wwid); > + goto out; > + } > > new_alias: > if (id <= 0) { > + /* > + * no existing alias was provided, or allocating it > + * failed. Try a new one. > + */ > id = lookup_binding(f, wwid, &alias, prefix, 1); If lookup_binding() was going to return (id == 0) it already would have when we called it earlier in this function, so I don't think this check is necessary either. -Ben > + if (id == 0 && alias_already_taken(alias, wwid)) { > + free(alias); > + alias = NULL; > + } > if (id <= 0) > goto out; > else > diff --git a/tests/alias.c b/tests/alias.c > index 3ca6c28..11f209e 100644 > --- a/tests/alias.c > +++ b/tests/alias.c > @@ -398,7 +398,7 @@ static void mock_self_alias(const char *alias, const char *wwid) > will_return(__wrap_dm_get_uuid, wwid); > } > > -#define USED_STR(alias_str, wwid_str) wwid_str ": alias '" alias_str "' already taken, but not in bindings file. reselecting alias\n" > +#define USED_STR(alias_str, wwid_str) wwid_str ": alias '" alias_str "' already taken, reselecting alias\n" > > static void mock_failed_alias(const char *alias, char *msg) > { > -- > 2.41.0 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Wed, 2023-09-06 at 17:42 -0500, Benjamin Marzinski wrote: > On Fri, Sep 01, 2023 at 08:02:17PM +0200, mwilck@suse.com wrote: > > From: Martin Wilck <mwilck@suse.com> > > > > If the bindings file is changed in a way that multipathd can't > > handle > > (e.g. by swapping the aliases of two maps), multipathd must not try > > to re-use an alias that is already used by another map. Creating > > or renaming a map with such an alias will fail. We already avoid > > this for some cases, but not for all. Fix it. > > > > Signed-off-by: Martin Wilck <mwilck@suse.com> > > Cc: David Bond <dbond@suse.com> > > --- > > libmultipath/alias.c | 36 +++++++++++++++++++++++++++++++----- > > tests/alias.c | 2 +- > > 2 files changed, 32 insertions(+), 6 deletions(-) > > > > diff --git a/libmultipath/alias.c b/libmultipath/alias.c > > index 9b9b789..f7834d1 100644 > > --- a/libmultipath/alias.c > > +++ b/libmultipath/alias.c > > @@ -120,7 +120,7 @@ static bool alias_already_taken(const char > > *alias, const char *map_wwid) > > if (dm_get_uuid(alias, wwid, sizeof(wwid)) == 0 && > > strncmp(map_wwid, wwid, sizeof(wwid)) == 0) > > return false; > > - condlog(3, "%s: alias '%s' already taken, but not > > in bindings file. reselecting alias", > > + condlog(3, "%s: alias '%s' already taken, > > reselecting alias", > > map_wwid, alias); > > return true; > > } > > @@ -363,28 +363,54 @@ char *get_user_friendly_alias(const char > > *wwid, const char *file, const char *al > > * allocated correctly > > */ > > if (strcmp(buff, wwid) == 0) { > > I'm confused about this. We should only have alias_old set if there > already exists a device that matches this WWID, right? That's what I > though alias_old means. Am I missing some way that alias_old could > get > set to something other than the alias of an already existing device > with > a matching WWID? Otherwise, once we verify that that this mapping > also > exists in the bindings file, we should be fine to use it? > This is mainly meant as an additional consistency check, to make sure that the logic in get_user_friendly_alias() is correct, no matter how "alias_old" was set by the caller. Note that alias_old is ab^H^Hreused by the ACT_RENAME action; it is not immediately obvious that alias_old can never be set to an invalid value in get_user_friendly_alias(). condlog() prints "ERROR" here because it's a condition that shouldn't occur. > > - alias = strdup(alias_old); > > + if (!alias_already_taken(alias_old, wwid)) > > + alias = strdup(alias_old); > > + else > > + condlog(0, "ERROR: old alias [%s] > > for wwid [%s] is used by other map", > > + alias_old, wwid); > > goto out; > > + > > } else { > > condlog(0, "alias %s already bound to wwid > > %s, cannot reuse", > > alias_old, buff); > > - goto new_alias; > > extra semicolon. > > > + goto new_alias; ; > > } > > } > > > > + /* > > + * Look for an existing alias in the bindings file. > > + * Pass prefix = NULL, so lookup_binding() won't try to > > allocate a new id. > > + */ > > There's no point in saving the return value here. We don't use if for > anything. > > > id = lookup_binding(f, wwid, &alias, NULL, 0); > > if (alias) { > > - condlog(3, "Use existing binding [%s] for WWID > > [%s]", > > - alias, wwid); > > + if (alias_already_taken(alias, wwid)) { > > + free(alias); > > + alias = NULL; > > + } else > > + condlog(3, "Use existing binding [%s] for > > WWID [%s]", > > + alias, wwid); > > goto out; > > } > > > > /* allocate the existing alias in the bindings file */ > > id = scan_devname(alias_old, prefix); > > Again, unless I'm overlooking something, I don't think we need to > check if the alias is already taken here. Since we know that a device > already exists with alias_old and the correct WWID, as long as > alias_old > is a valid user_friendly_name we can just use it. Similar reasoning as above. We could perhaps remove these checks, but we'd need to replace them by comments explaining why this condition can't occur. We could (and maybe should) move the call to find_existing_alias() from add_map_with_path() to get_user_friendly_alias(), so that we have the entire alias logic in a single place. The mpp->alias_old field would then only be used for ACT_RENAME. Martin -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Thu, 2023-09-07 at 09:24 +0200, Martin Wilck wrote: > On Wed, 2023-09-06 at 17:42 -0500, Benjamin Marzinski wrote: > > On Fri, Sep 01, 2023 at 08:02:17PM +0200, mwilck@suse.com wrote: > > > > > > Again, unless I'm overlooking something, I don't think we need to > > check if the alias is already taken here. Since we know that a > > device > > already exists with alias_old and the correct WWID, as long as > > alias_old > > is a valid user_friendly_name we can just use it. > > Similar reasoning as above. We could perhaps remove these checks, but > we'd need to replace them by comments explaining why this condition > can't occur. > > We could (and maybe should) move the call to find_existing_alias() > from > add_map_with_path() to get_user_friendly_alias(), so that we have the > entire alias logic in a single place. The mpp->alias_old field would > then only be used for ACT_RENAME. Well, if we do this, we would need to pass vecs->mpvec to get_user_friendly_alias(), which means that we could use it also for the alias_already_taken() check. It's not exactly the same because in one case we look at internal state and in the other case at kernel state. OTOH, we do trust that the two are in agreement, right? And we derive alias_old from the mpvec today, anyway. Do you agree? Regards Martin -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Thu, 2023-09-07 at 15:33 +0200, Martin Wilck wrote: > On Thu, 2023-09-07 at 09:24 +0200, Martin Wilck wrote: > > On Wed, 2023-09-06 at 17:42 -0500, Benjamin Marzinski wrote: > > > On Fri, Sep 01, 2023 at 08:02:17PM +0200, mwilck@suse.com wrote: > > > > > > > > > Again, unless I'm overlooking something, I don't think we need to > > > check if the alias is already taken here. Since we know that a > > > device > > > already exists with alias_old and the correct WWID, as long as > > > alias_old > > > is a valid user_friendly_name we can just use it. > > > > Similar reasoning as above. We could perhaps remove these checks, > > but > > we'd need to replace them by comments explaining why this condition > > can't occur. > > > > We could (and maybe should) move the call to find_existing_alias() > > from > > add_map_with_path() to get_user_friendly_alias(), so that we have > > the > > entire alias logic in a single place. The mpp->alias_old field > > would > > then only be used for ACT_RENAME. > > Well, if we do this, we would need to pass vecs->mpvec to > get_user_friendly_alias(), which means that we could use it also for > the alias_already_taken() check. It's not exactly the same because in > one case we look at internal state and in the other case at kernel > state. OTOH, we do trust that the two are in agreement, right? And we > derive alias_old from the mpvec today, anyway. Ok, this doesn't work. The kernel device mapper mandates no naming conventions for map aliases / names, except that they're unique. The mpvec contains only aliases of multipath maps. But it's not forbidden that some non-multipath map be called "mpathc". OTOH, if we have positive evidence that a given WWID has an alias mpathX assigned, we do know that no other map can have this same name. It just comes down to whether we trust the mpvec to correctly represent kernel state, and I suppose we have to, anyway. Sorry for the spam. I'll just remove these these checks, and the corresponding test cases. Martin -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
diff --git a/libmultipath/alias.c b/libmultipath/alias.c index 9b9b789..f7834d1 100644 --- a/libmultipath/alias.c +++ b/libmultipath/alias.c @@ -120,7 +120,7 @@ static bool alias_already_taken(const char *alias, const char *map_wwid) if (dm_get_uuid(alias, wwid, sizeof(wwid)) == 0 && strncmp(map_wwid, wwid, sizeof(wwid)) == 0) return false; - condlog(3, "%s: alias '%s' already taken, but not in bindings file. reselecting alias", + condlog(3, "%s: alias '%s' already taken, reselecting alias", map_wwid, alias); return true; } @@ -363,28 +363,54 @@ char *get_user_friendly_alias(const char *wwid, const char *file, const char *al * allocated correctly */ if (strcmp(buff, wwid) == 0) { - alias = strdup(alias_old); + if (!alias_already_taken(alias_old, wwid)) + alias = strdup(alias_old); + else + condlog(0, "ERROR: old alias [%s] for wwid [%s] is used by other map", + alias_old, wwid); goto out; + } else { condlog(0, "alias %s already bound to wwid %s, cannot reuse", alias_old, buff); - goto new_alias; + goto new_alias; ; } } + /* + * Look for an existing alias in the bindings file. + * Pass prefix = NULL, so lookup_binding() won't try to allocate a new id. + */ id = lookup_binding(f, wwid, &alias, NULL, 0); if (alias) { - condlog(3, "Use existing binding [%s] for WWID [%s]", - alias, wwid); + if (alias_already_taken(alias, wwid)) { + free(alias); + alias = NULL; + } else + condlog(3, "Use existing binding [%s] for WWID [%s]", + alias, wwid); goto out; } /* allocate the existing alias in the bindings file */ id = scan_devname(alias_old, prefix); + if (id > 0 && id_already_taken(id, prefix, wwid)) { + condlog(0, "ERROR: old alias [%s] for wwid [%s] is used by other map", + alias_old, wwid); + goto out; + } new_alias: if (id <= 0) { + /* + * no existing alias was provided, or allocating it + * failed. Try a new one. + */ id = lookup_binding(f, wwid, &alias, prefix, 1); + if (id == 0 && alias_already_taken(alias, wwid)) { + free(alias); + alias = NULL; + } if (id <= 0) goto out; else diff --git a/tests/alias.c b/tests/alias.c index 3ca6c28..11f209e 100644 --- a/tests/alias.c +++ b/tests/alias.c @@ -398,7 +398,7 @@ static void mock_self_alias(const char *alias, const char *wwid) will_return(__wrap_dm_get_uuid, wwid); } -#define USED_STR(alias_str, wwid_str) wwid_str ": alias '" alias_str "' already taken, but not in bindings file. reselecting alias\n" +#define USED_STR(alias_str, wwid_str) wwid_str ": alias '" alias_str "' already taken, reselecting alias\n" static void mock_failed_alias(const char *alias, char *msg) {