From fd7a0a748211ce018a89d3da4310064944e0d759 Mon Sep 17 00:00:00 2001 From: Igor Ryzhov Date: Thu, 25 Jan 2024 13:54:45 +0200 Subject: [PATCH] mgmtd: fix commit request overwrite There are places, where we can receive an existing commit transaction. If we don't check that the request already exists, it gets overwritten and we start having problems with transaction refcounters. Forbid having multiple configuration sessions simultaneously. Signed-off-by: Igor Ryzhov --- mgmtd/mgmt_be_adapter.c | 21 +++++---------------- mgmtd/mgmt_fe_adapter.c | 5 ++++- mgmtd/mgmt_txn.c | 19 ++++++------------- mgmtd/mgmt_txn.h | 6 +++--- 4 files changed, 18 insertions(+), 33 deletions(-) diff --git a/mgmtd/mgmt_be_adapter.c b/mgmtd/mgmt_be_adapter.c index 4f58973c3619..a7f3d4390e96 100644 --- a/mgmtd/mgmt_be_adapter.c +++ b/mgmtd/mgmt_be_adapter.c @@ -632,28 +632,17 @@ static void mgmt_be_adapter_conn_init(struct event *thread) adapter = (struct mgmt_be_client_adapter *)EVENT_ARG(thread); assert(adapter && adapter->conn->fd >= 0); - /* - * Check first if the current session can run a CONFIG - * transaction or not. Reschedule if a CONFIG transaction - * from another session is already in progress. - */ - if (mgmt_config_txn_in_progress() != MGMTD_SESSION_ID_NONE) { - zlog_err("XXX txn in progress, retry init"); - mgmt_be_adapter_sched_init_event(adapter); - return; - } - /* * Notify TXN module to create a CONFIG transaction and * download the CONFIGs identified for this new client. * If the TXN module fails to initiate the CONFIG transaction - * disconnect from the client forcing a reconnect later. - * That should also take care of destroying the adapter. + * retry a bit later. It only fails if there's an existing config + * transaction in progress. */ if (mgmt_txn_notify_be_adapter_conn(adapter, true) != 0) { - zlog_err("XXX notify be adapter conn fail"); - msg_conn_disconnect(adapter->conn, false); - adapter = NULL; + zlog_err("XXX txn in progress, retry init"); + mgmt_be_adapter_sched_init_event(adapter); + return; } } diff --git a/mgmtd/mgmt_fe_adapter.c b/mgmtd/mgmt_fe_adapter.c index d91987d8884f..ff85f56e3e46 100644 --- a/mgmtd/mgmt_fe_adapter.c +++ b/mgmtd/mgmt_fe_adapter.c @@ -724,7 +724,7 @@ mgmt_fe_session_handle_setcfg_req_msg(struct mgmt_fe_session_ctx *session, if (session->cfg_txn_id == MGMTD_TXN_ID_NONE) { /* as we have the lock no-one else should have a config txn */ - assert(mgmt_config_txn_in_progress() == MGMTD_SESSION_ID_NONE); + assert(!mgmt_config_txn_in_progress()); /* Start a CONFIG Transaction (if not started already) */ session->cfg_txn_id = mgmt_create_txn(session->session_id, @@ -890,6 +890,9 @@ static int mgmt_fe_session_handle_commit_config_req_msg( } if (session->cfg_txn_id == MGMTD_TXN_ID_NONE) { + /* as we have the lock no-one else should have a config txn */ + assert(!mgmt_config_txn_in_progress()); + /* * Start a CONFIG Transaction (if not started already) */ diff --git a/mgmtd/mgmt_txn.c b/mgmtd/mgmt_txn.c index 45bc3a293004..786fa0d0a3e1 100644 --- a/mgmtd/mgmt_txn.c +++ b/mgmtd/mgmt_txn.c @@ -1723,15 +1723,9 @@ static struct mgmt_txn_ctx *mgmt_txn_create_new(uint64_t session_id, { struct mgmt_txn_ctx *txn = NULL; - /* - * For 'CONFIG' transaction check if one is already created - * or not. TODO: figure out what code counts on this and fix it. - */ - if (type == MGMTD_TXN_TYPE_CONFIG && mgmt_txn_mm->cfg_txn) { - if (mgmt_config_txn_in_progress() == session_id) - txn = mgmt_txn_mm->cfg_txn; - goto mgmt_create_txn_done; - } + /* Do not allow multiple config transactions */ + if (type == MGMTD_TXN_TYPE_CONFIG && mgmt_config_txn_in_progress()) + return NULL; txn = mgmt_fe_find_txn_by_session_id(mgmt_txn_mm, session_id, type); if (!txn) { @@ -1761,7 +1755,6 @@ static struct mgmt_txn_ctx *mgmt_txn_create_new(uint64_t session_id, MGMTD_TXN_LOCK(txn); } -mgmt_create_txn_done: return txn; } @@ -1949,12 +1942,12 @@ void mgmt_txn_destroy(void) mgmt_txn_hash_destroy(); } -uint64_t mgmt_config_txn_in_progress(void) +bool mgmt_config_txn_in_progress(void) { if (mgmt_txn_mm && mgmt_txn_mm->cfg_txn) - return mgmt_txn_mm->cfg_txn->session_id; + return true; - return MGMTD_SESSION_ID_NONE; + return false; } uint64_t mgmt_create_txn(uint64_t session_id, enum mgmt_txn_type type) diff --git a/mgmtd/mgmt_txn.h b/mgmtd/mgmt_txn.h index 3f27f2f07b78..02b2baa95fd9 100644 --- a/mgmtd/mgmt_txn.h +++ b/mgmtd/mgmt_txn.h @@ -71,12 +71,12 @@ extern int mgmt_txn_init(struct mgmt_master *cm, struct event_loop *tm); extern void mgmt_txn_destroy(void); /* - * Check if transaction is in progress. + * Check if configuration transaction is in progress. * * Returns: - * session ID if in-progress, MGMTD_SESSION_ID_NONE otherwise. + * true if in-progress, false otherwise. */ -extern uint64_t mgmt_config_txn_in_progress(void); +extern bool mgmt_config_txn_in_progress(void); /** * Get the session ID associated with the given ``txn-id``.