[dm-devel] [PATCH] multipathd: Fixed multipathd parameter invoking
Martin Wilck
mwilck at suse.com
Thu Dec 1 09:11:39 UTC 2022
On Thu, 2022-12-01 at 15:30 +0800, lixiaokeng wrote:
> Before we apply f812466f ("multipathd: more robust command parsing"),
> multipathd help shows:
> ...
> map|multipath $map getprstatus
> map|multipath $map setprstatus
> map|multipath $map unsetprstatus
> map|multipath $map getprkey
> map|multipath $map setprkey key $key
> map|multipath $map unsetprkey
> forcequeueing daemon
> restorequeueing daemon
> path $path setmarginal
> path $path unsetmarginal
> map|multipath $map unsetmarginal
>
> We think it is better to keep this consistent, so we fix it
> as this patch. Besides that, we also change "getprstatus'
> "getprkey" "setmarginal" "unsetmarginal" orders to keep
> consistent and avoid same problem if users use "multipathd
> xxx" (like getprkey).
Yes, it's unfortunate that the current behavior doesn't comply with the
old help text. But I disagree that this should be "fixed" in the
current code. The old syntax was arguably inconsistent. We now have a
very consistent command structure with a verb followed by arguments,
and we should keep it at that, even if it means that some scripts need
to be adapted.
Therefore I support Ben's approach.
Martin
>
> On 2022/12/1 0:04, Benjamin Marzinski wrote:
> > On Wed, Nov 30, 2022 at 05:13:28PM +0800, miaoguanqin wrote:
> > > ping
> >
> > I fixed this issue a different way. Previously we accepted any
> > ordering
> > of keywords, but we have always advertised a specific order in the
> > CLI
> > commands reference (to see it, just run "mulitpathd help" or use
> > any
> > other invalid keyword). The command word is first and the arguments
> > follow. I think we should keep this consistent, and I don't think
> > we
> > should go changing the order of existing commands. Instead, my
> > patch
> > makes libmpathpersist issue commands in the correct order. Could
> > you
> > look at:
> >
> > [dm-devel] [PATCH 2/2] libmpathpersist: fix command keyword
> > ordering
> > https://listman.redhat.com/archives/dm-devel/2022-November/052773.html
> >
> > and see if that solves your issue.
> >
> > -Ben
> >
> > >
> > > On 2022/11/25 9:26, miaoguanqin wrote:
> > > > Users may fail to execute command: multipathd and mpathpersist.
> > > >
> > > > When we execute the command mpathpersist：
> > > > mpathpersist --out --register --param-sark=123 --prout-type=5
> > > > /dev/mapper/mpathb
> > > > It return an error : Missing arguement. The preceding command
> > > > calls the
> > > > function
> > > > cli_setprkey, which is called by checking whether the handle
> > > > values are
> > > > consistent
> > > > with the command input. CVE-2022-41974 changed the handler
> > > > value of
> > > > function and
> > > > changed the mode of calculating handle. The handler value is
> > > > not equal to
> > > > the
> > > > command input, causing multipathd can not execute the true
> > > > funcion. It
> > > > could be
> > > > an same error for executing multipathd by the old mode.
> > > >
> > > > multipathd invokes the corresponding function based on the
> > > > handle value.
> > > > CVE-2022-41964 changed the method of calculating handler value.
> > > > Modify the
> > > > handle
> > > > value so that the corresponding function can be correctly
> > > > execute.
> > > >
> > > > Signed-off-by: miaoguanqin <miaoguanqin at huawei.com>
> > > > Signed-off-by: lixiaokeng <lixiaokeng at huawei.com>
> > > > ---
> > > > multipathd/callbacks.c | 18 +++++++++---------
> > > > multipathd/cli.h | 9 ++++++++-
> > > > 2 files changed, 17 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/multipathd/callbacks.c b/multipathd/callbacks.c
> > > > index fb87b280..f32666be 100644
> > > > --- a/multipathd/callbacks.c
> > > > +++ b/multipathd/callbacks.c
> > > > @@ -57,16 +57,16 @@ void init_handler_callbacks(void)
> > > > set_handler_callback(VRB_RESTOREQ | Q1_MAPS,
> > > > HANDLER(cli_restore_all_queueing));
> > > > set_unlocked_handler_callback(VRB_QUIT,
> > > > HANDLER(cli_quit));
> > > > set_unlocked_handler_callback(VRB_SHUTDOWN,
> > > > HANDLER(cli_shutdown));
> > > > - set_handler_callback(VRB_GETPRSTATUS | Q1_MAP,
> > > > HANDLER(cli_getprstatus));
> > > > - set_handler_callback(VRB_SETPRSTATUS | Q1_MAP,
> > > > HANDLER(cli_setprstatus));
> > > > - set_handler_callback(VRB_UNSETPRSTATUS | Q1_MAP,
> > > > HANDLER(cli_unsetprstatus));
> > > > + set_handler_callback(KEY_MAP | Q1_GETPRSTATUS,
> > > > HANDLER(cli_getprstatus));
> > > > + set_handler_callback(KEY_MAP | Q1_SETSTATUS,
> > > > HANDLER(cli_setprstatus));
> > > > + set_handler_callback(KEY_MAP | Q1_UNSETSTATUS,
> > > > HANDLER(cli_unsetprstatus));
> > > > set_handler_callback(VRB_FORCEQ | Q1_DAEMON,
> > > > HANDLER(cli_force_no_daemon_q));
> > > > set_handler_callback(VRB_RESTOREQ | Q1_DAEMON,
> > > > HANDLER(cli_restore_no_daemon_q));
> > > > - set_handler_callback(VRB_GETPRKEY | Q1_MAP,
> > > > HANDLER(cli_getprkey));
> > > > - set_handler_callback(VRB_SETPRKEY | Q1_MAP | Q2_KEY,
> > > > HANDLER(cli_setprkey));
> > > > - set_handler_callback(VRB_UNSETPRKEY | Q1_MAP,
> > > > HANDLER(cli_unsetprkey));
> > > > - set_handler_callback(VRB_SETMARGINAL | Q1_PATH,
> > > > HANDLER(cli_set_marginal));
> > > > - set_handler_callback(VRB_UNSETMARGINAL | Q1_PATH,
> > > > HANDLER(cli_unset_marginal));
> > > > - set_handler_callback(VRB_UNSETMARGINAL | Q1_MAP,
> > > > + set_handler_callback(KEY_MAP | Q1_GETPRKEY,
> > > > HANDLER(cli_getprkey));
> > > > + set_handler_callback(KEY_MAP | Q1_SETKEY | Q2_KEY,
> > > > HANDLER(cli_setprkey));
> > > > + set_handler_callback(KEY_MAP | Q1_UNSETKEY,
> > > > HANDLER(cli_unsetprkey));
> > > > + set_handler_callback(KEY_PATH | Q1_SETMARGINAL,
> > > > HANDLER(cli_set_marginal));
> > > > + set_handler_callback(KEY_PATH | Q1_UNSETMARGINAL,
> > > > HANDLER(cli_unset_marginal));
> > > > + set_handler_callback(KEY_MAP | Q1_UNSETMARGINAL,
> > > > HANDLER(cli_unset_all_marginal));
> > > > }
> > > > diff --git a/multipathd/cli.h b/multipathd/cli.h
> > > > index c6b79c9d..08ee5c8d 100644
> > > > --- a/multipathd/cli.h
> > > > +++ b/multipathd/cli.h
> > > > @@ -80,7 +80,14 @@ enum {
> > > > Q1_ALL = KEY_ALL << 8,
> > > > Q1_DAEMON = KEY_DAEMON << 8,
> > > > Q1_STATUS = KEY_STATUS << 8,
> > > > -
> > > > + Q1_SETKEY = VRB_SETPRKEY << 8,
> > > > + Q1_UNSETKEY = VRB_UNSETPRKEY << 8,
> > > > + Q1_SETSTATUS = VRB_SETPRSTATUS << 8,
> > > > + Q1_UNSETSTATUS = VRB_UNSETPRSTATUS << 8,
> > > > + Q1_GETPRSTATUS = VRB_GETPRSTATUS << 8,
> > > > + Q1_GETPRKEY = VRB_GETPRKEY << 8,
> > > > + Q1_SETMARGINAL = VRB_SETMARGINAL << 8,
> > > > + Q1_UNSETMARGINAL = VRB_UNSETMARGINAL << 8,
> > > > /* byte 2: qualifier 2 */
> > > > Q2_FMT = KEY_FMT << 16,
> > > > Q2_RAW = KEY_RAW << 16,
> >
> > .
> >
More information about the dm-devel
mailing list