[dm-devel] [PATCH] libmpathpersist: fix thread safety of default functions

* [dm-devel] [PATCH] libmpathpersist: fix thread safety of default functions
@ 2021-01-26  5:31 Benjamin Marzinski
  2021-01-26  9:36 ` Martin Wilck
  2021-01-26 10:04 ` Martin Wilck
  0 siblings, 2 replies; 5+ messages in thread
From: Benjamin Marzinski @ 2021-01-26  5:31 UTC (permalink / raw)
  To: Christophe Varoqui; +Cc: device-mapper development, Martin Wilck

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>
---
 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 08077936..e4c24b93 100644
--- a/libmpathpersist/mpath_persist.c
+++ b/libmpathpersist/mpath_persist.c
@@ -133,69 +133,57 @@ 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)
 {
 	libmp_verbosity = verbose;
 
-	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;
@@ -255,13 +243,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;
 
@@ -270,8 +258,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;
@@ -279,7 +276,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;
 
@@ -349,6 +346,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, 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, 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 5435eae4..9e9c0a82 100644
--- a/libmpathpersist/mpath_persist.h
+++ b/libmpathpersist/mpath_persist.h
@@ -246,9 +246,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);
@@ -280,9 +284,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,
@@ -296,6 +304,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.
@@ -306,6 +315,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

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


^ permalink raw reply related	[flat|nested] 5+ messages in thread