Re: [PATCH 31/44] libmultipath: add is_mpath_uuid() helper

[Benjamin Marzinski <bmarzins@redhat.com>]

On Thu, Jul 11, 2024 at 02:56:05PM +0200, Martin Wilck wrote:
> On Wed, 2024-07-10 at 23:38 -0400, Benjamin Marzinski wrote:
> > On Tue, Jul 09, 2024 at 11:39:22PM +0200, Martin Wilck wrote:
> > > Export it, as it will be used by multipathd and libmpathpersist.
> > > 
> > 
> > This is fine. But I was thinking that if you wanted to actually add
> > some optional flags to the flags argument of libmp_mapinfo(), this
> > would
> > be a good use. You could have a flag like DM_MAP_MPATH_WWID, which
> > would
> > verify that the device had a multipath UUID_PREFIX (and possibly
> > force a
> > check for a multipath target type) and then return the wwid without
> > the
> > multipath uuid prefix.
> > 
> > Just a thought.
> 
> Interesting idea. My thinking was that libmp_mapinfo() should only be
> an interface to libdevmapper, and should not interpret the data it
> returns in any way.
> 
> The only exception is the target type. In theory, the function could
> just have returned the target type, and left the interpretation to the
> caller. But then, it would have to perform several strdup()s for the
> results, and the caller would have to free() the data it wasn't
> interested in, which would lead to clumsy and inefficent code in the
> frequent case that we're just interested in "multipath" or "linear"
> targets. That's why I decided to pass the tgt_type field in, and return
> DMP_NO_MATCH if it didn't match, even though it's arguably odd in the
> mapinfo_t structure, which is otherwise only used for output fields.

I did wonder if it would make more sense to just pass a struct to
libmp_mapinfo() that contained all the input data, with the flags and
the identifiers (tgt_type just being an additional optional identier)
grouped together.  But I'm fine with the exiting interface so it didn't
seem worth commenting on.
 
> Your flag idea would work, too (an earlier version of this patch set
> used flags to indicate which output fields should be filled in; I
> discarded that because it was duplicated information, and more elegant
> to simply check it the respective output field was non-NULL).
> 
> Instead of tgt_type, we could also pass a "filter" callback function
> with which the caller could determine whether or not it was interested
> in a given map. But I wonder if that'd be over-engineered.
> 
> Whatever we eventually do, I think the is_mpath_uuid() helper will be
> useful. I am not sure whether we can unexport it even if we implement
> more sophisticated "filtering" in libmp_mapinfo().
> 
> In general, I wonder what multipath-tools should do if it encounters a
> map that has "multipath" target type, but the WWID of which doesn't
> conform to the mpath-XYZ convention, or vice versa. This can only
> happen if another tool like dmsetup had been used to create the map.
> IMO, if multipathd encounters such a situation, it should warn about it
> and keep its fingers off the map in question. But we might also play it
> simple and just assume that multipath maps always adhere to our
> convention. I don't think we currently behave consistently in this
> respect.

There was a time when there were users of the kernel multipath target
outside of the multipath-tools. To deal with them, the idea was that
multipathd would only manage devices that had a uuid with UUID_PREFIX.
That was a while ago, and we likely have been violating that condition,
but I haven't heard of any complaints. It might be worth auditing the
code to see if/where we aren't verifying new devices.

Obviously if we find a dm device that isn't a multipath target, we
shouldn't do anything with it, regardless of its UUID.
 
> AFAIU, both the WWID and the target type are immutable and can't be
> altered without destroying the map completely (because "multipath" is
> an "immutable target" in DM). Thus it should be sufficient to check the
> WWID when a map is first encountered.

Probably. Obviously, a multipath device could get removed outside of
multipathd and another device added with the same identifier that we're
using. There would be a window before we got the remove event where we
would be looking at the wrong device. But I can't really come up with
a way for this to happen that doesn't involve very creative stupidity on
the user's part, and since you need to be root to do this, I don't see
any security issues here.

This isn't exactly what you're asking about but the results are worse,
and I don't think there is a way to guarantee the following can't happen:

1. multipathd decides in needs to reload a multipath device that is not in
use and grabs the vecs lock so no events will get processed
2. user removes that device outside of multipathd (and likely outside of
multipath which should delegate the remove).
3. user creates a new multipath device outside of mulitpathd that has the
same name as the removed device. For this to happen by running multipath,
they either needed to be using user_friendly_names and haved removed the old
mapping from the bindings file, or they need to have editted multipath.conf
to set the alias of the new device to the alias of the old one.
4. multipathd reloads the table of the new device to point to old paths
5. user writes data to these old paths
6. multipathd gets the event and removes the device.

This results in data corruption, and there's no guarantee that
rechecking anything will always catch this. At some point we need to
just say "Sorry. You shot yourself in the foot."

But if we are just grabbing information about a device we already know
about, and it has switched so that its no longer a multipath device,
then multipathd can't really mess with it. All of our ioctls will fail,
and we will shortly clean it up anyway.

It's possible that we want to be more careful validating the WWID of
devices that we are looking up by major:minor, since it's much more
likely that a device got removed and a completey different dm device got
created and reused the same minor number, but I don't know why we would
ever be looking up a device we already know about by major:minor. So we
should always be checking these devices to verify that they are
multipath devices, and we currently are. 

> Thoughts?
> 
> Martin
> 
> > -Ben
> > 
> > > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > > ---
> > >  libmultipath/devmapper.c          | 13 +++++++++----
> > >  libmultipath/devmapper.h          |  2 ++
> > >  libmultipath/libmultipath.version |  1 +
> > >  3 files changed, 12 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
> > > index 806ffb8..5f6c0c8 100644
> > > --- a/libmultipath/devmapper.c
> > > +++ b/libmultipath/devmapper.c
> > > @@ -829,6 +829,11 @@ int dm_get_map(const char *name, unsigned long
> > > long *size, char **outparams)
> > >  	}
> > >  }
> > >  
> > > +bool is_mpath_uuid(const char uuid[DM_UUID_LEN])
> > > +{
> > > +	return !strncmp(uuid, UUID_PREFIX, UUID_PREFIX_LEN);
> > > +}
> > > +
> > >  /**
> > >   * dm_get_wwid(): return WWID for a multipath map
> > >   * @returns:
> > > @@ -845,7 +850,7 @@ int dm_get_wwid(const char *name, char *uuid,
> > > int uuid_len)
> > >  	if (rc != DMP_OK)
> > >  		return rc;
> > >  
> > > -	if (!strncmp(tmp, UUID_PREFIX, UUID_PREFIX_LEN))
> > > +	if (is_mpath_uuid(tmp))
> > >  		strlcpy(uuid, tmp + UUID_PREFIX_LEN, uuid_len);
> > >  	else {
> > >  		uuid[0] = '\0';
> > > @@ -970,7 +975,7 @@ int dm_is_mpath(const char *name)
> > >  			       (mapid_t) { .str = name },
> > >  			       (mapinfo_t) { .uuid = uuid,
> > > .tgt_type = TGT_MPATH });
> > >  
> > > -	if (rc != DMP_OK || strncmp(uuid, UUID_PREFIX,
> > > UUID_PREFIX_LEN))
> > > +	if (rc != DMP_OK || !is_mpath_uuid(uuid))
> > >  		return DM_IS_MPATH_NO;
> > >  	else
> > >  		return DM_IS_MPATH_YES;
> > > @@ -1065,7 +1070,7 @@ int _dm_flush_map (const char *mapname, int
> > > flags, int retries)
> > >  				  .uuid = uuid,
> > >  				  .tgt_type = TGT_MPATH,
> > >  				  .target = &params }) != DMP_OK
> > > -	    || strncmp(uuid, UUID_PREFIX, UUID_PREFIX_LEN))
> > > +	    || !is_mpath_uuid(uuid))
> > >  		return DM_FLUSH_OK; /* nothing to do */
> > >  
> > >  	/* if the device currently has no partitions, do not
> > > @@ -1309,7 +1314,7 @@ struct multipath *dm_get_multipath(const char
> > > *name)
> > >  			  }) != DMP_OK)
> > >  		return NULL;
> > >  
> > > -	if (strncmp(uuid, UUID_PREFIX, UUID_PREFIX_LEN))
> > > +	if (!is_mpath_uuid(uuid))
> > >  		return NULL;
> > >  
> > >  	strlcpy(mpp->wwid, uuid + UUID_PREFIX_LEN, sizeof(mpp-
> > > >wwid));
> > > diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
> > > index db5c5fd..a2b2837 100644
> > > --- a/libmultipath/devmapper.h
> > > +++ b/libmultipath/devmapper.h
> > > @@ -131,6 +131,8 @@ enum {
> > >  	DM_IS_MPATH_YES,
> > >  	DM_IS_MPATH_ERR,
> > >  };
> > > +
> > > +bool is_mpath_uuid(const char uuid[DM_UUID_LEN]);
> > >  int dm_is_mpath(const char *name);
> > >  
> > >  enum {
> > > diff --git a/libmultipath/libmultipath.version
> > > b/libmultipath/libmultipath.version
> > > index 7d3ff63..5b8f9e0 100644
> > > --- a/libmultipath/libmultipath.version
> > > +++ b/libmultipath/libmultipath.version
> > > @@ -127,6 +127,7 @@ global:
> > >  	init_foreign;
> > >  	init_prio;
> > >  	io_err_stat_handle_pathfail;
> > > +	is_mpath_uuid;
> > >  	is_path_valid;
> > >  	libmp_dm_task_create;
> > >  	libmp_get_version;
> > > -- 
> > > 2.45.2
> >