| Message ID | 1675963711-22722-1-git-send-email-bmarzins@redhat.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
| Series | libmultipath: limit paths that can get wwid from environment | expand |
On Thu, 2023-02-09 at 11:28 -0600, Benjamin Marzinski wrote: > Currently, whenever getting the uid_attribute from the udev database > fails, multipath will try to get it from the environment variables. > This > normally isn't a problem, since either multipath -u is getting called > from a uevent and the environment will have the correct value in that > variable, or something else is being run and that variable won't be > set. > However, when find_multipaths is configured to "smart", this causes > problems. For maybe devices, multipath needs to get the WWIDs of all > the > other block devices, to see if they match the maybe device wwid. If > one > of those devices doesn't have uid_attribute set in its udev database, > multipath will fall back to checking the environment for it, and it > will > find that variable set to the WWID of the maybe device that this > uevent > is for. This means that all devices with no WWID will end up > appearing > to have the same WWID as the maybe device, causing multipath to > incorrectly claim it. > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> I have to say I don't quite understand why we read from the environment at all if the libudev call fails. This was coded before I joined the project, so perhaps you can clarify it. Why do we expect the environment to provide the correct value if libudev cannot? If we need to keep this fallback, I wonder if we need another field in "struct path" for it. For example, we could read MAJOR and MINOR from the environment and use uid_attribute only if the result matches the path's devt. Regards Martin > --- > libmultipath/discovery.c | 2 +- > libmultipath/structs.h | 1 + > multipath/main.c | 2 ++ > 3 files changed, 4 insertions(+), 1 deletion(-) > > diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c > index 67ac0e6d..3a5ba173 100644 > --- a/libmultipath/discovery.c > +++ b/libmultipath/discovery.c > @@ -2093,7 +2093,7 @@ get_udev_uid(struct path * pp, const char > *uid_attribute, struct udev_device *ud > const char *value; > > value = udev_device_get_property_value(udev, uid_attribute); > - if (!value || strlen(value) == 0) > + if ((!value || strlen(value) == 0) && pp->can_use_env_uid) > value = getenv(uid_attribute); > if (value && strlen(value)) { > len = strlcpy(pp->wwid, value, WWID_SIZE); > diff --git a/libmultipath/structs.h b/libmultipath/structs.h > index e2294323..a1208751 100644 > --- a/libmultipath/structs.h > +++ b/libmultipath/structs.h > @@ -368,6 +368,7 @@ struct path { > unsigned int dev_loss; > int eh_deadline; > bool is_checked; > + bool can_use_env_uid; > /* configlet pointers */ > vector hwe; > struct gen_path generic_path; > diff --git a/multipath/main.c b/multipath/main.c > index b9f360b4..90f940f1 100644 > --- a/multipath/main.c > +++ b/multipath/main.c > @@ -607,6 +607,8 @@ check_path_valid(const char *name, struct config > *conf, bool is_uevent) > pp = alloc_path(); > if (!pp) > return RTVL_FAIL; > + if (is_uevent) > + pp->can_use_env_uid = true; > > r = is_path_valid(name, conf, pp, is_uevent); > if (r <= PATH_IS_ERROR || r >= PATH_MAX_VALID_RESULT) -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Mon, Mar 20, 2023 at 02:53:20PM +0000, Martin Wilck wrote: > On Thu, 2023-02-09 at 11:28 -0600, Benjamin Marzinski wrote: > > Currently, whenever getting the uid_attribute from the udev database > > fails, multipath will try to get it from the environment variables. > > This > > normally isn't a problem, since either multipath -u is getting called > > from a uevent and the environment will have the correct value in that > > variable, or something else is being run and that variable won't be > > set. > > However, when find_multipaths is configured to "smart", this causes > > problems. For maybe devices, multipath needs to get the WWIDs of all > > the > > other block devices, to see if they match the maybe device wwid. If > > one > > of those devices doesn't have uid_attribute set in its udev database, > > multipath will fall back to checking the environment for it, and it > > will > > find that variable set to the WWID of the maybe device that this > > uevent > > is for. This means that all devices with no WWID will end up > > appearing > > to have the same WWID as the maybe device, causing multipath to > > incorrectly claim it. > > > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> > > I have to say I don't quite understand why we read from the environment > at all if the libudev call fails. This was coded before I joined the > project, so perhaps you can clarify it. Why do we expect the > environment to provide the correct value if libudev cannot? I'm pretty sure that the udev database for a device isn't set up yet when we are in the middle of processing the ADD event for it. When udev calls "multipath -u" on the add event, we can't look up the value in the database because the database entry for the device doesn't exist yet. We have to use the value it passed in via the enviroment. > If we need to keep this fallback, I wonder if we need another field in > "struct path" for it. For example, we could read MAJOR and MINOR from > the environment and use uid_attribute only if the result matches the > path's devt. This is basically what my patch does. It sets the can_use_env_uid flag only on the device that the uevent is for, so the only device that can get its WWID from the environment is the device whose uevent we are processing. The other paths we find in path_discovery() must be intialized, which means that they will have a udev database entry, and we can find their WWID there. -Ben > Regards > Martin > > > > > > > > --- > > libmultipath/discovery.c | 2 +- > > libmultipath/structs.h | 1 + > > multipath/main.c | 2 ++ > > 3 files changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c > > index 67ac0e6d..3a5ba173 100644 > > --- a/libmultipath/discovery.c > > +++ b/libmultipath/discovery.c > > @@ -2093,7 +2093,7 @@ get_udev_uid(struct path * pp, const char > > *uid_attribute, struct udev_device *ud > > const char *value; > > > > value = udev_device_get_property_value(udev, uid_attribute); > > - if (!value || strlen(value) == 0) > > + if ((!value || strlen(value) == 0) && pp->can_use_env_uid) > > value = getenv(uid_attribute); > > if (value && strlen(value)) { > > len = strlcpy(pp->wwid, value, WWID_SIZE); > > diff --git a/libmultipath/structs.h b/libmultipath/structs.h > > index e2294323..a1208751 100644 > > --- a/libmultipath/structs.h > > +++ b/libmultipath/structs.h > > @@ -368,6 +368,7 @@ struct path { > > unsigned int dev_loss; > > int eh_deadline; > > bool is_checked; > > + bool can_use_env_uid; > > /* configlet pointers */ > > vector hwe; > > struct gen_path generic_path; > > diff --git a/multipath/main.c b/multipath/main.c > > index b9f360b4..90f940f1 100644 > > --- a/multipath/main.c > > +++ b/multipath/main.c > > @@ -607,6 +607,8 @@ check_path_valid(const char *name, struct config > > *conf, bool is_uevent) > > pp = alloc_path(); > > if (!pp) > > return RTVL_FAIL; > > + if (is_uevent) > > + pp->can_use_env_uid = true; > > > > r = is_path_valid(name, conf, pp, is_uevent); > > if (r <= PATH_IS_ERROR || r >= PATH_MAX_VALID_RESULT) -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Mon, 2023-03-20 at 15:37 -0500, Benjamin Marzinski wrote: > > > > I have to say I don't quite understand why we read from the > > environment > > at all if the libudev call fails. This was coded before I joined > > the > > project, so perhaps you can clarify it. Why do we expect the > > environment to provide the correct value if libudev cannot? > > I'm pretty sure that the udev database for a device isn't set up yet > when we are in the middle of processing the ADD event for it. You are right, of course. I had a brain fart. > When udev > calls "multipath -u" on the add event, we can't look up the value in > the > database because the database entry for the device doesn't exist yet. > We > have to use the value it passed in via the enviroment. > > > If we need to keep this fallback, I wonder if we need another field > > in > > "struct path" for it. For example, we could read MAJOR and MINOR > > from > > the environment and use uid_attribute only if the result matches > > the > > path's devt. > > This is basically what my patch does. It sets the can_use_env_uid > flag > only on the device that the uevent is for, so the only device that > can > get its WWID from the environment is the device whose uevent we are > processing. The other paths we find in path_discovery() must be > intialized, which means that they will have a udev database entry, > and > we can find their WWID there. I wasn't saying your patch was not correct. I was just not so excited about adding this field, which only multipath uses, to "struct path". But I just checked this doesn't change the struct size, and given that the environment is more important than I realized yesterday, your approach is more efficient than what I suggested. Martin -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Thu, 2023-02-09 at 11:28 -0600, Benjamin Marzinski wrote: > Currently, whenever getting the uid_attribute from the udev database > fails, multipath will try to get it from the environment variables. > This > normally isn't a problem, since either multipath -u is getting called > from a uevent and the environment will have the correct value in that > variable, or something else is being run and that variable won't be > set. > However, when find_multipaths is configured to "smart", this causes > problems. For maybe devices, multipath needs to get the WWIDs of all > the > other block devices, to see if they match the maybe device wwid. If > one > of those devices doesn't have uid_attribute set in its udev database, > multipath will fall back to checking the environment for it, and it > will > find that variable set to the WWID of the maybe device that this > uevent > is for. This means that all devices with no WWID will end up > appearing > to have the same WWID as the maybe device, causing multipath to > incorrectly claim it. > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> Reviewed-by: Martin Wilck <mwilck@suse.com> -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c index 67ac0e6d..3a5ba173 100644 --- a/libmultipath/discovery.c +++ b/libmultipath/discovery.c @@ -2093,7 +2093,7 @@ get_udev_uid(struct path * pp, const char *uid_attribute, struct udev_device *ud const char *value; value = udev_device_get_property_value(udev, uid_attribute); - if (!value || strlen(value) == 0) + if ((!value || strlen(value) == 0) && pp->can_use_env_uid) value = getenv(uid_attribute); if (value && strlen(value)) { len = strlcpy(pp->wwid, value, WWID_SIZE); diff --git a/libmultipath/structs.h b/libmultipath/structs.h index e2294323..a1208751 100644 --- a/libmultipath/structs.h +++ b/libmultipath/structs.h @@ -368,6 +368,7 @@ struct path { unsigned int dev_loss; int eh_deadline; bool is_checked; + bool can_use_env_uid; /* configlet pointers */ vector hwe; struct gen_path generic_path; diff --git a/multipath/main.c b/multipath/main.c index b9f360b4..90f940f1 100644 --- a/multipath/main.c +++ b/multipath/main.c @@ -607,6 +607,8 @@ check_path_valid(const char *name, struct config *conf, bool is_uevent) pp = alloc_path(); if (!pp) return RTVL_FAIL; + if (is_uevent) + pp->can_use_env_uid = true; r = is_path_valid(name, conf, pp, is_uevent); if (r <= PATH_IS_ERROR || r >= PATH_MAX_VALID_RESULT)
Currently, whenever getting the uid_attribute from the udev database fails, multipath will try to get it from the environment variables. This normally isn't a problem, since either multipath -u is getting called from a uevent and the environment will have the correct value in that variable, or something else is being run and that variable won't be set. However, when find_multipaths is configured to "smart", this causes problems. For maybe devices, multipath needs to get the WWIDs of all the other block devices, to see if they match the maybe device wwid. If one of those devices doesn't have uid_attribute set in its udev database, multipath will fall back to checking the environment for it, and it will find that variable set to the WWID of the maybe device that this uevent is for. This means that all devices with no WWID will end up appearing to have the same WWID as the maybe device, causing multipath to incorrectly claim it. Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com> --- libmultipath/discovery.c | 2 +- libmultipath/structs.h | 1 + multipath/main.c | 2 ++ 3 files changed, 4 insertions(+), 1 deletion(-)