* [PATCH 0/4] multipath: fixes for invalid path device handling
@ 2019-02-07 23:52 Benjamin Marzinski
2019-02-07 23:52 ` [PATCH 1/4] multipathd: avoid null pointer dereference in LOG_MSG Benjamin Marzinski
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Benjamin Marzinski @ 2019-02-07 23:52 UTC (permalink / raw)
To: device-mapper development; +Cc: Martin Wilck
I recently got a bug from a user with a bad property
blacklist_exceptions line, where multipath tried to use a zram device,
which caused multipathd to crash. This patch set fixes that crash, and
then adds some additional safeguards to prevent things like this
happening in the future.
The first patch fixes the crash. This crash could be triggered by any
path device that isn't part of a multipath device, and has it's checker
fail to check the path correctly.
The second patch blacklists zram devices specifically. The third patch
fixes up some code weirdness that doesn't really cause any problems,
aside from the messages it prints.
The fourth patch changes how pathinfo works so that it now drops devices
of an unknown type if they can't get a wwid. The biggest reason to do
this is so that multipath won't hold an open file descriptor for these
devices, which are almost definitely not multipathable (and occasionally
send them TUR ioctls, which are bound to fail, since they aren't scsi
devices).
As an aside, is there a good reason why multipath uses a devnode
blacklist to individually blacklist device types, instead of
blacklisting all denodes, and adding exceptions for the device types
that we do support?
Benjamin Marzinski (4):
multipathd: avoid null pointer dereference in LOG_MSG
multipath: blacklist zram devices
multipathd: fix pp->initialized state ping-ponging
multipathd: don't resend change events for unknown devices
libmultipath/blacklist.c | 2 +-
libmultipath/checkers.c | 2 +-
libmultipath/discovery.c | 10 +++++++---
libmultipath/structs.h | 1 +
multipath/multipath.conf.5 | 2 +-
multipathd/main.c | 10 +++++++---
6 files changed, 18 insertions(+), 9 deletions(-)
--
2.17.2
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/4] multipathd: avoid null pointer dereference in LOG_MSG
2019-02-07 23:52 [PATCH 0/4] multipath: fixes for invalid path device handling Benjamin Marzinski
@ 2019-02-07 23:52 ` Benjamin Marzinski
2019-02-08 9:05 ` Martin Wilck
2019-02-07 23:53 ` [PATCH 2/4] multipath: blacklist zram devices Benjamin Marzinski
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Benjamin Marzinski @ 2019-02-07 23:52 UTC (permalink / raw)
To: device-mapper development; +Cc: Martin Wilck
LOG_MSG() will dereference pp->mpp. Commit cb5ec664 added a call to
LOG_MSG() before the check for (!pp->mpp) in check_path. This can cause
multipathd to crash. LOG_MSG() should only be called if pp->mpp is set
and a checker is selected.
Also, checker_message() should fail to a generic message if c->cls isn't
set (which means that a checker hasn't been selected).
Fixes: cb5ec664 (multipathd: check_path: improve logging for "unusable
path" case)
Cc: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/checkers.c | 2 +-
multipathd/main.c | 6 ++++--
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
index 848c4c3..ca95cae 100644
--- a/libmultipath/checkers.c
+++ b/libmultipath/checkers.c
@@ -295,7 +295,7 @@ const char *checker_message(const struct checker *c)
{
int id;
- if (!c || c->msgid < 0 ||
+ if (!c || !c->cls || c->msgid < 0 ||
(c->msgid >= CHECKER_GENERIC_MSGTABLE_SIZE &&
c->msgid < CHECKER_FIRST_MSGID))
goto bad_id;
diff --git a/multipathd/main.c b/multipathd/main.c
index 0e3ac2c..1caa40f 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2017,8 +2017,10 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
}
if (newstate == PATH_WILD || newstate == PATH_UNCHECKED) {
- condlog(2, "%s: unusable path - checker failed", pp->dev);
- LOG_MSG(2, verbosity, pp);
+ condlog(2, "%s: unusable path (%s) - checker failed", pp->dev,
+ checker_state_name(newstate));
+ if (pp->mpp && checker_selected(&pp->checker))
+ LOG_MSG(2, verbosity, pp);
conf = get_multipath_config();
pthread_cleanup_push(put_multipath_config, conf);
pathinfo(pp, conf, 0);
--
2.17.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/4] multipath: blacklist zram devices
2019-02-07 23:52 [PATCH 0/4] multipath: fixes for invalid path device handling Benjamin Marzinski
2019-02-07 23:52 ` [PATCH 1/4] multipathd: avoid null pointer dereference in LOG_MSG Benjamin Marzinski
@ 2019-02-07 23:53 ` Benjamin Marzinski
2019-02-08 9:08 ` Martin Wilck
2019-02-07 23:53 ` [PATCH 3/4] multipathd: fix pp->initialized state ping-ponging Benjamin Marzinski
2019-02-07 23:53 ` [PATCH 4/4] multipathd: don't resend change events for unknown devices Benjamin Marzinski
3 siblings, 1 reply; 10+ messages in thread
From: Benjamin Marzinski @ 2019-02-07 23:53 UTC (permalink / raw)
To: device-mapper development; +Cc: Martin Wilck
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/blacklist.c | 2 +-
multipath/multipath.conf.5 | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/libmultipath/blacklist.c b/libmultipath/blacklist.c
index 709895e..e0d0279 100644
--- a/libmultipath/blacklist.c
+++ b/libmultipath/blacklist.c
@@ -192,7 +192,7 @@ setup_default_blist (struct config * conf)
char * str;
int i;
- str = STRDUP("^(ram|raw|loop|fd|md|dm-|sr|scd|st|dcssblk)[0-9]");
+ str = STRDUP("^(ram|zram|raw|loop|fd|md|dm-|sr|scd|st|dcssblk)[0-9]");
if (!str)
return 1;
if (store_ble(conf->blist_devnode, str, ORIGIN_DEFAULT))
diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
index 88b8edd..0fe8461 100644
--- a/multipath/multipath.conf.5
+++ b/multipath/multipath.conf.5
@@ -1218,7 +1218,7 @@ Regular expression matching the device nodes to be excluded/included.
.RS
.PP
The default \fIblacklist\fR consists of the regular expressions
-"^(ram|raw|loop|fd|md|dm-|sr|scd|st|dcssblk)[0-9]" and
+"^(ram|zram|raw|loop|fd|md|dm-|sr|scd|st|dcssblk)[0-9]" and
"^(td|hd|vd)[a-z]". This causes virtual devices, non-disk devices, and some other
device types to be excluded from multipath handling by default.
.RE
--
2.17.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/4] multipathd: fix pp->initialized state ping-ponging
2019-02-07 23:52 [PATCH 0/4] multipath: fixes for invalid path device handling Benjamin Marzinski
2019-02-07 23:52 ` [PATCH 1/4] multipathd: avoid null pointer dereference in LOG_MSG Benjamin Marzinski
2019-02-07 23:53 ` [PATCH 2/4] multipath: blacklist zram devices Benjamin Marzinski
@ 2019-02-07 23:53 ` Benjamin Marzinski
2019-02-08 9:21 ` Martin Wilck
2019-02-07 23:53 ` [PATCH 4/4] multipathd: don't resend change events for unknown devices Benjamin Marzinski
3 siblings, 1 reply; 10+ messages in thread
From: Benjamin Marzinski @ 2019-02-07 23:53 UTC (permalink / raw)
To: device-mapper development; +Cc: Martin Wilck
When a multipath device fails to get a wwid in pathinfo, it moves to the
INIT_MISSING_UDEV state. After a device in this state sends
retrigger_tries change uevents in check_path(), it moves to the
INIT_FAILED state. However, when check_path() is run on a device in
INIT_FAILED, it can call pathinfo, which will set the path back
into INIT_MISSING_UDEV if it cannot get a wwid. The next call to
check_path() will put the path back into INIT_FAILED. The device will
continue to ping-pong between these states.
To solve this a new pp->initialized state has been added INIT_NEW. New
path devices start in this state, instead of INIT_FAILED. INIT_NEW and
INIT_FAILED are treated exactly the same, with one exception. A device
in INIT_FAILED cannot transition back to INIT_MISSING_UDEV.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/discovery.c | 8 +++++---
libmultipath/structs.h | 1 +
multipathd/main.c | 4 +++-
3 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 1748eeb..6aef188 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1966,8 +1966,10 @@ int pathinfo(struct path *pp, struct config *conf, int mask)
if ((mask & DI_WWID) && !strlen(pp->wwid)) {
get_uid(pp, path_state, pp->udev);
if (!strlen(pp->wwid)) {
- pp->initialized = INIT_MISSING_UDEV;
- pp->tick = conf->retrigger_delay;
+ if (pp->initialized != INIT_FAILED) {
+ pp->initialized = INIT_MISSING_UDEV;
+ pp->tick = conf->retrigger_delay;
+ }
return PATHINFO_OK;
}
else
@@ -2000,7 +2002,7 @@ blank:
* Recoverable error, for example faulty or offline path
*/
pp->chkrstate = pp->state = PATH_DOWN;
- if (pp->initialized == INIT_FAILED)
+ if (pp->initialized == INIT_NEW || pp->initialized == INIT_FAILED)
memset(pp->wwid, 0, WWID_SIZE);
return PATHINFO_OK;
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 375c728..b794b0d 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -202,6 +202,7 @@ enum ghost_delay_states {
};
enum initialized_states {
+ INIT_NEW,
INIT_FAILED,
INIT_MISSING_UDEV,
INIT_REQUESTED_UDEV,
diff --git a/multipathd/main.c b/multipathd/main.c
index 1caa40f..4d0fa8c 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2028,7 +2028,9 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
return 1;
}
if (!pp->mpp) {
- if (!strlen(pp->wwid) && pp->initialized == INIT_FAILED &&
+ if (!strlen(pp->wwid) &&
+ (pp->initialized == INIT_FAILED ||
+ pp->initialized == INIT_NEW) &&
(newstate == PATH_UP || newstate == PATH_GHOST)) {
condlog(2, "%s: add missing path", pp->dev);
conf = get_multipath_config();
--
2.17.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 4/4] multipathd: don't resend change events for unknown devices
2019-02-07 23:52 [PATCH 0/4] multipath: fixes for invalid path device handling Benjamin Marzinski
` (2 preceding siblings ...)
2019-02-07 23:53 ` [PATCH 3/4] multipathd: fix pp->initialized state ping-ponging Benjamin Marzinski
@ 2019-02-07 23:53 ` Benjamin Marzinski
2019-02-08 9:21 ` Martin Wilck
3 siblings, 1 reply; 10+ messages in thread
From: Benjamin Marzinski @ 2019-02-07 23:53 UTC (permalink / raw)
To: device-mapper development; +Cc: Martin Wilck
If multipath fails to get the wwid for a device, and the device is
of an unknown type (pp->bus == SYSFS_BUS_UNDEF), don't send change
events. Instead, assume that the device was not meant to be used
and skip it.
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
libmultipath/discovery.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 6aef188..10bd8cd 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1966,6 +1966,8 @@ int pathinfo(struct path *pp, struct config *conf, int mask)
if ((mask & DI_WWID) && !strlen(pp->wwid)) {
get_uid(pp, path_state, pp->udev);
if (!strlen(pp->wwid)) {
+ if (pp->bus == SYSFS_BUS_UNDEF)
+ return PATHINFO_SKIPPED;
if (pp->initialized != INIT_FAILED) {
pp->initialized = INIT_MISSING_UDEV;
pp->tick = conf->retrigger_delay;
--
2.17.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/4] multipathd: avoid null pointer dereference in LOG_MSG
2019-02-07 23:52 ` [PATCH 1/4] multipathd: avoid null pointer dereference in LOG_MSG Benjamin Marzinski
@ 2019-02-08 9:05 ` Martin Wilck
2019-02-08 17:28 ` [PATCH 1/4] multipathd: avoid null pointer dereference in LOG_MSG\ Benjamin Marzinski
0 siblings, 1 reply; 10+ messages in thread
From: Martin Wilck @ 2019-02-08 9:05 UTC (permalink / raw)
To: Benjamin Marzinski, device-mapper development
On Thu, 2019-02-07 at 17:52 -0600, Benjamin Marzinski wrote:
> LOG_MSG() will dereference pp->mpp. Commit cb5ec664 added a call to
> LOG_MSG() before the check for (!pp->mpp) in check_path. This can
> cause
> multipathd to crash. LOG_MSG() should only be called if pp->mpp is
> set
> and a checker is selected.
>
> Also, checker_message() should fail to a generic message if c->cls
> isn't
> set (which means that a checker hasn't been selected).
>
> Fixes: cb5ec664 (multipathd: check_path: improve logging for
> "unusable
> path" case)
> Cc: Martin Wilck <mwilck@suse.com>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Thanks a lot, but could we do the below instead? IMO it's better to
avoid similar errors in the future.
Martin
From e5bef42f8f9d2aef9406873f515a45914c1aa251 Mon Sep 17 00:00:00 2001
From: Benjamin Marzinski <bmarzins@redhat.com>
Date: Thu, 7 Feb 2019 17:52:59 -0600
Subject: [PATCH] multipathd: avoid null pointer dereference in LOG_MSG
LOG_MSG() will dereference pp->mpp. Commit cb5ec664 added a call to
LOG_MSG() before the check for (!pp->mpp) in check_path. This can cause
multipathd to crash. LOG_MSG() should check the fields before dereferencing
them. Make checker_selected() an inline function to allow the compiler
to optimize away the usually redundant test "if (&checker->pp != NULL)".
Also, checker_message() should fail to a generic message if c->cls isn't
set (which means that a checker hasn't been selected).
Fixes: cb5ec664 (multipathd: check_path: improve logging for "unusable
path" case)
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
libmultipath/checkers.c | 9 +--------
libmultipath/checkers.h | 6 +++++-
multipathd/main.c | 6 ++++--
3 files changed, 10 insertions(+), 11 deletions(-)
diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
index 848c4c34..f4fdcae9 100644
--- a/libmultipath/checkers.c
+++ b/libmultipath/checkers.c
@@ -261,13 +261,6 @@ int checker_check (struct checker * c, int path_state)
return r;
}
-int checker_selected(const struct checker *c)
-{
- if (!c)
- return 0;
- return c->cls != NULL;
-}
-
const char *checker_name(const struct checker *c)
{
if (!c || !c->cls)
@@ -295,7 +288,7 @@ const char *checker_message(const struct checker *c)
{
int id;
- if (!c || c->msgid < 0 ||
+ if (!c || !c->cls || c->msgid < 0 ||
(c->msgid >= CHECKER_GENERIC_MSGTABLE_SIZE &&
c->msgid < CHECKER_FIRST_MSGID))
goto bad_id;
diff --git a/libmultipath/checkers.h b/libmultipath/checkers.h
index b2e8f9aa..dab197f9 100644
--- a/libmultipath/checkers.h
+++ b/libmultipath/checkers.h
@@ -129,6 +129,11 @@ struct checker {
you want to stuff data in. */
};
+static inline int checker_selected(const struct checker *c)
+{
+ return c != NULL && c->cls != NULL;
+}
+
const char *checker_state_name(int);
int init_checkers(const char *);
void cleanup_checkers (void);
@@ -142,7 +147,6 @@ void checker_set_fd (struct checker *, int);
void checker_enable (struct checker *);
void checker_disable (struct checker *);
int checker_check (struct checker *, int);
-int checker_selected(const struct checker *);
int checker_is_sync(const struct checker *);
const char *checker_name (const struct checker *);
/*
diff --git a/multipathd/main.c b/multipathd/main.c
index d1a4f629..d1dd286c 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -92,7 +92,8 @@ static int use_watchdog;
#define LOG_MSG(lvl, verb, pp) \
do { \
- if (lvl <= verb) { \
+ if (pp->mpp && checker_selected(&pp->checker) && \
+ lvl <= verb) { \
if (pp->offline) \
condlog(lvl, "%s: %s - path offline", \
pp->mpp->alias, pp->dev); \
@@ -2017,7 +2018,8 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
}
if (newstate == PATH_WILD || newstate == PATH_UNCHECKED) {
- condlog(2, "%s: unusable path - checker failed", pp->dev);
+ condlog(2, "%s: unusable path (%s) - checker failed",
+ pp->dev, checker_state_name(newstate));
LOG_MSG(2, verbosity, pp);
conf = get_multipath_config();
pthread_cleanup_push(put_multipath_config, conf);
--
2.20.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/4] multipath: blacklist zram devices
2019-02-07 23:53 ` [PATCH 2/4] multipath: blacklist zram devices Benjamin Marzinski
@ 2019-02-08 9:08 ` Martin Wilck
0 siblings, 0 replies; 10+ messages in thread
From: Martin Wilck @ 2019-02-08 9:08 UTC (permalink / raw)
To: Benjamin Marzinski, device-mapper development
On Thu, 2019-02-07 at 17:53 -0600, Benjamin Marzinski wrote:
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
> libmultipath/blacklist.c | 2 +-
> multipath/multipath.conf.5 | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
Ack, but - how come that this device hadn't been auto-blacklisted
by the "udev property missing" test?
Martin
>
> diff --git a/libmultipath/blacklist.c b/libmultipath/blacklist.c
> index 709895e..e0d0279 100644
> --- a/libmultipath/blacklist.c
> +++ b/libmultipath/blacklist.c
> @@ -192,7 +192,7 @@ setup_default_blist (struct config * conf)
> char * str;
> int i;
>
> - str = STRDUP("^(ram|raw|loop|fd|md|dm-|sr|scd|st|dcssblk)[0-
> 9]");
> + str = STRDUP("^(ram|zram|raw|loop|fd|md|dm-
> |sr|scd|st|dcssblk)[0-9]");
> if (!str)
> return 1;
> if (store_ble(conf->blist_devnode, str, ORIGIN_DEFAULT))
> diff --git a/multipath/multipath.conf.5 b/multipath/multipath.conf.5
> index 88b8edd..0fe8461 100644
> --- a/multipath/multipath.conf.5
> +++ b/multipath/multipath.conf.5
> @@ -1218,7 +1218,7 @@ Regular expression matching the device nodes to
> be excluded/included.
> .RS
> .PP
> The default \fIblacklist\fR consists of the regular expressions
> -"^(ram|raw|loop|fd|md|dm-|sr|scd|st|dcssblk)[0-9]" and
> +"^(ram|zram|raw|loop|fd|md|dm-|sr|scd|st|dcssblk)[0-9]" and
> "^(td|hd|vd)[a-z]". This causes virtual devices, non-disk devices,
> and some other
> device types to be excluded from multipath handling by default.
> .RE
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/4] multipathd: fix pp->initialized state ping-ponging
2019-02-07 23:53 ` [PATCH 3/4] multipathd: fix pp->initialized state ping-ponging Benjamin Marzinski
@ 2019-02-08 9:21 ` Martin Wilck
0 siblings, 0 replies; 10+ messages in thread
From: Martin Wilck @ 2019-02-08 9:21 UTC (permalink / raw)
To: Benjamin Marzinski, device-mapper development
On Thu, 2019-02-07 at 17:53 -0600, Benjamin Marzinski wrote:
> When a multipath device fails to get a wwid in pathinfo, it moves to
> the
> INIT_MISSING_UDEV state. After a device in this state sends
> retrigger_tries change uevents in check_path(), it moves to the
> INIT_FAILED state. However, when check_path() is run on a device in
> INIT_FAILED, it can call pathinfo, which will set the path back
> into INIT_MISSING_UDEV if it cannot get a wwid. The next call to
> check_path() will put the path back into INIT_FAILED. The device
> will
> continue to ping-pong between these states.
>
> To solve this a new pp->initialized state has been added
> INIT_NEW. New
> path devices start in this state, instead of INIT_FAILED. INIT_NEW
> and
> INIT_FAILED are treated exactly the same, with one exception. A
> device
> in INIT_FAILED cannot transition back to INIT_MISSING_UDEV.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>
> ---
> libmultipath/discovery.c | 8 +++++---
> libmultipath/structs.h | 1 +
> multipathd/main.c | 4 +++-
> 3 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 1748eeb..6aef188 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1966,8 +1966,10 @@ int pathinfo(struct path *pp, struct config
> *conf, int mask)
> if ((mask & DI_WWID) && !strlen(pp->wwid)) {
> get_uid(pp, path_state, pp->udev);
> if (!strlen(pp->wwid)) {
> - pp->initialized = INIT_MISSING_UDEV;
> - pp->tick = conf->retrigger_delay;
> + if (pp->initialized != INIT_FAILED) {
> + pp->initialized = INIT_MISSING_UDEV;
> + pp->tick = conf->retrigger_delay;
> + }
> return PATHINFO_OK;
> }
> else
> @@ -2000,7 +2002,7 @@ blank:
> * Recoverable error, for example faulty or offline path
> */
> pp->chkrstate = pp->state = PATH_DOWN;
> - if (pp->initialized == INIT_FAILED)
> + if (pp->initialized == INIT_NEW || pp->initialized ==
> INIT_FAILED)
> memset(pp->wwid, 0, WWID_SIZE);
>
> return PATHINFO_OK;
> diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> index 375c728..b794b0d 100644
> --- a/libmultipath/structs.h
> +++ b/libmultipath/structs.h
> @@ -202,6 +202,7 @@ enum ghost_delay_states {
> };
>
> enum initialized_states {
> + INIT_NEW,
> INIT_FAILED,
> INIT_MISSING_UDEV,
> INIT_REQUESTED_UDEV,
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 1caa40f..4d0fa8c 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2028,7 +2028,9 @@ check_path (struct vectors * vecs, struct path
> * pp, int ticks)
> return 1;
> }
> if (!pp->mpp) {
> - if (!strlen(pp->wwid) && pp->initialized == INIT_FAILED
> &&
> + if (!strlen(pp->wwid) &&
> + (pp->initialized == INIT_FAILED ||
> + pp->initialized == INIT_NEW) &&
> (newstate == PATH_UP || newstate == PATH_GHOST)) {
> condlog(2, "%s: add missing path", pp->dev);
> conf = get_multipath_config();
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 4/4] multipathd: don't resend change events for unknown devices
2019-02-07 23:53 ` [PATCH 4/4] multipathd: don't resend change events for unknown devices Benjamin Marzinski
@ 2019-02-08 9:21 ` Martin Wilck
0 siblings, 0 replies; 10+ messages in thread
From: Martin Wilck @ 2019-02-08 9:21 UTC (permalink / raw)
To: Benjamin Marzinski, device-mapper development
On Thu, 2019-02-07 at 17:53 -0600, Benjamin Marzinski wrote:
> If multipath fails to get the wwid for a device, and the device is
> of an unknown type (pp->bus == SYSFS_BUS_UNDEF), don't send change
> events. Instead, assume that the device was not meant to be used
> and skip it.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>
> ---
> libmultipath/discovery.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 6aef188..10bd8cd 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1966,6 +1966,8 @@ int pathinfo(struct path *pp, struct config
> *conf, int mask)
> if ((mask & DI_WWID) && !strlen(pp->wwid)) {
> get_uid(pp, path_state, pp->udev);
> if (!strlen(pp->wwid)) {
> + if (pp->bus == SYSFS_BUS_UNDEF)
> + return PATHINFO_SKIPPED;
> if (pp->initialized != INIT_FAILED) {
> pp->initialized = INIT_MISSING_UDEV;
> pp->tick = conf->retrigger_delay;
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/4] multipathd: avoid null pointer dereference in LOG_MSG\
2019-02-08 9:05 ` Martin Wilck
@ 2019-02-08 17:28 ` Benjamin Marzinski
0 siblings, 0 replies; 10+ messages in thread
From: Benjamin Marzinski @ 2019-02-08 17:28 UTC (permalink / raw)
To: Martin Wilck; +Cc: device-mapper development
On Fri, Feb 08, 2019 at 10:05:51AM +0100, Martin Wilck wrote:
> On Thu, 2019-02-07 at 17:52 -0600, Benjamin Marzinski wrote:
> > LOG_MSG() will dereference pp->mpp. Commit cb5ec664 added a call to
> > LOG_MSG() before the check for (!pp->mpp) in check_path. This can
> > cause
> > multipathd to crash. LOG_MSG() should only be called if pp->mpp is
> > set
> > and a checker is selected.
> >
> > Also, checker_message() should fail to a generic message if c->cls
> > isn't
> > set (which means that a checker hasn't been selected).
> >
> > Fixes: cb5ec664 (multipathd: check_path: improve logging for
> > "unusable
> > path" case)
> > Cc: Martin Wilck <mwilck@suse.com>
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
>
> Thanks a lot, but could we do the below instead? IMO it's better to
> avoid similar errors in the future.
>
> Martin
Sure. I can resend the series with this patch instead.
-Ben
>
>
> >From e5bef42f8f9d2aef9406873f515a45914c1aa251 Mon Sep 17 00:00:00 2001
> From: Benjamin Marzinski <bmarzins@redhat.com>
> Date: Thu, 7 Feb 2019 17:52:59 -0600
> Subject: [PATCH] multipathd: avoid null pointer dereference in LOG_MSG
>
> LOG_MSG() will dereference pp->mpp. Commit cb5ec664 added a call to
> LOG_MSG() before the check for (!pp->mpp) in check_path. This can cause
> multipathd to crash. LOG_MSG() should check the fields before dereferencing
> them. Make checker_selected() an inline function to allow the compiler
> to optimize away the usually redundant test "if (&checker->pp != NULL)".
>
> Also, checker_message() should fail to a generic message if c->cls isn't
> set (which means that a checker hasn't been selected).
>
> Fixes: cb5ec664 (multipathd: check_path: improve logging for "unusable
> path" case)
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
> libmultipath/checkers.c | 9 +--------
> libmultipath/checkers.h | 6 +++++-
> multipathd/main.c | 6 ++++--
> 3 files changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/libmultipath/checkers.c b/libmultipath/checkers.c
> index 848c4c34..f4fdcae9 100644
> --- a/libmultipath/checkers.c
> +++ b/libmultipath/checkers.c
> @@ -261,13 +261,6 @@ int checker_check (struct checker * c, int path_state)
> return r;
> }
>
> -int checker_selected(const struct checker *c)
> -{
> - if (!c)
> - return 0;
> - return c->cls != NULL;
> -}
> -
> const char *checker_name(const struct checker *c)
> {
> if (!c || !c->cls)
> @@ -295,7 +288,7 @@ const char *checker_message(const struct checker *c)
> {
> int id;
>
> - if (!c || c->msgid < 0 ||
> + if (!c || !c->cls || c->msgid < 0 ||
> (c->msgid >= CHECKER_GENERIC_MSGTABLE_SIZE &&
> c->msgid < CHECKER_FIRST_MSGID))
> goto bad_id;
> diff --git a/libmultipath/checkers.h b/libmultipath/checkers.h
> index b2e8f9aa..dab197f9 100644
> --- a/libmultipath/checkers.h
> +++ b/libmultipath/checkers.h
> @@ -129,6 +129,11 @@ struct checker {
> you want to stuff data in. */
> };
>
> +static inline int checker_selected(const struct checker *c)
> +{
> + return c != NULL && c->cls != NULL;
> +}
> +
> const char *checker_state_name(int);
> int init_checkers(const char *);
> void cleanup_checkers (void);
> @@ -142,7 +147,6 @@ void checker_set_fd (struct checker *, int);
> void checker_enable (struct checker *);
> void checker_disable (struct checker *);
> int checker_check (struct checker *, int);
> -int checker_selected(const struct checker *);
> int checker_is_sync(const struct checker *);
> const char *checker_name (const struct checker *);
> /*
> diff --git a/multipathd/main.c b/multipathd/main.c
> index d1a4f629..d1dd286c 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -92,7 +92,8 @@ static int use_watchdog;
>
> #define LOG_MSG(lvl, verb, pp) \
> do { \
> - if (lvl <= verb) { \
> + if (pp->mpp && checker_selected(&pp->checker) && \
> + lvl <= verb) { \
> if (pp->offline) \
> condlog(lvl, "%s: %s - path offline", \
> pp->mpp->alias, pp->dev); \
> @@ -2017,7 +2018,8 @@ check_path (struct vectors * vecs, struct path * pp, int ticks)
> }
>
> if (newstate == PATH_WILD || newstate == PATH_UNCHECKED) {
> - condlog(2, "%s: unusable path - checker failed", pp->dev);
> + condlog(2, "%s: unusable path (%s) - checker failed",
> + pp->dev, checker_state_name(newstate));
> LOG_MSG(2, verbosity, pp);
> conf = get_multipath_config();
> pthread_cleanup_push(put_multipath_config, conf);
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-02-08 17:28 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-02-07 23:52 [PATCH 0/4] multipath: fixes for invalid path device handling Benjamin Marzinski
2019-02-07 23:52 ` [PATCH 1/4] multipathd: avoid null pointer dereference in LOG_MSG Benjamin Marzinski
2019-02-08 9:05 ` Martin Wilck
2019-02-08 17:28 ` [PATCH 1/4] multipathd: avoid null pointer dereference in LOG_MSG\ Benjamin Marzinski
2019-02-07 23:53 ` [PATCH 2/4] multipath: blacklist zram devices Benjamin Marzinski
2019-02-08 9:08 ` Martin Wilck
2019-02-07 23:53 ` [PATCH 3/4] multipathd: fix pp->initialized state ping-ponging Benjamin Marzinski
2019-02-08 9:21 ` Martin Wilck
2019-02-07 23:53 ` [PATCH 4/4] multipathd: don't resend change events for unknown devices Benjamin Marzinski
2019-02-08 9:21 ` Martin Wilck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox