From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001

From: Benjamin Marzinski <bmarzins@redhat.com>

Date: Mon, 25 Jan 2021 23:31:04 -0600

Subject: [PATCH] libmpathpersist: fix thread safety of default functions

commit a839e39e ("libmpathpersist: factor out initialization and

teardown") made mpath_presistent_reserve_{in,out} use share variables

for curmp and pathvec.  There are users of this library that call these

functions in a multi-threaded process, and this change causes their

application to crash. config and udev are also shared variables, but

libmpathpersist doesn't write to the config in

mpath_presistent_reserve_{in,out}, and looking into the libudev code, I

don't see any place where libmpathpersist uses the udev object in a way

that isn't thread-safe.

This patch makes mpath_presistent_reserve_{in,out} go back to using

local variables for curmp and pathvec, so that multiple threads won't

be operating on these variables at the same time.

Fixes: a839e39e ("libmpathpersist: factor out initialization and teardown")

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

Reviewed-by: Martin Wilck <mwilck@suse.com>

---

 libmpathpersist/mpath_persist.c | 116 +++++++++++++++++++++-----------

 libmpathpersist/mpath_persist.h |  24 +++++--

 2 files changed, 94 insertions(+), 46 deletions(-)

diff --git a/libmpathpersist/mpath_persist.c b/libmpathpersist/mpath_persist.c

index a01dfb0b..07a5f17f 100644

--- a/libmpathpersist/mpath_persist.c

+++ b/libmpathpersist/mpath_persist.c

@@ -147,72 +147,60 @@ mpath_prin_activepath (struct multipath *mpp, int rq_servact,

 	return ret;

 }

-int mpath_persistent_reserve_in (int fd, int rq_servact,

-	struct prin_resp *resp, int noisy, int verbose)

-{

-	int ret = mpath_persistent_reserve_init_vecs(verbose);

-

-	if (ret != MPATH_PR_SUCCESS)

-		return ret;

-	ret = __mpath_persistent_reserve_in(fd, rq_servact, resp, noisy);

-	mpath_persistent_reserve_free_vecs();

-	return ret;

-}

-

-int mpath_persistent_reserve_out ( int fd, int rq_servact, int rq_scope,

-	unsigned int rq_type, struct prout_param_descriptor *paramp, int noisy, int verbose)

-{

-	int ret = mpath_persistent_reserve_init_vecs(verbose);

-

-	if (ret != MPATH_PR_SUCCESS)

-		return ret;

-	ret = __mpath_persistent_reserve_out(fd, rq_servact, rq_scope, rq_type,

-					     paramp, noisy);

-	mpath_persistent_reserve_free_vecs();

-	return ret;

-}

-

 static vector curmp;

 static vector pathvec;

-void mpath_persistent_reserve_free_vecs(void)

+static void __mpath_persistent_reserve_free_vecs(vector curmp, vector pathvec)

 {

 	free_multipathvec(curmp, KEEP_PATHS);

 	free_pathvec(pathvec, FREE_PATHS);

+}

+

+void mpath_persistent_reserve_free_vecs(void)

+{

+	__mpath_persistent_reserve_free_vecs(curmp, pathvec);

 	curmp = pathvec = NULL;

 }

-int mpath_persistent_reserve_init_vecs(int verbose)

+static int __mpath_persistent_reserve_init_vecs(vector *curmp_p,

+						vector *pathvec_p, int verbose)

 {

 	struct config *conf = get_multipath_config();

 	conf->verbosity = verbose;

 	put_multipath_config(conf);

-	if (curmp)

+	if (*curmp_p)

 		return MPATH_PR_SUCCESS;

 	/*

 	 * allocate core vectors to store paths and multipaths

 	 */

-	curmp = vector_alloc ();

-	pathvec = vector_alloc ();

+	*curmp_p = vector_alloc ();

+	*pathvec_p = vector_alloc ();

-	if (!curmp || !pathvec){

+	if (!*curmp_p || !*pathvec_p){

 		condlog (0, "vector allocation failed.");

 		goto err;

 	}

-	if (dm_get_maps(curmp))

+	if (dm_get_maps(*curmp_p))

 		goto err;

 	return MPATH_PR_SUCCESS;

 err:

-	mpath_persistent_reserve_free_vecs();

+	__mpath_persistent_reserve_free_vecs(*curmp_p, *pathvec_p);

+	*curmp_p = *pathvec_p = NULL;

 	return MPATH_PR_DMMP_ERROR;

 }

-static int mpath_get_map(int fd, char **palias, struct multipath **pmpp)

+int mpath_persistent_reserve_init_vecs(int verbose)

+{

+	return __mpath_persistent_reserve_init_vecs(&curmp, &pathvec, verbose);

+}

+

+static int mpath_get_map(vector curmp, vector pathvec, int fd, char **palias,

+			 struct multipath **pmpp)

 {

 	int ret = MPATH_PR_DMMP_ERROR;

 	struct stat info;

@@ -272,13 +260,13 @@ out:

 	return ret;

 }

-int __mpath_persistent_reserve_in (int fd, int rq_servact,

-	struct prin_resp *resp, int noisy)

+static int do_mpath_persistent_reserve_in (vector curmp, vector pathvec,

+	int fd, int rq_servact, struct prin_resp *resp, int noisy)

 {

 	struct multipath *mpp;

 	int ret;

-	ret = mpath_get_map(fd, NULL, &mpp;;

+	ret = mpath_get_map(curmp, pathvec, fd, NULL, &mpp;;

 	if (ret != MPATH_PR_SUCCESS)

 		return ret;

@@ -287,8 +275,17 @@ int __mpath_persistent_reserve_in (int fd, int rq_servact,

 	return ret;

 }

-int __mpath_persistent_reserve_out ( int fd, int rq_servact, int rq_scope,

-	unsigned int rq_type, struct prout_param_descriptor *paramp, int noisy)

+

+int __mpath_persistent_reserve_in (int fd, int rq_servact,

+	struct prin_resp *resp, int noisy)

+{

+	return do_mpath_persistent_reserve_in(curmp, pathvec, fd, rq_servact,

+					      resp, noisy);

+}

+

+static int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,

+	int rq_servact, int rq_scope, unsigned int rq_type,

+	struct prout_param_descriptor *paramp, int noisy)

 {

 	struct multipath *mpp;

 	char *alias;

@@ -296,7 +293,7 @@ int __mpath_persistent_reserve_out ( int fd, int rq_servact, int rq_scope,

 	uint64_t prkey;

 	struct config *conf;

-	ret = mpath_get_map(fd, &alias, &mpp;;

+	ret = mpath_get_map(curmp, pathvec, fd, &alias, &mpp;;

 	if (ret != MPATH_PR_SUCCESS)

 		return ret;

@@ -366,6 +363,45 @@ out1:

 	return ret;

 }

+

+int __mpath_persistent_reserve_out ( int fd, int rq_servact, int rq_scope,

+	unsigned int rq_type, struct prout_param_descriptor *paramp, int noisy)

+{

+	return do_mpath_persistent_reserve_out(curmp, pathvec, fd, rq_servact,

+					       rq_scope, rq_type, paramp,

+					       noisy);

+}

+

+int mpath_persistent_reserve_in (int fd, int rq_servact,

+	struct prin_resp *resp, int noisy, int verbose)

+{

+	vector curmp = NULL, pathvec;

+	int ret = __mpath_persistent_reserve_init_vecs(&curmp, &pathvec,

+						       verbose);

+

+	if (ret != MPATH_PR_SUCCESS)

+		return ret;

+	ret = do_mpath_persistent_reserve_in(curmp, pathvec, fd, rq_servact,

+					     resp, noisy);

+	__mpath_persistent_reserve_free_vecs(curmp, pathvec);

+	return ret;

+}

+

+int mpath_persistent_reserve_out ( int fd, int rq_servact, int rq_scope,

+	unsigned int rq_type, struct prout_param_descriptor *paramp, int noisy, int verbose)

+{

+	vector curmp = NULL, pathvec;

+	int ret = __mpath_persistent_reserve_init_vecs(&curmp, &pathvec,

+						       verbose);

+

+	if (ret != MPATH_PR_SUCCESS)

+		return ret;

+	ret = do_mpath_persistent_reserve_out(curmp, pathvec, fd, rq_servact,

+					      rq_scope, rq_type, paramp, noisy);

+	__mpath_persistent_reserve_free_vecs(curmp, pathvec);

+	return ret;

+}

+

 int

 get_mpvec (vector curmp, vector pathvec, char * refwwid)

 {

diff --git a/libmpathpersist/mpath_persist.h b/libmpathpersist/mpath_persist.h

index 7cf4faf9..0e4e0e53 100644

--- a/libmpathpersist/mpath_persist.h

+++ b/libmpathpersist/mpath_persist.h

@@ -215,9 +215,13 @@ extern int mpath_persistent_reserve_in (int fd, int rq_servact, struct prin_resp

 /*

  * DESCRIPTION :

- * This function is like mpath_persistent_reserve_in(), except that it doesn't call

- * mpath_persistent_reserve_init_vecs() and mpath_persistent_reserve_free_vecs()

- * before and after the actual PR call.

+ * This function is like mpath_persistent_reserve_in(), except that it

+ * requires mpath_persistent_reserve_init_vecs() to be called before the

+ * PR call to set up internal variables. These must later be cleanup up

+ * by calling mpath_persistent_reserve_free_vecs().

+ *

+ * RESTRICTIONS:

+ * This function uses static internal variables, and is not thread-safe.

  */

 extern int __mpath_persistent_reserve_in(int fd, int rq_servact,

 		struct prin_resp *resp, int noisy);

@@ -249,9 +253,13 @@ extern int mpath_persistent_reserve_out ( int fd, int rq_servact, int rq_scope,

 		int verbose);

 /*

  * DESCRIPTION :

- * This function is like mpath_persistent_reserve_out(), except that it doesn't call

- * mpath_persistent_reserve_init_vecs() and mpath_persistent_reserve_free_vecs()

- * before and after the actual PR call.

+ * This function is like mpath_persistent_reserve_out(), except that it

+ * requires mpath_persistent_reserve_init_vecs() to be called before the

+ * PR call to set up internal variables. These must later be cleanup up

+ * by calling mpath_persistent_reserve_free_vecs().

+ *

+ * RESTRICTIONS:

+ * This function uses static internal variables, and is not thread-safe.

  */

 extern int __mpath_persistent_reserve_out( int fd, int rq_servact, int rq_scope,

 		unsigned int rq_type, struct prout_param_descriptor *paramp,

@@ -265,6 +273,7 @@ extern int __mpath_persistent_reserve_out( int fd, int rq_servact, int rq_scope,

  * @verbose: Set verbosity level. Input argument. value:0 to 3. 0->disabled, 3->Max verbose

  *

  * RESTRICTIONS:

+ * This function uses static internal variables, and is not thread-safe.

  *

  * RETURNS: MPATH_PR_SUCCESS if successful else returns any of the status specified

  *       above in RETURN_STATUS.

@@ -275,6 +284,9 @@ int mpath_persistent_reserve_init_vecs(int verbose);

  * DESCRIPTION :

  * This function frees data structures allocated by

  * mpath_persistent_reserve_init_vecs().

+ *

+ * RESTRICTIONS:

+ * This function uses static internal variables, and is not thread-safe.

  */

 void mpath_persistent_reserve_free_vecs(void);

-- 

2.17.2