[6/6] libmpathutil: remove systemd_service_enabled()

Commit Message

Martin Wilck Oct. 24, 2023, 4:49 p.m. UTC

From: Martin Wilck <[e-mail]>

Since  c203929 ("multipathd.service: add dependency on
systemd-udevd-kernel.socket"), multipathd will start early during boot.
Moreover, we recommend using socket activation for multipathd,
and if multipathd.socket is enabled, the __mpath_connect()
check will succeed anyway.

The systemd_service_enabled() test was just "good enough" for
standard situations but never robust; it checked for multipathd.wants in
"some" directory, which might or might not be the current target,
and it would return true even of multipathd.service was masked.

Remove this test.

Signed-off-by: Martin Wilck <[e-mail]>
---
 libmpathutil/libmpathutil.version | 17 +++------
 libmpathutil/util.c               | 58 -------------------------------
 libmpathutil/util.h               |  1 -
 libmultipath/valid.c              | 16 ++-------
 tests/valid.c                     | 24 ++-----------
 5 files changed, 9 insertions(+), 107 deletions(-)

Comments

Benjamin Marzinski Oct. 25, 2023, 11:58 p.m. UTC | #1

On Tue, Oct 24, 2023 at 06:49:37PM +0200, [e-mail] wrote:
> From: Martin Wilck <[e-mail]>
> 
> Since  c203929 ("multipathd.service: add dependency on
> systemd-udevd-kernel.socket"), multipathd will start early during boot.
> Moreover, we recommend using socket activation for multipathd,
> and if multipathd.socket is enabled, the __mpath_connect()
> check will succeed anyway.
> 
> The systemd_service_enabled() test was just "good enough" for
> standard situations but never robust; it checked for multipathd.wants in
> "some" directory, which might or might not be the current target,
> and it would return true even of multipathd.service was masked.
> 
> Remove this test.
> 
> Signed-off-by: Martin Wilck <[e-mail]>

I'd be lying if I said I had no worries at all about this. Removing this
check means that if someone isn't using socket activation, and
multipathd is temporarily down, and a new device appears, it will always
be marked as not claimed by multipath. This could cause the device to be
grabbed by LVM, and not multipathed. This is probably just may paranoia
talking, since the chance of this happening in the real world seems
pretty low. So,

Reviewed-by: Benjamin Marzinski <[e-mail]>

> ---
>  libmpathutil/libmpathutil.version | 17 +++------
>  libmpathutil/util.c               | 58 -------------------------------
>  libmpathutil/util.h               |  1 -
>  libmultipath/valid.c              | 16 ++-------
>  tests/valid.c                     | 24 ++-----------
>  5 files changed, 9 insertions(+), 107 deletions(-)
> 
> diff --git a/libmpathutil/libmpathutil.version b/libmpathutil/libmpathutil.version
> index 6ebb718..15ff467 100644
> --- a/libmpathutil/libmpathutil.version
> +++ b/libmpathutil/libmpathutil.version
> @@ -93,12 +93,15 @@ local:
>  };
>  
>  /* symbols referenced internally by libmultipath */
> -LIBMPATHUTIL_1.0 {
> +LIBMPATHUTIL_2.0 {
>  	alloc_bitfield;
>  	__append_strbuf_str;
>  	append_strbuf_quoted;
>  	basenamecpy;
> +	cleanup_fd_ptr;
>  	cleanup_free_ptr;
> +	cleanup_vector_free;
> +	cleanup_fclose;
>  	filepresent;
>  	find_keyword;
>  	free_keywords;
> @@ -120,21 +123,9 @@ LIBMPATHUTIL_1.0 {
>  	snprint_keyword;
>  	steal_strbuf_str;
>  	strlcat;
> -	systemd_service_enabled;
>  	validate_config_strvec;
>  	vector_find_or_add_slot;
>  	vector_insert_slot;
>  	vector_move_up;
>  	vector_sort;
>  };
> -
> -LIBMPATHUTIL_1.1 {
> -global:
> -	cleanup_fd_ptr;
> -} LIBMPATHUTIL_1.0;
> -
> -LIBMPATHUTIL_1.2 {
> -global:
> -	cleanup_vector_free;
> -	cleanup_fclose;
> -} LIBMPATHUTIL_1.0;
> diff --git a/libmpathutil/util.c b/libmpathutil/util.c
> index 92f25a5..9d147fc 100644
> --- a/libmpathutil/util.c
> +++ b/libmpathutil/util.c
> @@ -213,64 +213,6 @@ setup_thread_attr(pthread_attr_t *attr, size_t stacksize, int detached)
>  	}
>  }
>  
> -int systemd_service_enabled_in(const char *dev, const char *prefix)
> -{
> -	static const char service[] = "multipathd.service";
> -	char path[PATH_MAX], file[PATH_MAX];
> -	DIR *dirfd;
> -	struct dirent *d;
> -	int found = 0;
> -
> -	if (safe_sprintf(path, "%s/systemd/system", prefix))
> -		return 0;
> -
> -	condlog(3, "%s: checking for %s in %s", dev, service, path);
> -
> -	dirfd = opendir(path);
> -	if (dirfd == NULL)
> -		return 0;
> -
> -	while ((d = readdir(dirfd)) != NULL) {
> -		char *p;
> -		struct stat stbuf;
> -
> -		if ((strcmp(d->d_name,".") == 0) ||
> -		    (strcmp(d->d_name,"..") == 0))
> -			continue;
> -
> -		if (strlen(d->d_name) < 6)
> -			continue;
> -
> -		p = d->d_name + strlen(d->d_name) - 6;
> -		if (strcmp(p, ".wants"))
> -			continue;
> -		if (!safe_sprintf(file, "%s/%s/%s",
> -				  path, d->d_name, service)
> -		    && stat(file, &stbuf) == 0) {
> -			condlog(3, "%s: found %s", dev, file);
> -			found++;
> -			break;
> -		}
> -	}
> -	closedir(dirfd);
> -
> -	return found;
> -}
> -
> -int systemd_service_enabled(const char *dev)
> -{
> -	int found = 0;
> -
> -	found = systemd_service_enabled_in(dev, "/etc");
> -	if (!found)
> -		found = systemd_service_enabled_in(dev, "/usr/lib");
> -	if (!found)
> -		found = systemd_service_enabled_in(dev, "/lib");
> -	if (!found)
> -		found = systemd_service_enabled_in(dev, "/run");
> -	return found;
> -}
> -
>  static int _linux_version_code;
>  static pthread_once_t _lvc_initialized = PTHREAD_ONCE_INIT;
>  
> diff --git a/libmpathutil/util.h b/libmpathutil/util.h
> index 99a471d..de9fcfd 100644
> --- a/libmpathutil/util.h
> +++ b/libmpathutil/util.h
> @@ -21,7 +21,6 @@ size_t strlcat(char * restrict dst, const char * restrict src, size_t size);
>  dev_t parse_devt(const char *dev_t);
>  char *convert_dev(char *dev, int is_path_device);
>  void setup_thread_attr(pthread_attr_t *attr, size_t stacksize, int detached);
> -int systemd_service_enabled(const char *dev);
>  int get_linux_version_code(void);
>  int safe_write(int fd, const void *buf, size_t count);
>  void set_max_fds(rlim_t max_fds);
> diff --git a/libmultipath/valid.c b/libmultipath/valid.c
> index d4dae3e..f223778 100644
> --- a/libmultipath/valid.c
> +++ b/libmultipath/valid.c
> @@ -314,23 +314,11 @@ is_path_valid(const char *name, struct config *conf, struct path *pp,
>  		return PATH_IS_VALID_NO_CHECK;
>  	}
>  
> -	/*
> -	 * "multipath -u" may be run before the daemon is started. In this
> -	 * case, systemd might own the socket but might delay multipathd
> -	 * startup until some other unit (udev settle!)  has finished
> -	 * starting. With many LUNs, the listen backlog may be exceeded, which
> -	 * would cause connect() to block. This causes udev workers calling
> -	 * "multipath -u" to hang, and thus creates a deadlock, until "udev
> -	 * settle" times out.  To avoid this, call connect() in non-blocking
> -	 * mode here, and take EAGAIN as indication for a filled-up systemd
> -	 * backlog.
> -	 */
> -
>  	if (check_multipathd) {
>  		fd = __mpath_connect(1);
>  		if (fd < 0) {
> -			if (errno != EAGAIN && !systemd_service_enabled(name)) {
> -				condlog(3, "multipathd not running or enabled");
> +			if (errno != EAGAIN) {
> +				condlog(3, "multipathd not running");
>  				return PATH_IS_NOT_VALID;
>  			}
>  		} else
> diff --git a/tests/valid.c b/tests/valid.c
> index 7032932..18a5a7b 100644
> --- a/tests/valid.c
> +++ b/tests/valid.c
> @@ -62,11 +62,6 @@ int __wrap___mpath_connect(int nonblocking)
>  	return -1;
>  }
>  
> -int __wrap_systemd_service_enabled(const char *dev)
> -{
> -	return (int)mock_type(bool);
> -}
> -
>  /* There's no point in checking the return value here */
>  int __wrap_mpath_disconnect(int fd)
>  {
> @@ -216,7 +211,6 @@ enum {
>  enum {
>  	CHECK_MPATHD_RUNNING,
>  	CHECK_MPATHD_EAGAIN,
> -	CHECK_MPATHD_ENABLED,
>  	CHECK_MPATHD_SKIP,
>  };
>  
> @@ -232,11 +226,8 @@ static void setup_passing(char *name, char *wwid, unsigned int check_multipathd,
>  	else if (check_multipathd == CHECK_MPATHD_EAGAIN) {
>  		will_return(__wrap___mpath_connect, false);
>  		will_return(__wrap___mpath_connect, EAGAIN);
> -	} else if (check_multipathd == CHECK_MPATHD_ENABLED) {
> -		will_return(__wrap___mpath_connect, false);
> -		will_return(__wrap___mpath_connect, ECONNREFUSED);
> -		will_return(__wrap_systemd_service_enabled, true);
>  	}
> +
>  	/* nothing for CHECK_MPATHD_SKIP */
>  	if (stage == STAGE_CHECK_MULTIPATHD)
>  		return;
> @@ -342,19 +333,10 @@ static void test_check_multipathd(void **state)
>  	will_return(__wrap_sysfs_is_multipathed, false);
>  	will_return(__wrap___mpath_connect, false);
>  	will_return(__wrap___mpath_connect, ECONNREFUSED);
> -	will_return(__wrap_systemd_service_enabled, false);
> +
>  	assert_int_equal(is_path_valid(name, &conf, &pp, true),
>  			 PATH_IS_NOT_VALID);
>  	assert_string_equal(pp.dev, name);
> -	/* test pass because service is enabled. fail getting udev */
> -	memset(&pp, 0, sizeof(pp));
> -	setup_passing(name, NULL, CHECK_MPATHD_ENABLED, STAGE_CHECK_MULTIPATHD);
> -	will_return(__wrap_udev_device_new_from_subsystem_sysname, false);
> -	will_return(__wrap_udev_device_new_from_subsystem_sysname,
> -		    name);
> -	assert_int_equal(is_path_valid(name, &conf, &pp, true),
> -			 PATH_IS_ERROR);
> -	assert_string_equal(pp.dev, name);
>  	/* test pass because connect returned EAGAIN. fail getting udev */
>  	setup_passing(name, NULL, CHECK_MPATHD_EAGAIN, STAGE_CHECK_MULTIPATHD);
>  	will_return(__wrap_udev_device_new_from_subsystem_sysname, false);
> @@ -533,7 +515,7 @@ static void test_check_uuid_present(void **state)
>  
>  	memset(&pp, 0, sizeof(pp));
>  	conf.find_multipaths = FIND_MULTIPATHS_STRICT;
> -	setup_passing(name, wwid, CHECK_MPATHD_ENABLED, STAGE_CHECK_WWIDS);
> +	setup_passing(name, wwid, CHECK_MPATHD_RUNNING, STAGE_CHECK_WWIDS);
>  	will_return(__wrap_dm_map_present_by_uuid, 1);
>  	will_return(__wrap_dm_map_present_by_uuid, wwid);
>  	assert_int_equal(is_path_valid(name, &conf, &pp, true),
> -- 
> 2.42.0

Martin Wilck Oct. 26, 2023, 8:42 a.m. UTC | #2

On Wed, 2023-10-25 at 19:58 -0400, Benjamin Marzinski wrote:
> On Tue, Oct 24, 2023 at 06:49:37PM +0200, [e-mail] wrote:
> > From: Martin Wilck <[e-mail]>
> > 
> > Since  c203929 ("multipathd.service: add dependency on
> > systemd-udevd-kernel.socket"), multipathd will start early during
> > boot.
> > Moreover, we recommend using socket activation for multipathd,
> > and if multipathd.socket is enabled, the __mpath_connect()
> > check will succeed anyway.
> > 
> > The systemd_service_enabled() test was just "good enough" for
> > standard situations but never robust; it checked for
> > multipathd.wants in
> > "some" directory, which might or might not be the current target,
> > and it would return true even of multipathd.service was masked.
> > 
> > Remove this test.
> > 
> > Signed-off-by: Martin Wilck <[e-mail]>
> 
> I'd be lying if I said I had no worries at all about this. Removing
> this
> check means that if someone isn't using socket activation, and
> multipathd is temporarily down, and a new device appears, it will
> always
> be marked as not claimed by multipath. This could cause the device to
> be
> grabbed by LVM, and not multipathed. This is probably just may
> paranoia
> talking, since the chance of this happening in the real world seems
> pretty low. So,
> 
> Reviewed-by: Benjamin Marzinski <[e-mail]>

Thanks. I had the same doubts, but I came to the conclusion that
there's no need to be afraid.

I would put your argument a bit differently: If socket activation is
not used and multipathd is not running, we are in a situation where
multipath can't seriously claim any device. Whether multipathd is
enabled in some (apparently inactive) systemd target doesn't really
matter in such a situation. Note that we never checked  _which_ systemd
unit lists multipathd under its ".wants". It could be
"shutdown.target".

Martin

Patch

diff --git a/libmpathutil/libmpathutil.version b/libmpathutil/libmpathutil.version
index 6ebb718..15ff467 100644
--- a/libmpathutil/libmpathutil.version
+++ b/libmpathutil/libmpathutil.version
@@ -93,12 +93,15 @@  local:
 };
 
 /* symbols referenced internally by libmultipath */
-LIBMPATHUTIL_1.0 {
+LIBMPATHUTIL_2.0 {
 	alloc_bitfield;
 	__append_strbuf_str;
 	append_strbuf_quoted;
 	basenamecpy;
+	cleanup_fd_ptr;
 	cleanup_free_ptr;
+	cleanup_vector_free;
+	cleanup_fclose;
 	filepresent;
 	find_keyword;
 	free_keywords;
@@ -120,21 +123,9 @@  LIBMPATHUTIL_1.0 {
 	snprint_keyword;
 	steal_strbuf_str;
 	strlcat;
-	systemd_service_enabled;
 	validate_config_strvec;
 	vector_find_or_add_slot;
 	vector_insert_slot;
 	vector_move_up;
 	vector_sort;
 };
-
-LIBMPATHUTIL_1.1 {
-global:
-	cleanup_fd_ptr;
-} LIBMPATHUTIL_1.0;
-
-LIBMPATHUTIL_1.2 {
-global:
-	cleanup_vector_free;
-	cleanup_fclose;
-} LIBMPATHUTIL_1.0;
diff --git a/libmpathutil/util.c b/libmpathutil/util.c
index 92f25a5..9d147fc 100644
--- a/libmpathutil/util.c
+++ b/libmpathutil/util.c
@@ -213,64 +213,6 @@  setup_thread_attr(pthread_attr_t *attr, size_t stacksize, int detached)
 	}
 }
 
-int systemd_service_enabled_in(const char *dev, const char *prefix)
-{
-	static const char service[] = "multipathd.service";
-	char path[PATH_MAX], file[PATH_MAX];
-	DIR *dirfd;
-	struct dirent *d;
-	int found = 0;
-
-	if (safe_sprintf(path, "%s/systemd/system", prefix))
-		return 0;
-
-	condlog(3, "%s: checking for %s in %s", dev, service, path);
-
-	dirfd = opendir(path);
-	if (dirfd == NULL)
-		return 0;
-
-	while ((d = readdir(dirfd)) != NULL) {
-		char *p;
-		struct stat stbuf;
-
-		if ((strcmp(d->d_name,".") == 0) ||
-		    (strcmp(d->d_name,"..") == 0))
-			continue;
-
-		if (strlen(d->d_name) < 6)
-			continue;
-
-		p = d->d_name + strlen(d->d_name) - 6;
-		if (strcmp(p, ".wants"))
-			continue;
-		if (!safe_sprintf(file, "%s/%s/%s",
-				  path, d->d_name, service)
-		    && stat(file, &stbuf) == 0) {
-			condlog(3, "%s: found %s", dev, file);
-			found++;
-			break;
-		}
-	}
-	closedir(dirfd);
-
-	return found;
-}
-
-int systemd_service_enabled(const char *dev)
-{
-	int found = 0;
-
-	found = systemd_service_enabled_in(dev, "/etc");
-	if (!found)
-		found = systemd_service_enabled_in(dev, "/usr/lib");
-	if (!found)
-		found = systemd_service_enabled_in(dev, "/lib");
-	if (!found)
-		found = systemd_service_enabled_in(dev, "/run");
-	return found;
-}
-
 static int _linux_version_code;
 static pthread_once_t _lvc_initialized = PTHREAD_ONCE_INIT;
 
diff --git a/libmpathutil/util.h b/libmpathutil/util.h
index 99a471d..de9fcfd 100644
--- a/libmpathutil/util.h
+++ b/libmpathutil/util.h
@@ -21,7 +21,6 @@  size_t strlcat(char * restrict dst, const char * restrict src, size_t size);
 dev_t parse_devt(const char *dev_t);
 char *convert_dev(char *dev, int is_path_device);
 void setup_thread_attr(pthread_attr_t *attr, size_t stacksize, int detached);
-int systemd_service_enabled(const char *dev);
 int get_linux_version_code(void);
 int safe_write(int fd, const void *buf, size_t count);
 void set_max_fds(rlim_t max_fds);
diff --git a/libmultipath/valid.c b/libmultipath/valid.c
index d4dae3e..f223778 100644
--- a/libmultipath/valid.c
+++ b/libmultipath/valid.c
@@ -314,23 +314,11 @@  is_path_valid(const char *name, struct config *conf, struct path *pp,
 		return PATH_IS_VALID_NO_CHECK;
 	}
 
-	/*
-	 * "multipath -u" may be run before the daemon is started. In this
-	 * case, systemd might own the socket but might delay multipathd
-	 * startup until some other unit (udev settle!)  has finished
-	 * starting. With many LUNs, the listen backlog may be exceeded, which
-	 * would cause connect() to block. This causes udev workers calling
-	 * "multipath -u" to hang, and thus creates a deadlock, until "udev
-	 * settle" times out.  To avoid this, call connect() in non-blocking
-	 * mode here, and take EAGAIN as indication for a filled-up systemd
-	 * backlog.
-	 */
-
 	if (check_multipathd) {
 		fd = __mpath_connect(1);
 		if (fd < 0) {
-			if (errno != EAGAIN && !systemd_service_enabled(name)) {
-				condlog(3, "multipathd not running or enabled");
+			if (errno != EAGAIN) {
+				condlog(3, "multipathd not running");
 				return PATH_IS_NOT_VALID;
 			}
 		} else
diff --git a/tests/valid.c b/tests/valid.c
index 7032932..18a5a7b 100644
--- a/tests/valid.c
+++ b/tests/valid.c
@@ -62,11 +62,6 @@  int __wrap___mpath_connect(int nonblocking)
 	return -1;
 }
 
-int __wrap_systemd_service_enabled(const char *dev)
-{
-	return (int)mock_type(bool);
-}
-
 /* There's no point in checking the return value here */
 int __wrap_mpath_disconnect(int fd)
 {
@@ -216,7 +211,6 @@  enum {
 enum {
 	CHECK_MPATHD_RUNNING,
 	CHECK_MPATHD_EAGAIN,
-	CHECK_MPATHD_ENABLED,
 	CHECK_MPATHD_SKIP,
 };
 
@@ -232,11 +226,8 @@  static void setup_passing(char *name, char *wwid, unsigned int check_multipathd,
 	else if (check_multipathd == CHECK_MPATHD_EAGAIN) {
 		will_return(__wrap___mpath_connect, false);
 		will_return(__wrap___mpath_connect, EAGAIN);
-	} else if (check_multipathd == CHECK_MPATHD_ENABLED) {
-		will_return(__wrap___mpath_connect, false);
-		will_return(__wrap___mpath_connect, ECONNREFUSED);
-		will_return(__wrap_systemd_service_enabled, true);
 	}
+
 	/* nothing for CHECK_MPATHD_SKIP */
 	if (stage == STAGE_CHECK_MULTIPATHD)
 		return;
@@ -342,19 +333,10 @@  static void test_check_multipathd(void **state)
 	will_return(__wrap_sysfs_is_multipathed, false);
 	will_return(__wrap___mpath_connect, false);
 	will_return(__wrap___mpath_connect, ECONNREFUSED);
-	will_return(__wrap_systemd_service_enabled, false);
+
 	assert_int_equal(is_path_valid(name, &conf, &pp, true),
 			 PATH_IS_NOT_VALID);
 	assert_string_equal(pp.dev, name);
-	/* test pass because service is enabled. fail getting udev */
-	memset(&pp, 0, sizeof(pp));
-	setup_passing(name, NULL, CHECK_MPATHD_ENABLED, STAGE_CHECK_MULTIPATHD);
-	will_return(__wrap_udev_device_new_from_subsystem_sysname, false);
-	will_return(__wrap_udev_device_new_from_subsystem_sysname,
-		    name);
-	assert_int_equal(is_path_valid(name, &conf, &pp, true),
-			 PATH_IS_ERROR);
-	assert_string_equal(pp.dev, name);
 	/* test pass because connect returned EAGAIN. fail getting udev */
 	setup_passing(name, NULL, CHECK_MPATHD_EAGAIN, STAGE_CHECK_MULTIPATHD);
 	will_return(__wrap_udev_device_new_from_subsystem_sysname, false);
@@ -533,7 +515,7 @@  static void test_check_uuid_present(void **state)
 
 	memset(&pp, 0, sizeof(pp));
 	conf.find_multipaths = FIND_MULTIPATHS_STRICT;
-	setup_passing(name, wwid, CHECK_MPATHD_ENABLED, STAGE_CHECK_WWIDS);
+	setup_passing(name, wwid, CHECK_MPATHD_RUNNING, STAGE_CHECK_WWIDS);
 	will_return(__wrap_dm_map_present_by_uuid, 1);
 	will_return(__wrap_dm_map_present_by_uuid, wwid);
 	assert_int_equal(is_path_valid(name, &conf, &pp, true),