[dm-devel] [PATCH 0/3] Minor multipath-tools patches

[dm-devel] [PATCH 0/3] Minor multipath-tools patches

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Various minor changes that resulted from recent work on lixiaokeng's
reports, collected here for review.

Martin Wilck (3):
  multipathd: avoid crash in uevent_cleanup()
  multipathd: ev_add_path: fail if add_map_with_path() fails
  libmultipath: check return value of udev_device_get_devnum()

 libmultipath/discovery.c | 3 +++
 libmultipath/uevent.c    | 7 ++++---
 multipathd/main.c        | 2 +-
 3 files changed, 8 insertions(+), 4 deletions(-)

-- 
2.29.2


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 1/3] multipathd: avoid crash in uevent_cleanup()

[mwilck]

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

[dm-devel] [PATCH 2/3] multipathd: ev_add_path: fail if add_map_with_path() fails

[mwilck]

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

[dm-devel] [PATCH 3/3] libmultipath: check return value of udev_device_get_devnum()

[mwilck]

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

Re: [dm-devel] [PATCH 2/3] multipathd: ev_add_path: fail if add_map_with_path() fails

[Benjamin Marzinski]

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

Re: [dm-devel] [PATCH 3/3] libmultipath: check return value of udev_device_get_devnum()

[Benjamin Marzinski]

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