[action] [PR:22408] Ported the fix https://github.com/FRRouting/frr/pull/18601 from FRR community

src/sonic-frr/patch/0057-mgmtd-remove-bogus-hedge-code-which-corrupted-active.patch

@@ -0,0 +1,56 @@
+From b12b4c28b4c4a76cbc906b703ee5a694a082ab74 Mon Sep 17 00:00:00 2001
+From: Christian Hopps <chopps@labn.net>
+Date: Tue, 8 Apr 2025 05:15:53 +0000
+Subject: [PATCH] mgmtd: remove bogus "hedge" code which corrupted active
+ candidate DS
+
+Say you have 2 mgmtd frontend sessions (2 vtysh's) the first one is long
+running and is actively changing the global candidate datastore (DS),
+the second one starts and exits, this code would then copy running
+back over the candidate, blowing away any changes made by the first
+session.
+
+(the long running session could technically be any user)
+
+Instead we need to trust the various cleanup code that already exits.
+For example in the commit_cfg_reply on success candidate is copied to
+running, and on failure *for implicit commit* running is copied back to
+candidate clearing the change. This leaves the non-implicit
+configuration changes in this case we actually want candidate to keep
+it's changes in transactional cases, in the other case of pending commit
+during a file read the code restores candidate (if needed) on exit from
+"config terminal", with this call stack:
+
+ vty_config_node_exit()
+   nb_cli_pending_commit_check()
+     nb_cli_classic_commit()
+       nb_candidate_commit_prepare() [fail] -> copy running -> candidate
+       nb_candidate_commit_apply() -> copy candidate -> running
+
+fixes #18541
+
+Signed-off-by: Christian Hopps <chopps@labn.net>
+---
+ mgmtd/mgmt_fe_adapter.c | 6 ------
+ 1 file changed, 6 deletions(-)
+
+diff --git a/mgmtd/mgmt_fe_adapter.c b/mgmtd/mgmt_fe_adapter.c
+index 8d59198803..d077e08679 100644
+--- a/mgmtd/mgmt_fe_adapter.c
++++ b/mgmtd/mgmt_fe_adapter.c
+@@ -216,12 +216,6 @@ static void mgmt_fe_session_unlock_ds(Mgmtd__DatastoreId ds_id,
+ static void
+ mgmt_fe_session_cfg_txn_cleanup(struct mgmt_fe_session_ctx *session)
+ {
+-	/*
+-	 * Ensure any uncommitted changes in Candidate DS
+-	 * is discarded.
+-	 */
+-	mgmt_ds_copy_dss(mm->running_ds, mm->candidate_ds, false);
+-
+ 	/*
+ 	 * Destroy the actual transaction created earlier.
+ 	 */
+-- 
+2.39.4
+

src/sonic-frr/patch/0058-mgmtd-normalize-argument-order-to-copy-dst-src.patch

@@ -0,0 +1,164 @@
+From 59d2368b0f055f28aeda8f6080d686acfa35c20b Mon Sep 17 00:00:00 2001
+From: Christian Hopps <chopps@labn.net>
+Date: Tue, 8 Apr 2025 05:55:03 +0000
+Subject: [PATCH] mgmtd: normalize argument order to copy(dst, src)
+
+Having just completed a code audit during RCA, the fact that we have 2
+different argument orders for the related datastore copying functions
+was unnecessary and super confusing.
+
+Fix this code-maintenance/comprehension mistake and move the newer mgmtd
+copy routines to use the same arg order as the pre-existing underlying
+northbound copy functions (i.e., use `copy(dst, src)`)
+
+Signed-off-by: Christian Hopps <chopps@labn.net>
+---
+ mgmtd/mgmt_ds.c  | 19 ++++++++-----------
+ mgmtd/mgmt_ds.h  | 12 +++++-------
+ mgmtd/mgmt_txn.c | 26 ++++++++++----------------
+ 3 files changed, 23 insertions(+), 34 deletions(-)
+
+diff --git a/mgmtd/mgmt_ds.c b/mgmtd/mgmt_ds.c
+index dabae4afd1..e83980f97b 100644
+--- a/mgmtd/mgmt_ds.c
++++ b/mgmtd/mgmt_ds.c
+@@ -74,8 +74,7 @@ static int mgmt_ds_dump_in_memory(struct mgmt_ds_ctx *ds_ctx,
+ 	return 0;
+ }
+
+-static int mgmt_ds_replace_dst_with_src_ds(struct mgmt_ds_ctx *src,
+-					   struct mgmt_ds_ctx *dst)
++static int ds_copy(struct mgmt_ds_ctx *dst, struct mgmt_ds_ctx *src)
+ {
+ 	if (!src || !dst)
+ 		return -1;
+@@ -95,8 +94,7 @@ static int mgmt_ds_replace_dst_with_src_ds(struct mgmt_ds_ctx *src,
+ 	return 0;
+ }
+
+-static int mgmt_ds_merge_src_with_dst_ds(struct mgmt_ds_ctx *src,
+-					 struct mgmt_ds_ctx *dst)
++static int ds_merge(struct mgmt_ds_ctx *dst, struct mgmt_ds_ctx *src)
+ {
+ 	int ret;
+
+@@ -251,14 +249,13 @@ void mgmt_ds_unlock(struct mgmt_ds_ctx *ds_ctx)
+ 	ds_ctx->locked = 0;
+ }
+
+-int mgmt_ds_copy_dss(struct mgmt_ds_ctx *src_ds_ctx,
+-		     struct mgmt_ds_ctx *dst_ds_ctx, bool updt_cmt_rec)
++int mgmt_ds_copy_dss(struct mgmt_ds_ctx *dst, struct mgmt_ds_ctx *src, bool updt_cmt_rec)
+ {
+-	if (mgmt_ds_replace_dst_with_src_ds(src_ds_ctx, dst_ds_ctx) != 0)
++	if (ds_copy(dst, src) != 0)
+ 		return -1;
+
+-	if (updt_cmt_rec && dst_ds_ctx->ds_id == MGMTD_DS_RUNNING)
+-		mgmt_history_new_record(dst_ds_ctx);
++	if (updt_cmt_rec && dst->ds_id == MGMTD_DS_RUNNING)
++		mgmt_history_new_record(dst);
+
+ 	return 0;
+ }
+@@ -416,9 +413,9 @@ int mgmt_ds_load_config_from_file(struct mgmt_ds_ctx *dst,
+ 	parsed.ds_id = dst->ds_id;
+
+ 	if (merge)
+-		mgmt_ds_merge_src_with_dst_ds(&parsed, dst);
++		ds_merge(dst, &parsed);
+ 	else
+-		mgmt_ds_replace_dst_with_src_ds(&parsed, dst);
++		ds_copy(dst, &parsed);
+
+ 	nb_config_free(parsed.root.cfg_root);
+
+diff --git a/mgmtd/mgmt_ds.h b/mgmtd/mgmt_ds.h
+index b8e77e330a..f7e1d7c5ee 100644
+--- a/mgmtd/mgmt_ds.h
++++ b/mgmtd/mgmt_ds.h
+@@ -196,21 +196,19 @@ extern void mgmt_ds_unlock(struct mgmt_ds_ctx *ds_ctx);
+ /*
+  * Copy from source to destination datastore.
+  *
+- * src_ds
+- *    Source datastore handle (ds to be copied from).
+- *
+- * dst_ds
++ * dst
+  *    Destination datastore handle (ds to be copied to).
+  *
++ * src
++ *    Source datastore handle (ds to be copied from).
++ *
+  * update_cmd_rec
+  *    TRUE if need to update commit record, FALSE otherwise.
+  *
+  * Returns:
+  *    0 on success, -1 on failure.
+  */
+-extern int mgmt_ds_copy_dss(struct mgmt_ds_ctx *src_ds_ctx,
+-			    struct mgmt_ds_ctx *dst_ds_ctx,
+-			    bool update_cmt_rec);
++extern int mgmt_ds_copy_dss(struct mgmt_ds_ctx *dst, struct mgmt_ds_ctx *src, bool update_cmt_rec);
+
+ /*
+  * Fetch northbound configuration for a given datastore context.
+diff --git a/mgmtd/mgmt_txn.c b/mgmtd/mgmt_txn.c
+index 483dfab8e8..2b4734e971 100644
+--- a/mgmtd/mgmt_txn.c
++++ b/mgmtd/mgmt_txn.c
+@@ -764,17 +764,15 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn,
+ 			 !txn->commit_cfg_req->req.commit_cfg.rollback);
+
+ 		/*
+-		 * Successful commit: Merge Src DS into Dst DS if and only if
++		 * Successful commit: Copy Src DS to Dst DS if and only if
+ 		 * this was not a validate-only or abort request.
+ 		 */
+ 		if ((txn->session_id &&
+ 		     !txn->commit_cfg_req->req.commit_cfg.validate_only &&
+ 		     !txn->commit_cfg_req->req.commit_cfg.abort) ||
+ 		    txn->commit_cfg_req->req.commit_cfg.rollback) {
+-			mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg
+-						 .src_ds_ctx,
+-					 txn->commit_cfg_req->req.commit_cfg
+-						 .dst_ds_ctx,
++			mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx,
++					 txn->commit_cfg_req->req.commit_cfg.src_ds_ctx,
+ 					 create_cmt_info_rec);
+ 		}
+
+@@ -783,22 +781,18 @@ static int mgmt_txn_send_commit_cfg_reply(struct mgmt_txn_ctx *txn,
+ 		 * request.
+ 		 */
+ 		if (txn->session_id && txn->commit_cfg_req->req.commit_cfg.abort)
+-			mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg
+-						 .dst_ds_ctx,
+-					 txn->commit_cfg_req->req.commit_cfg
+-						 .src_ds_ctx,
+-					 false);
++			mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg.src_ds_ctx,
++					 txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx, false);
+ 	} else {
+ 		/*
+ 		 * The commit has failied. For implicit commit requests restore
+-		 * back the contents of the candidate DS.
++		 * back the contents of the candidate DS. For non-implicit
++		 * commit we want to allow the user to re-commit on the changes
++		 * (whether further modified or not).
+ 		 */
+ 		if (txn->commit_cfg_req->req.commit_cfg.implicit)
+-			mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg
+-						 .dst_ds_ctx,
+-					 txn->commit_cfg_req->req.commit_cfg
+-						 .src_ds_ctx,
+-					 false);
++			mgmt_ds_copy_dss(txn->commit_cfg_req->req.commit_cfg.src_ds_ctx,
++					 txn->commit_cfg_req->req.commit_cfg.dst_ds_ctx, false);
+ 	}
+
+ 	if (txn->commit_cfg_req->req.commit_cfg.rollback) {
+-- 
+2.39.4
+

src/sonic-frr/patch/series

@@ -54,3 +54,5 @@
 0054-zebra-Prevent-active-setting-if-interface-is-not-ope.patch
 0055-zebra-Add-nexthop-group-id-to-route-dump.patch
 0056-zebra-Display-interface-name-not-ifindex-in-nh-dump.patch
+0057-mgmtd-remove-bogus-hedge-code-which-corrupted-active.patch
+0058-mgmtd-normalize-argument-order-to-copy-dst-src.patch