device-mapper-multipath/0119-pathinfo-call-filter_property-after-sysfs_pathinfo.patch

From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Martin Wilck <mwilck@suse.com>
Date: Tue, 2 Feb 2021 17:07:37 +0100
Subject: [PATCH] pathinfo: call filter_property() after sysfs_pathinfo()

The of filter_property() depends on the value of pp->uid_attribute.
This may in turn depend on pp->hwe, which is initialized in
sysfs_pathinfo(). To obtain consistent results from pathinfo(),
make sure uid_attribute is correctly set before calling filter_property().

filter_property() is now called from pathinfo() with properly set
uid_attribute, thus we don't need to call it from is_path_valid() any more.

Thes changes require modifications to the unit tests. The is_path_valid()
test now wouldn't need to test filter_property() any more, because
is_path_valid() calls filter_property() no more. But that doesn't feel
right. Instead, test_filter_property() is modified to test the behavior
with the filter_property() test called indirectly from pathinfo().

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/discovery.c | 21 +++++++++-
 libmultipath/valid.c     |  4 --
 tests/Makefile           |  2 +-
 tests/test-lib.c         |  5 ++-
 tests/valid.c            | 91 ++++++++++++++++++++++++++++++++++++----
 5 files changed, 105 insertions(+), 18 deletions(-)

diff --git a/libmultipath/discovery.c b/libmultipath/discovery.c
index 15cf6413..febcd0ae 100644
--- a/libmultipath/discovery.c
+++ b/libmultipath/discovery.c
@@ -2247,9 +2247,17 @@ int pathinfo(struct path *pp, struct config *conf, int mask)
 			condlog(4, "%s: hidden", pp->dev);
 			return PATHINFO_SKIPPED;
 		}
-		if (is_claimed_by_foreign(pp->udev) ||
-		    filter_property(conf, pp->udev, 4, pp->uid_attribute) > 0)
+
+		if (is_claimed_by_foreign(pp->udev))
 			return PATHINFO_SKIPPED;
+
+		/*
+		 * uid_attribute is required for filter_property below,
+		 * and needs access to pp->hwe.
+		 */
+		if (!(mask & DI_SYSFS) && !pp->uid_attribute &&
+		    VECTOR_SIZE(pp->hwe) == 0)
+			mask |= DI_SYSFS;
 	}
 
 	if (strlen(pp->dev) != 0 && filter_devnode(conf->blist_devnode,
@@ -2287,6 +2295,15 @@ int pathinfo(struct path *pp, struct config *conf, int mask)
 		}
 	}
 
