[dm-devel] [PATCH v2 84/84] libmultipath: add consistency check for alias settings

[Martin Wilck]

On Mon, 2020-08-17 at 19:30 -0500, Benjamin Marzinski wrote:
> On Wed, Aug 12, 2020 at 01:36:01PM +0200, mwilck at suse.com wrote:
> > From: Martin Wilck <mwilck at suse.com>
> > 
> > A typo in a config file, assigning the same alias to multiple
> > WWIDs,
> > can cause massive confusion and even data corruption. Check and
> > if possible fix the bindings file in such cases.
> > 
> > Signed-off-by: Martin Wilck <mwilck at suse.com>
> > ---
> >  libmultipath/alias.c | 257
> > +++++++++++++++++++++++++++++++++++++++++++
> >  libmultipath/alias.h |   3 +
> >  multipath/main.c     |   3 +
> >  multipathd/main.c    |   3 +
> >  4 files changed, 266 insertions(+)
> > 
> > diff --git a/libmultipath/alias.c b/libmultipath/alias.c
> > index 0759c4e..d328f77 100644
> > --- a/libmultipath/alias.c
> > +++ b/libmultipath/alias.c
> > 
> > +static int _check_bindings_file(const struct config *conf, FILE
> > *file,
> > +				 Bindings *bindings)
> > +{
> > +	int rc = 0;
> > +	unsigned int linenr = 0;
> > +	char *line = NULL;
> > +	size_t line_len = 0;
> > +	ssize_t n;
> > +
> > +	pthread_cleanup_push(free, line);
> > +	while ((n = getline(&line, &line_len, file)) >= 0) {
> > +		char *c, *alias, *wwid;
> > +		const char *mpe_wwid;
> > +
> > +		linenr++;
> > +		c = strpbrk(line, "#\n\r");
> > +		if (c)
> > +			*c = '\0';
> > +		alias = strtok(line, " \t");
> 
> I realize that your patch is just copying the existing code and I
> think
> our locking will save us here, but strtok isn't thread safe. We
> should
> consider changing all these to strtok_r().

I'll change this to strtok_r. I'll send a cumulative patch fixing the
other users as well.

I guess we'll also need to protect access to the bindings file. All
current accesses seem to be protected by the vecs lock (even
check_alias_settings(), called from reconfigure()), but we shouldn't
rely only on that. Also, multipath and multipathd could access the file
in parallel. Let's consider that later.

Btw, the vector holding the bindings in memory could obviously also
be used as a cache for looking up aliases at runtime. That would be
another item for later improvement. Actually, I thought we might
replace the vector here by some sort of hash map for even quicker
lookup. But I wanted to keep the already large patch series within
limits.

I'll send a v3 with your issues fixed.

Thanks,
Martin