From 18efd061795d01f371bad77594254bf6b494684c Mon Sep 17 00:00:00 2001 From: Benjamin Marzinski Date: Tue, 11 Nov 2025 20:46:21 -0500 Subject: [PATCH] device-mapper-multipath-0.8.7-41 Add 0195-multipathd-Fix-race-while-registering-PR-key.patch Add 0196-mpathpersist-Fix-REPORT-CAPABILITIES-output.patch * Fixes RHEL-118515 ("There are many bugs in multipath's persistent reservation handling") Resolves: RHEL-118515 --- ...hd-Fix-race-while-registering-PR-key.patch | 246 ++++++++++++++++++ ...rsist-Fix-REPORT-CAPABILITIES-output.patch | 108 ++++++++ device-mapper-multipath.spec | 11 +- 3 files changed, 364 insertions(+), 1 deletion(-) create mode 100644 0195-multipathd-Fix-race-while-registering-PR-key.patch create mode 100644 0196-mpathpersist-Fix-REPORT-CAPABILITIES-output.patch diff --git a/0195-multipathd-Fix-race-while-registering-PR-key.patch b/0195-multipathd-Fix-race-while-registering-PR-key.patch new file mode 100644 index 0000000..8e2a84f --- /dev/null +++ b/0195-multipathd-Fix-race-while-registering-PR-key.patch @@ -0,0 +1,246 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Benjamin Marzinski +Date: Thu, 6 Nov 2025 20:08:14 -0500 +Subject: [PATCH] multipathd: Fix race while registering PR key + +When libmpathpersist registers a key, It first checks which paths are +active, then registers the key on those paths, and then tells multipathd +that the key has been registered, and it should start tracking it. If +a path comes up after libmpathpersist checks for active paths, but before +it tells multipathd that the key has been registered, multipathd will not +register a key no that path (since it hasn't been told to that the device +has a registered key yet). This can leave the device with a path that is +missing a key. + +To solve this, when multipathd is told that a key has been registered, +it checks if there are the same number of registered keys as active +paths. If there aren't, it registers keys on all the paths (at least +until the number of registered keys and active paths are equal). To +avoid doing a bunch of unnecessary PR work, pr_register_active_paths() +has a new option to track the number of active paths, and +mpath_pr_event_handle() now takes the number of keys to expect. The +first time it's called, when there is an unknown number of keys, if the +number of keys it finds matches the number of active_paths, it and +pr_register_active_paths() exit early, since there is no registering +work to do. + +Signed-off-by: Benjamin Marzinski +Reviewed-by: Martin Wilck +--- + multipathd/cli_handlers.c | 6 +++- + multipathd/main.c | 67 ++++++++++++++++++++++++++++----------- + multipathd/main.h | 1 + + 3 files changed, 54 insertions(+), 20 deletions(-) + +diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c +index 541dd700..0c63ca9a 100644 +--- a/multipathd/cli_handlers.c ++++ b/multipathd/cli_handlers.c +@@ -1324,7 +1324,11 @@ cli_setprstatus(void * v, char ** reply, int * len, void * data) + + if (mpp->prflag != PR_SET) { + set_pr(mpp); +- condlog(2, "%s: prflag set", param); ++ pr_register_active_paths(mpp, true); ++ if (mpp->prflag == PR_SET) ++ condlog(2, "%s: prflag set", param); ++ else ++ condlog(0, "%s: Failed to set prflag", param); + } + memset(&mpp->old_pr_key, 0, 8); + +diff --git a/multipathd/main.c b/multipathd/main.c +index 1811747e..a85c0db4 100644 +--- a/multipathd/main.c ++++ b/multipathd/main.c +@@ -92,7 +92,8 @@ + #define MSG_SIZE 32 + + static unsigned int +-mpath_pr_event_handle(struct path *pp, unsigned int nr_keys_needed); ++mpath_pr_event_handle(struct path *pp, unsigned int nr_keys_needed, ++ unsigned int nr_keys_wanted); + + #define LOG_MSG(lvl, pp) \ + do { \ +@@ -547,19 +548,28 @@ flush_map_nopaths(struct multipath *mpp, struct vectors *vecs) { + return true; + } + +-static void +-pr_register_active_paths(struct multipath *mpp) ++void pr_register_active_paths(struct multipath *mpp, bool check_nr_active) + { + unsigned int i, j, nr_keys = 0; ++ unsigned int nr_active = 0; + struct path *pp; + struct pathgroup *pgp; + ++ if (check_nr_active) { ++ nr_active = count_active_paths(mpp); ++ if (!nr_active) ++ return; ++ } ++ + vector_foreach_slot (mpp->pg, pgp, i) { + vector_foreach_slot (pgp->paths, pp, j) { + if (mpp->prflag == PR_UNSET) + return; +- if ((pp->state == PATH_UP) || (pp->state == PATH_GHOST)) +- nr_keys = mpath_pr_event_handle(pp, nr_keys); ++ if (pp->state == PATH_UP || pp->state == PATH_GHOST) { ++ nr_keys = mpath_pr_event_handle(pp, nr_keys, nr_active); ++ if (check_nr_active && nr_keys == nr_active) ++ return; ++ } + } + } + } +@@ -646,7 +656,7 @@ fail: + + sync_map_state(mpp); + +- pr_register_active_paths(mpp); ++ pr_register_active_paths(mpp, false); + + if (VECTOR_SIZE(offline_paths) != 0) + handle_orphaned_offline_paths(offline_paths); +@@ -1185,7 +1195,7 @@ rescan: + verify_paths(mpp); + mpp->action = ACT_RELOAD; + prflag = mpp->prflag; +- mpath_pr_event_handle(pp, 0); ++ mpath_pr_event_handle(pp, 0, 0); + } else { + if (!should_multipath(pp, vecs->pathvec, vecs->mpvec)) { + orphan_path(pp, "only one path"); +@@ -1268,9 +1278,8 @@ rescan: + sync_map_state(mpp); + + if (retries >= 0) { +- if ((mpp->prflag == PR_SET && prflag != PR_SET) || +- start_waiter) +- pr_register_active_paths(mpp); ++ if ((mpp->prflag == PR_SET && prflag != PR_SET) || start_waiter) ++ pr_register_active_paths(mpp, false); + condlog(2, "%s [%s]: path added to devmap %s", + pp->dev, pp->dev_t, mpp->alias); + return 0; +@@ -2547,10 +2556,10 @@ check_path (struct vectors * vecs, struct path * pp, unsigned int ticks) + */ + condlog(2, "%s: checking persistent " + "reservation registration", pp->dev); +- mpath_pr_event_handle(pp, 0); ++ mpath_pr_event_handle(pp, 0, 0); + if (pp->mpp->prflag == PR_SET && + prflag != PR_SET) +- pr_register_active_paths(pp->mpp); ++ pr_register_active_paths(pp->mpp, false); + } + } + +@@ -2887,7 +2896,7 @@ configure (struct vectors * vecs) + vector_foreach_slot(mpvec, mpp, i){ + if (remember_wwid(mpp->wwid) == 1) + trigger_paths_udev_change(mpp, true); +- pr_register_active_paths(mpp); ++ pr_register_active_paths(mpp, false); + } + + /* +@@ -3769,7 +3778,8 @@ void unset_pr(struct multipath *mpp) + * + * The number of found keys must be at least as large as *nr_keys, + * and if MPATH_PR_SUCCESS is returned and mpp->prflag is PR_SET after +- * the call, *nr_keys will be set to the number of found keys. ++ * the call, *nr_keys will be set to the number of found keys. Otherwise ++ * it will be set to 0. + */ + static int update_map_pr(struct multipath *mpp, struct path *pp, unsigned int *nr_keys) + { +@@ -3779,13 +3789,18 @@ static int update_map_pr(struct multipath *mpp, struct path *pp, unsigned int *n + bool was_set = (mpp->prflag == PR_SET); + + /* If pr is explicitly unset, it must be manually set */ +- if (mpp->prflag == PR_UNSET) ++ if (mpp->prflag == PR_UNSET) { ++ *nr_keys = 0; + return MPATH_PR_SUCCESS; ++ } + + if (!get_be64(mpp->reservation_key)) { + /* Nothing to do. Assuming pr mgmt feature is disabled*/ + unset_pr(mpp); +- condlog(was_set ? 2 : 4, "%s: reservation_key not set in multipath.conf", mpp->alias); ++ condlog(was_set ? 2 : 4, ++ "%s: reservation_key not set in multipath.conf", ++ mpp->alias); ++ *nr_keys = 0; + return MPATH_PR_SUCCESS; + } + +@@ -3795,7 +3810,9 @@ static int update_map_pr(struct multipath *mpp, struct path *pp, unsigned int *n + if (ret != MPATH_PR_SUCCESS) { + if (ret == MPATH_PR_ILLEGAL_REQ) + unset_pr(mpp); +- condlog(0,"%s : pr in read keys service action failed Error=%d", mpp->alias, ret); ++ condlog(0, "%s : pr in read keys service action failed Error=%d", ++ mpp->alias, ret); ++ *nr_keys = 0; + return ret; + } + +@@ -3833,22 +3850,32 @@ static int update_map_pr(struct multipath *mpp, struct path *pp, unsigned int *n + condlog(was_set ? 1 : 3, + "%s: %u keys found. needed %u. prflag unset.", + mpp->alias, nr_found, *nr_keys); ++ *nr_keys = 0; + } + + return MPATH_PR_SUCCESS; + } + + /* +- * This function is called with the number of registered keys that should be ++ * This function is called with two numbers ++ * ++ * nr_keys_needed: the number of registered keys that should be + * seen for this device to know that the key has not been preempted while the + * path was getting registered. If 0 is passed in, update_mpath_pr is called + * before registering the key to figure out the number, assuming that at + * least one key must exist. + * ++ * nr_keys_wanted: Only used if nr_keys_needed is 0, so we don't know how ++ * many keys we currently have. If nr_keys_wanted in non-zero and the ++ * number of keys found by the initial call to update_map_pr() matches it, ++ * exit early, since we have all the keys we are expecting. ++ * + * The function returns the number of keys that are registered or 0 if + * it's unknown. + */ +-static unsigned int mpath_pr_event_handle(struct path *pp, unsigned int nr_keys_needed) ++static unsigned int ++mpath_pr_event_handle(struct path *pp, unsigned int nr_keys_needed, ++ unsigned int nr_keys_wanted) + { + struct multipath *mpp = pp->mpp; + int ret; +@@ -3864,6 +3891,8 @@ static unsigned int mpath_pr_event_handle(struct path *pp, unsigned int nr_keys_ + nr_keys_needed = 1; + if (update_map_pr(mpp, pp, &nr_keys_needed) != MPATH_PR_SUCCESS) + return 0; ++ if (nr_keys_wanted && nr_keys_wanted == nr_keys_needed) ++ return nr_keys_needed; + } + + check_prhold(mpp, pp); +diff --git a/multipathd/main.h b/multipathd/main.h +index 77603751..8a4c5f88 100644 +--- a/multipathd/main.h ++++ b/multipathd/main.h +@@ -60,4 +60,5 @@ int resize_map(struct multipath *mpp, unsigned long long size, + struct vectors *vecs); + void set_pr(struct multipath *mpp); + void unset_pr(struct multipath *mpp); ++void pr_register_active_paths(struct multipath *mpp, bool check_active_nr); + #endif /* MAIN_H */ diff --git a/0196-mpathpersist-Fix-REPORT-CAPABILITIES-output.patch b/0196-mpathpersist-Fix-REPORT-CAPABILITIES-output.patch new file mode 100644 index 0000000..060b442 --- /dev/null +++ b/0196-mpathpersist-Fix-REPORT-CAPABILITIES-output.patch @@ -0,0 +1,108 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: Benjamin Marzinski +Date: Mon, 10 Nov 2025 15:25:51 -0500 +Subject: [PATCH] mpathpersist: Fix REPORT CAPABILITIES output + +mpathpersist was incorrectly parsing the REPORT CAPABILITES service +action output. In reality, the type mask is two bytes where the type +information is stored in bits 7, 6, 5, 3, & 1 (0xea) of the first byte +and bit 0 (0x01) of the second byte. libmpathpersist was treating these +two bytes as a big endian 16 bit number, but mpathpersist was looking +for bits in that number as if it was little endian number. + +Ideally, libmpathpersist would treat prin_capdescr.pr_type_mask as +two bytes, like it does for the flags. But we already expose this +as a 16 bit number, where we treated the input bytes as a big endian +number. There's no great reason to mess with the libmpathpersist API, +when we can just make mpathpersist treat this data like libmpathpersist +provides it. So, fix mpathpersist to print the data out correctly. + +Additionally, instead of printing a 1 or a 0 to indicate if a type was +supported or not, it was printing the value of the type flag. Also, +Persist Through Power Loss Capable (PTPL_C) was being reported if any +bit in flags[0] was set. Fix these as well. Reformat all of the +capability printing lines, since it is less confusing than only +reformatting some of them. + +Fixes: ae4e8a6 ("mpathpersist: Add new utility for managing persistent reservation on dm multipath device") +Signed-off-by: Benjamin Marzinski +--- + libmpathpersist/mpath_persist.h | 4 ++- + mpathpersist/main.c | 43 +++++++++++++++++++++++---------- + 2 files changed, 33 insertions(+), 14 deletions(-) + +diff --git a/libmpathpersist/mpath_persist.h b/libmpathpersist/mpath_persist.h +index 807787b5..3d6fa51d 100644 +--- a/libmpathpersist/mpath_persist.h ++++ b/libmpathpersist/mpath_persist.h +@@ -118,7 +118,9 @@ struct prin_capdescr + { + uint16_t length; + uint8_t flags[2]; +- uint16_t pr_type_mask; ++ uint16_t pr_type_mask; /* The two bytes of the type mask are treated ++ as a single big-endian number. So the valid ++ type bits are 0xea01 */ + uint16_t _reserved; + }; + +diff --git a/mpathpersist/main.c b/mpathpersist/main.c +index 26c69b01..a0646cde 100644 +--- a/mpathpersist/main.c ++++ b/mpathpersist/main.c +@@ -754,25 +754,42 @@ void mpath_print_buf_readcap( struct prin_resp *pr_buff) + + printf("Report capabilities response:\n"); + +- printf(" Compatible Reservation Handling(CRH): %d\n", !!(pr_buff->prin_descriptor.prin_readcap.flags[0] & 0x10)); +- printf(" Specify Initiator Ports Capable(SIP_C): %d\n",!!(pr_buff->prin_descriptor.prin_readcap.flags[0] & 0x8)); +- printf(" All Target Ports Capable(ATP_C): %d\n",!!(pr_buff->prin_descriptor.prin_readcap.flags[0] & 0x4 )); +- printf(" Persist Through Power Loss Capable(PTPL_C): %d\n",!!(pr_buff->prin_descriptor.prin_readcap.flags[0])); +- printf(" Type Mask Valid(TMV): %d\n", !!(pr_buff->prin_descriptor.prin_readcap.flags[1] & 0x80)); +- printf(" Allow Commands: %d\n", !!(( pr_buff->prin_descriptor.prin_readcap.flags[1] >> 4) & 0x7)); ++ printf(" Compatible Reservation Handling(CRH): %d\n", ++ !!(pr_buff->prin_descriptor.prin_readcap.flags[0] & 0x10)); ++ printf(" Specify Initiator Ports Capable(SIP_C): %d\n", ++ !!(pr_buff->prin_descriptor.prin_readcap.flags[0] & 0x8)); ++ printf(" All Target Ports Capable(ATP_C): %d\n", ++ !!(pr_buff->prin_descriptor.prin_readcap.flags[0] & 0x4)); ++ printf(" Persist Through Power Loss Capable(PTPL_C): %d\n", ++ !!(pr_buff->prin_descriptor.prin_readcap.flags[0] & 0x1)); ++ printf(" Type Mask Valid(TMV): %d\n", ++ !!(pr_buff->prin_descriptor.prin_readcap.flags[1] & 0x80)); ++ printf(" Allow Commands: %d\n", ++ !!((pr_buff->prin_descriptor.prin_readcap.flags[1] >> 4) & 0x7)); + printf(" Persist Through Power Loss Active(PTPL_A): %d\n", +- !!(pr_buff->prin_descriptor.prin_readcap.flags[1] & 0x1)); ++ !!(pr_buff->prin_descriptor.prin_readcap.flags[1] & 0x1)); + + if(pr_buff->prin_descriptor.prin_readcap.flags[1] & 0x80) + { + printf(" Support indicated in Type mask:\n"); + +- printf(" %s: %d\n", pr_type_strs[7], pr_buff->prin_descriptor.prin_readcap.pr_type_mask & 0x80); +- printf(" %s: %d\n", pr_type_strs[6], pr_buff->prin_descriptor.prin_readcap.pr_type_mask & 0x40); +- printf(" %s: %d\n", pr_type_strs[5], pr_buff->prin_descriptor.prin_readcap.pr_type_mask & 0x20); +- printf(" %s: %d\n", pr_type_strs[3], pr_buff->prin_descriptor.prin_readcap.pr_type_mask & 0x8); +- printf(" %s: %d\n", pr_type_strs[1], pr_buff->prin_descriptor.prin_readcap.pr_type_mask & 0x2); +- printf(" %s: %d\n", pr_type_strs[8], pr_buff->prin_descriptor.prin_readcap.pr_type_mask & 0x100); ++ printf(" %s: %d\n", pr_type_strs[7], ++ !!(pr_buff->prin_descriptor.prin_readcap.pr_type_mask & ++ 0x8000)); ++ printf(" %s: %d\n", pr_type_strs[6], ++ !!(pr_buff->prin_descriptor.prin_readcap.pr_type_mask & ++ 0x4000)); ++ printf(" %s: %d\n", pr_type_strs[5], ++ !!(pr_buff->prin_descriptor.prin_readcap.pr_type_mask & ++ 0x2000)); ++ printf(" %s: %d\n", pr_type_strs[3], ++ !!(pr_buff->prin_descriptor.prin_readcap.pr_type_mask & ++ 0x800)); ++ printf(" %s: %d\n", pr_type_strs[1], ++ !!(pr_buff->prin_descriptor.prin_readcap.pr_type_mask & ++ 0x200)); ++ printf(" %s: %d\n", pr_type_strs[8], ++ !!(pr_buff->prin_descriptor.prin_readcap.pr_type_mask & 0x1)); + } + } + diff --git a/device-mapper-multipath.spec b/device-mapper-multipath.spec index b30bc76..954173c 100644 --- a/device-mapper-multipath.spec +++ b/device-mapper-multipath.spec @@ -1,6 +1,6 @@ Name: device-mapper-multipath Version: 0.8.7 -Release: 40%{?dist} +Release: 41%{?dist} Summary: Tools to manage multipath devices using device-mapper License: GPLv2 URL: http://christophe.varoqui.free.fr/ @@ -204,6 +204,8 @@ Patch0191: 0191-libmpathpersist-use-check_holding_reservation-in-mpa.patch Patch0192: 0192-libmpathpersist-Fix-unregistering-while-holding-the-.patch Patch0193: 0193-libmpathpersist-Fix-race-between-restoring-a-path-an.patch Patch0194: 0194-multipathd-Fix-tracking-of-old-PR-key.patch +Patch0195: 0195-multipathd-Fix-race-while-registering-PR-key.patch +Patch0196: 0196-mpathpersist-Fix-REPORT-CAPABILITIES-output.patch # runtime Requires: %{name}-libs = %{version}-%{release} @@ -406,6 +408,13 @@ fi %{_pkgconfdir}/libdmmp.pc %changelog +* Tue Nov 11 2025 Benjamin Marzinski - 0.8.7-41 +- Add 0195-multipathd-Fix-race-while-registering-PR-key.patch +- Add 0196-mpathpersist-Fix-REPORT-CAPABILITIES-output.patch + * Fixes RHEL-118515 ("There are many bugs in multipath's persistent + reservation handling") +- Resolves: RHEL-118515 + * Wed Oct 1 2025 Benjamin Marzinski - 0.8.7-40 - Add 0150-libmpathpersist-retry-commands-on-other-paths-in-mpa.patch - Add 0151-libmpathpersist-check-released-key-against-the-reser.patch