[dm-devel] [PATCH 06/14] multipathd: reconfigure: disallow changing uid_attrs

[Martin Wilck]

On Fri, 2022-04-01 at 22:40 -0500, Benjamin Marzinski wrote:
> > On Thu, Mar 31, 2022 at 12:15:02AM +0200, mwilck at suse.com wrote:
> > uevent merging by WWID relies on the uid_attrs config option. As we
> > drop struct config between calls to uevent_merge(), we must be sure
> > that the WWID is not changed in reconfigure().
> > 
> > Signed-off-by: Martin Wilck <mwilck at suse.com>
> > ---
> >  multipath/multipath.conf.5 |  2 ++
> >  multipathd/main.c          | 53 +++++++++++++++++++++++++++++++---
> > ----
> >  2 files changed, 46 insertions(+), 9 deletions(-)
> > 
> > diff --git a/multipath/multipath.conf.5
> > b/multipath/multipath.conf.5
> > index 605b46e..a9cd776 100644
> > --- a/multipath/multipath.conf.5
> > +++ b/multipath/multipath.conf.5
> > @@ -264,6 +264,8 @@ If this option is configured and matches the
> > device
> >  node name of a device, it overrides any other configured  methods
> > for
> >  determining the WWID for this device.
> >  .PP
> > +This option cannot be changed during runtime with the multipathd
> > \fBreconfigure\fR command.
> > +.PP
> >  The default is: \fB<unset>\fR. To enable uevent merging, set it
> > e.g. to
> >  \(dqsd:ID_SERIAL dasd:ID_UID nvme:ID_WWN\(dq.
> >  .RE
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index 13b1948..f514b32 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -2835,11 +2835,52 @@ void rcu_free_config(struct rcu_head *head)
> >         free_config(conf);
> >  }
> >  
> > +static bool reconfigure_check_uid_attrs(const struct _vector
> > *old_attrs,
> > +                                       const struct _vector
> > *new_attrs)
> > +{
> > +       int i;
> > +       char *old;
> > +
> > +       if (VECTOR_SIZE(old_attrs) != VECTOR_SIZE(new_attrs))
> > +               return true;
> > +
> > +       vector_foreach_slot(old_attrs, old, i) {
> > +               char *new = VECTOR_SLOT(new_attrs, i);
> > +
> > +               if (strcmp(old, new))
> > +                       return true;
> > +       }
> > +
> > +       return false;
> > +}
> > +
> > +static void reconfigure_check(struct config *old, struct config
> > *new)
> > +{
> > +       int old_marginal_pathgroups;
> > +
> > +       old_marginal_pathgroups = old->marginal_pathgroups;
> > +       if ((old_marginal_pathgroups == MARGINAL_PATHGROUP_FPIN) !=
> > +           (new->marginal_pathgroups == MARGINAL_PATHGROUP_FPIN))
> > {
> > +               condlog(1, "multipathd must be restarted to turn %s
> > fpin marginal paths",
> > +                       (old_marginal_pathgroups ==
> > MARGINAL_PATHGROUP_FPIN)?
> > +                       "off" : "on");
> > +               new->marginal_pathgroups = old_marginal_pathgroups;
> > +       }
> > +
> > +       if (reconfigure_check_uid_attrs(&old->uid_attrs, &new-
> > >uid_attrs)) {
> > +               struct _vector v = new->uid_attrs;
> > +
> > +               condlog(1, "multipathd must be restarted to change
> > uid_attrs, keeping old values");
> > +               new->uid_attrs = old->uid_attrs;
> > +               vector_reset(&v);
> 
> This leaks the strings that v points to, right?  
> This also can happen in
> uid_attrs_handler(), I just noticed.

I was wondering about the same thing. But vector_reset() calls free()
on every slot. So no, I don't think anything is leaked here. The API
behaves admittedly in a surprising manner.

Martin