* [dm-devel] [PATCH 1/3] multipathd: avoid crash in uevent_cleanup()
2021-02-02 21:41 [dm-devel] [PATCH 0/3] Minor multipath-tools patches mwilck
@ 2021-02-02 21:41 ` mwilck
2021-02-02 21:41 ` [dm-devel] [PATCH 2/3] multipathd: ev_add_path: fail if add_map_with_path() fails mwilck
2021-02-02 21:41 ` [dm-devel] [PATCH 3/3] libmultipath: check return value of udev_device_get_devnum() mwilck
2 siblings, 0 replies; 6+ messages in thread
From: mwilck @ 2021-02-02 21:41 UTC (permalink / raw)
To: Benjamin Marzinski, Christophe Varoqui, lixiaokeng; +Cc: dm-devel, Martin Wilck
From: Martin Wilck <mwilck@suse.com>
Crashes have been observed in the unwinder stack of uevent_listen().
This can only be explained by "udev" not being a valid object at that
time. Be sure to pass a valid pointer, and don't call udev_unref() if
it has been set to NULL already.
I'm not quite sure how this would come to pass, as we join the threads
before setting udev to NULL, but this is unwinder code, so I guess it
might actually be executed after the thread has terminated.
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
libmultipath/uevent.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index d3061bf..4e662ff 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -397,10 +397,11 @@ service_uevq(struct list_head *tmpq)
static void uevent_cleanup(void *arg)
{
- struct udev *udev = arg;
+ struct udev **pudev = arg;
+ if (*pudev)
+ udev_unref(*pudev);
condlog(3, "Releasing uevent_listen() resources");
- udev_unref(udev);
}
static void monitor_cleanup(void *arg)
@@ -560,7 +561,7 @@ int uevent_listen(struct udev *udev)
return 1;
}
udev_ref(udev);
- pthread_cleanup_push(uevent_cleanup, udev);
+ pthread_cleanup_push(uevent_cleanup, &udev);
monitor = udev_monitor_new_from_netlink(udev, "udev");
if (!monitor) {
--
2.29.2
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread* [dm-devel] [PATCH 2/3] multipathd: ev_add_path: fail if add_map_with_path() fails
2021-02-02 21:41 [dm-devel] [PATCH 0/3] Minor multipath-tools patches mwilck
2021-02-02 21:41 ` [dm-devel] [PATCH 1/3] multipathd: avoid crash in uevent_cleanup() mwilck
@ 2021-02-02 21:41 ` mwilck
2021-02-03 19:30 ` Benjamin Marzinski
2021-02-02 21:41 ` [dm-devel] [PATCH 3/3] libmultipath: check return value of udev_device_get_devnum() mwilck
2 siblings, 1 reply; 6+ messages in thread
From: mwilck @ 2021-02-02 21:41 UTC (permalink / raw)
To: Benjamin Marzinski, Christophe Varoqui, lixiaokeng; +Cc: dm-devel, Martin Wilck
From: Martin Wilck <mwilck@suse.com>
If start_waiter was set before and the "rescan" label was used,
we may try to set up an empty/invalid map.
Always fail if add_map_with_path() isn't successful.
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
multipathd/main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/multipathd/main.c b/multipathd/main.c
index 43d7768..425492a 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -1028,7 +1028,7 @@ rescan:
*/
start_waiter = 1;
}
- if (!start_waiter)
+ else
goto fail; /* leave path added to pathvec */
}
--
2.29.2
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [dm-devel] [PATCH 2/3] multipathd: ev_add_path: fail if add_map_with_path() fails
2021-02-02 21:41 ` [dm-devel] [PATCH 2/3] multipathd: ev_add_path: fail if add_map_with_path() fails mwilck
@ 2021-02-03 19:30 ` Benjamin Marzinski
0 siblings, 0 replies; 6+ messages in thread
From: Benjamin Marzinski @ 2021-02-03 19:30 UTC (permalink / raw)
To: mwilck; +Cc: lixiaokeng, dm-devel
On Tue, Feb 02, 2021 at 10:41:30PM +0100, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
>
> If start_waiter was set before and the "rescan" label was used,
> we may try to set up an empty/invalid map.
> Always fail if add_map_with_path() isn't successful.
>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
This patch looks fine, but I don't think that there was any risk before.
I don't see how you could loop back to rescan with mpp == NULL, which
is what would need to happen before you could ever run this code.
At any rate
Reviewed-by: Benjamin Marzinski <bmarzin@redhat.com>
> ---
> multipathd/main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 43d7768..425492a 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -1028,7 +1028,7 @@ rescan:
> */
> start_waiter = 1;
> }
> - if (!start_waiter)
> + else
> goto fail; /* leave path added to pathvec */
> }
>
> --
> 2.29.2
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* [dm-devel] [PATCH 3/3] libmultipath: check return value of udev_device_get_devnum()
2021-02-02 21:41 [dm-devel] [PATCH 0/3] Minor multipath-tools patches mwilck
2021-02-02 21:41 ` [dm-devel] [PATCH 1/3] multipathd: avoid crash in uevent_cleanup() mwilck
2021-02-02 21:41 ` [dm-devel] [PATCH 2/3] multipathd: ev_add_path: fail if add_map_with_path() fails mwilck
@ 2021-02-02 21:41 ` mwilck
2021-02-03 23:21 ` Benjamin Marzinski
2 siblings, 1 reply; 6+ messages in thread
From: mwilck @ 2021-02-02 21:41 UTC (permalink / raw)
To: Benjamin Marzinski, Christophe Varoqui, lixiaokeng; +Cc: dm-devel, Martin Wilck
From: Martin Wilck <mwilck@suse.com>
udev_device_get_devnum() may fail, in which case it returns
makedev(0, 0).
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
libmultipath/discovery.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 921025d..15cf641 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -1659,6 +1659,9 @@ common_sysfs_pathinfo (struct path * pp)
return PATHINFO_FAILED;
}
devt = udev_device_get_devnum(pp->udev);
+ if (major(devt) == 0 && minor(devt) == 0)
+ return PATHINFO_FAILED;
+
snprintf(pp->dev_t, BLK_DEV_SIZE, "%d:%d", major(devt), minor(devt));
condlog(4, "%s: dev_t = %s", pp->dev, pp->dev_t);
--
2.29.2
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [dm-devel] [PATCH 3/3] libmultipath: check return value of udev_device_get_devnum()
2021-02-02 21:41 ` [dm-devel] [PATCH 3/3] libmultipath: check return value of udev_device_get_devnum() mwilck
@ 2021-02-03 23:21 ` Benjamin Marzinski
0 siblings, 0 replies; 6+ messages in thread
From: Benjamin Marzinski @ 2021-02-03 23:21 UTC (permalink / raw)
To: mwilck; +Cc: lixiaokeng, dm-devel
On Tue, Feb 02, 2021 at 10:41:31PM +0100, mwilck@suse.com wrote:
> From: Martin Wilck <mwilck@suse.com>
>
> udev_device_get_devnum() may fail, in which case it returns
> makedev(0, 0).
>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
> libmultipath/discovery.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
> index 921025d..15cf641 100644
> --- a/libmultipath/discovery.c
> +++ b/libmultipath/discovery.c
> @@ -1659,6 +1659,9 @@ common_sysfs_pathinfo (struct path * pp)
> return PATHINFO_FAILED;
> }
> devt = udev_device_get_devnum(pp->udev);
> + if (major(devt) == 0 && minor(devt) == 0)
> + return PATHINFO_FAILED;
> +
> snprintf(pp->dev_t, BLK_DEV_SIZE, "%d:%d", major(devt), minor(devt));
>
> condlog(4, "%s: dev_t = %s", pp->dev, pp->dev_t);
> --
> 2.29.2
--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
^ permalink raw reply [flat|nested] 6+ messages in thread