On Thu, 2024-07-11 at 14:39 -0400, Benjamin Marzinski wrote: > 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." Right. One thing we might consider is to use the DM UUID as primary identifier for maps, rather than the name/alias. That wouldn't completely prevent a scenario like the above, but the user would have to mess with uid_attribute to make it happen. Instead of the WWID, we could actually have a dm_uuid field in struct multipath. Martin