+	if (pp->udev) {
+		/* uid_attribute is required for filter_property() */
+		if (!pp->uid_attribute)
+			select_getuid(conf, pp);
+
+		if (filter_property(conf, pp->udev, 4, pp->uid_attribute) > 0)
+			return PATHINFO_SKIPPED;
+	}
+
 	if (mask & DI_BLACKLIST && mask & DI_SYSFS) {
 		if (filter_device(conf->blist_device, conf->elist_device,
 				  pp->vendor_id, pp->product_id, pp->dev) > 0 ||
diff --git a/libmultipath/valid.c b/libmultipath/valid.c
index 456b1f6e..a6aa9215 100644
--- a/libmultipath/valid.c
+++ b/libmultipath/valid.c
@@ -89,10 +89,6 @@ is_path_valid(const char *name, struct config *conf, struct path *pp,
 	if (pp->wwid[0] == '\0')
 		return PATH_IS_NOT_VALID;
 
-	if (pp->udev && pp->uid_attribute &&
-	    filter_property(conf, pp->udev, 3, pp->uid_attribute) > 0)
-		return PATH_IS_NOT_VALID;
-
 	r = is_failed_wwid(pp->wwid);
 	if (r != WWID_IS_NOT_FAILED) {
 		if (r == WWID_IS_FAILED)
diff --git a/tests/Makefile b/tests/Makefile
index 50673fae..11ca1be5 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -54,7 +54,7 @@ vpd-test_OBJDEPS :=  ../libmultipath/discovery.o
 vpd-test_LIBDEPS := -ludev -lpthread -ldl
 alias-test_TESTDEPS := test-log.o
 alias-test_LIBDEPS := -lpthread -ldl
-valid-test_OBJDEPS := ../libmultipath/valid.o
+valid-test_OBJDEPS := ../libmultipath/valid.o ../libmultipath/discovery.o
 valid-test_LIBDEPS := -ludev -lpthread -ldl
 devt-test_LIBDEPS := -ludev
 mpathvalid-test_LIBDEPS := -ludev -lpthread -ldl
diff --git a/tests/test-lib.c b/tests/test-lib.c
index e7663f9a..960a7665 100644
--- a/tests/test-lib.c
+++ b/tests/test-lib.c
@@ -257,6 +257,9 @@ void mock_pathinfo(int mask, const struct mocked_path *mp)
 	} else
 		will_return(__wrap_udev_device_get_sysattr_value, "0");
 
+	if (mask & DI_SYSFS)
+		mock_sysfs_pathinfo(mp);
+
 	/* filter_property */
 	will_return(__wrap_udev_device_get_sysname, mp->devnode);
 	if (mp->flags & BL_BY_PROPERTY) {
@@ -265,8 +268,6 @@ void mock_pathinfo(int mask, const struct mocked_path *mp)
 	} else
 		will_return(__wrap_udev_list_entry_get_name,
 			    "SCSI_IDENT_LUN_NAA_EXT");
-	if (mask & DI_SYSFS)
-		mock_sysfs_pathinfo(mp);
 
 	if (mp->flags & BL_BY_DEVICE &&
 	    (mask & DI_BLACKLIST && mask & DI_SYSFS))
diff --git a/tests/valid.c b/tests/valid.c
index 693c72c5..8ec803e8 100644
--- a/tests/valid.c
+++ b/tests/valid.c
@@ -25,13 +25,18 @@
 #include <stdlib.h>
 #include <errno.h>
 #include <cmocka.h>
+#include <sys/sysmacros.h>
+
 #include "globals.c"
 #include "util.h"
 #include "discovery.h"
 #include "wwids.h"
 #include "blacklist.h"
+#include "foreign.h"
 #include "valid.h"
 
+#define PATHINFO_REAL 9999
+
 int test_fd;
 struct udev_device {
 	int unused;
@@ -78,12 +83,66 @@ struct udev_device *__wrap_udev_device_new_from_subsystem_sysname(struct udev *u
 	return NULL;
 }
 
+/* For the "hidden" check in pathinfo() */
+const char *__wrap_udev_device_get_sysattr_value(struct udev_device *udev_device,
+					 const char *sysattr)
+{
+	check_expected(sysattr);
+	return mock_ptr_type(char *);
+}
+
+/* For pathinfo() -> is_claimed_by_foreign() */
+int __wrap_add_foreign(struct udev_device *udev_device)
+{
+	return mock_type(int);
+}
+
+/* called from pathinfo() */
+int __wrap_filter_devnode(struct config *conf, const struct _vector *elist,
+			  const char *vendor, const char * product, const char *dev)
+{
+	return mock_type(int);
+}
+
+/* called from pathinfo() */
+int __wrap_filter_device(const struct _vector *blist, const struct _vector *elist,
+	       const char *vendor, const char * product, const char *dev)
+{
+	return mock_type(int);
+}
+
+/* for common_sysfs_pathinfo() */
+dev_t __wrap_udev_device_get_devnum(struct udev_device *ud)
+{
+	return  mock_type(dev_t);
+}
+
+/* for common_sysfs_pathinfo() */
+int __wrap_sysfs_get_size(struct path *pp, unsigned long long * size)
+{
+	return mock_type(int);
+}
+
+/* called in pathinfo() before filter_property() */
+int __wrap_select_getuid(struct config *conf, struct path *pp)
+{
+	pp->uid_attribute = mock_ptr_type(char *);
+	return 0;
+}
+
+int __real_pathinfo(struct path *pp, struct config *conf, int mask);
+
 int __wrap_pathinfo(struct path *pp, struct config *conf, int mask)
 {
 	int ret = mock_type(int);
+
 	assert_string_equal(pp->dev, mock_ptr_type(char *));
 	assert_int_equal(mask, DI_SYSFS | DI_WWID | DI_BLACKLIST);
-	if (ret == PATHINFO_OK) {
+	if (ret == PATHINFO_REAL) {
+		/* for test_filter_property() */
+		ret =  __real_pathinfo(pp, conf, mask);
+		return ret;
+	} else if (ret == PATHINFO_OK) {
 		pp->uid_attribute = "ID_TEST";
 		strlcpy(pp->wwid, mock_ptr_type(char *), WWID_SIZE);
 	} else
@@ -128,6 +187,7 @@ enum {
 	STAGE_IS_MULTIPATHED,
 	STAGE_CHECK_MULTIPATHD,
 	STAGE_GET_UDEV_DEVICE,
+	STAGE_PATHINFO_REAL,
 	STAGE_PATHINFO,
 	STAGE_FILTER_PROPERTY,
 	STAGE_IS_FAILED,
@@ -167,12 +227,25 @@ static void setup_passing(char *name, char *wwid, unsigned int check_multipathd,
 		    name);
 	if (stage == STAGE_GET_UDEV_DEVICE)
 		return;
+	if  (stage == STAGE_PATHINFO_REAL) {
+		/* special case for test_filter_property() */
+		will_return(__wrap_pathinfo, PATHINFO_REAL);
+		will_return(__wrap_pathinfo, name);
+		expect_string(__wrap_udev_device_get_sysattr_value,
+			      sysattr, "hidden");
+		will_return(__wrap_udev_device_get_sysattr_value, NULL);
+		will_return(__wrap_add_foreign, FOREIGN_IGNORED);
+		will_return(__wrap_filter_devnode, MATCH_NOTHING);
+		will_return(__wrap_udev_device_get_devnum, makedev(259, 0));
+		will_return(__wrap_sysfs_get_size, 0);
+		will_return(__wrap_select_getuid, "ID_TEST");
+		return;
+	}
 	will_return(__wrap_pathinfo, PATHINFO_OK);
 	will_return(__wrap_pathinfo, name);
 	will_return(__wrap_pathinfo, wwid);
 	if (stage == STAGE_PATHINFO)
 		return;
-	will_return(__wrap_filter_property, MATCH_PROPERTY_BLIST_EXCEPT);
 	if (stage == STAGE_FILTER_PROPERTY)
 		return;
 	will_return(__wrap_is_failed_wwid, WWID_IS_NOT_FAILED);
@@ -317,24 +390,24 @@ static void test_filter_property(void **state)
 	/* test blacklist property */
 	memset(&pp, 0, sizeof(pp));
 	conf.find_multipaths = FIND_MULTIPATHS_STRICT;
-	setup_passing(name, wwid, CHECK_MPATHD_SKIP, STAGE_PATHINFO);
+	setup_passing(name, wwid, CHECK_MPATHD_SKIP, STAGE_PATHINFO_REAL);
 	will_return(__wrap_filter_property, MATCH_PROPERTY_BLIST);
 	assert_int_equal(is_path_valid(name, &conf, &pp, false),
 			 PATH_IS_NOT_VALID);
 	assert_ptr_equal(pp.udev, &test_udev);
-	assert_string_equal(pp.wwid, wwid);
+
 	/* test missing property */
 	memset(&pp, 0, sizeof(pp));
-	setup_passing(name, wwid, CHECK_MPATHD_SKIP, STAGE_PATHINFO);
+	setup_passing(name, wwid, CHECK_MPATHD_SKIP, STAGE_PATHINFO_REAL);
 	will_return(__wrap_filter_property, MATCH_PROPERTY_BLIST_MISSING);
 	assert_int_equal(is_path_valid(name, &conf, &pp, false),
 			 PATH_IS_NOT_VALID);
-	/* test MATCH_NOTHING fail on is_failed_wwid */
+
+	/* test MATCH_NOTHING fail on filter_device */
 	memset(&pp, 0, sizeof(pp));
-	setup_passing(name, wwid, CHECK_MPATHD_SKIP, STAGE_PATHINFO);
+	setup_passing(name, wwid, CHECK_MPATHD_SKIP, STAGE_PATHINFO_REAL);
 	will_return(__wrap_filter_property, MATCH_NOTHING);
-	will_return(__wrap_is_failed_wwid, WWID_IS_FAILED);
-	will_return(__wrap_is_failed_wwid, wwid);
+	will_return(__wrap_filter_device, MATCH_DEVICE_BLIST);
 	assert_int_equal(is_path_valid(name, &conf, &pp, false),
 			 PATH_IS_NOT_VALID);
 }
-- 
2.17.2