[dm-devel] [PATCH] libmpathpersist: fix thread safety of default functions
Benjamin Marzinski
bmarzins at redhat.com
Tue Jan 26 05:31:04 UTC 2021
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 at 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
More information about the dm-devel
mailing list