Re: [PATCH] libmultipath: limit paths that can get wwid from environment

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel





[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux