diff --git a/0078-multipathd-Fix-race-while-registering-PR-key.patch b/0078-multipathd-Fix-race-while-registering-PR-key.patch new file mode 100644 index 0000000..981eef0 --- /dev/null +++ b/0078-multipathd-Fix-race-while-registering-PR-key.patch @@ -0,0 +1,239 @@ +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 | 57 ++++++++++++++++++++++++++++----------- + multipathd/main.h | 1 + + 3 files changed, 48 insertions(+), 16 deletions(-) + +diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c +index 1696dbd4..1677dd1e 100644 +--- a/multipathd/cli_handlers.c ++++ b/multipathd/cli_handlers.c +@@ -1281,7 +1281,11 @@ cli_setprstatus(void * v, struct strbuf *reply, 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 e17a3355..0882ef2a 100644 +--- a/multipathd/main.c ++++ b/multipathd/main.c +@@ -90,7 +90,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 { \ +@@ -636,19 +637,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; ++ } + } + } + } +@@ -735,7 +745,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); +@@ -1299,7 +1309,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"); +@@ -1383,7 +1393,7 @@ rescan: + + if (retries >= 0) { + if ((mpp->prflag == PR_SET && prflag != PR_SET) || start_waiter) +- pr_register_active_paths(mpp); ++ pr_register_active_paths(mpp, false); + condlog(2, "%s [%s]: path added to devmap %s", + pp->dev, pp->dev_t, mpp->alias); + return 0; +@@ -2653,10 +2663,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); + } + } + +@@ -3027,7 +3037,7 @@ configure (struct vectors * vecs, enum force_reload_types reload_type) + 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); + } + + /* +@@ -3985,7 +3995,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) + { +@@ -3995,8 +4006,10 @@ 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*/ +@@ -4004,6 +4017,7 @@ static int update_map_pr(struct multipath *mpp, struct path *pp, unsigned int *n + condlog(was_set ? 2 : 4, + "%s: reservation_key not set in multipath.conf", + mpp->alias); ++ *nr_keys = 0; + return MPATH_PR_SUCCESS; + } + +@@ -4015,6 +4029,7 @@ static int update_map_pr(struct multipath *mpp, struct path *pp, unsigned int *n + unset_pr(mpp); + condlog(0, "%s : pr in read keys service action failed Error=%d", + mpp->alias, ret); ++ *nr_keys = 0; + return ret; + } + +@@ -4053,22 +4068,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; +@@ -4084,6 +4109,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 c9b3e0fd..25ddbaa7 100644 +--- a/multipathd/main.h ++++ b/multipathd/main.h +@@ -54,4 +54,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/0079-mpathpersist-Fix-REPORT-CAPABILITIES-output.patch b/0079-mpathpersist-Fix-REPORT-CAPABILITIES-output.patch new file mode 100644 index 0000000..57030a4 --- /dev/null +++ b/0079-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 22c1d4ac..4ac40bed 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 2cacafb7..33ab69cb 100644 +--- a/mpathpersist/main.c ++++ b/mpathpersist/main.c +@@ -748,25 +748,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 bb0423d..d2a136b 100644 --- a/device-mapper-multipath.spec +++ b/device-mapper-multipath.spec @@ -1,6 +1,6 @@ Name: device-mapper-multipath Version: 0.9.9 -Release: 13%{?dist} +Release: 14%{?dist} Summary: Tools to manage multipath devices using device-mapper License: GPLv2 URL: http://christophe.varoqui.free.fr/ @@ -87,6 +87,8 @@ Patch0074: 0074-libmpathpersist-use-check_holding_reservation-in-mpa.patch Patch0075: 0075-libmpathpersist-Fix-unregistering-while-holding-the-.patch Patch0076: 0076-libmpathpersist-Fix-race-between-restoring-a-path-an.patch Patch0077: 0077-multipathd-Fix-tracking-of-old-PR-key.patch +Patch0078: 0078-multipathd-Fix-race-while-registering-PR-key.patch +Patch0079: 0079-mpathpersist-Fix-REPORT-CAPABILITIES-output.patch # runtime Requires: %{name}-libs = %{version}-%{release} @@ -296,6 +298,13 @@ fi %{_pkgconfdir}/libdmmp.pc %changelog +* Tue Nov 11 2025 Benjamin Marzinski - 0.9.9-14 +- Add 0078-multipathd-Fix-race-while-registering-PR-key.patch +- Add 0079-mpathpersist-Fix-REPORT-CAPABILITIES-output.patch + * Fixes RHEL-118720 ("There are many bugs in multipath's persistent + reservation handling [rhel-10]") +- Resolves: RHEL-118720 + * Wed Oct 1 2025 Benjamin Marzinski - 0.9.9-13 - Add 0035-libmpathpersist-fix-memory-leak-in-mpath_prout_rel.patch - Add 0036-libmpathpersist-retry-commands-on-other-paths-in-mpa.patch