From 9fa82f0d8d29d1f808829e16c09bb3942f53aa3b Mon Sep 17 00:00:00 2001 From: Seppo Takalo Date: Wed, 4 Dec 2024 17:14:33 +0200 Subject: [PATCH] mgmt: smp: Fix race condition by using same work queue Fix possible race condition where SMP client might release the network buffer before system worker queue has processed it. In smp_client uses shared resources like worker queue linked list and network buffers without maintaining any thread safety. For unknown reasons, retry timeout handling is pushed into system worker queue while the actual transmission is handled from SMP work queue. Fix the issue by using the same SMP work queue for both delayable timeout handling as well as transmission handling. Signed-off-by: Seppo Takalo --- subsys/mgmt/mcumgr/smp_client/src/client.c | 10 ++++++---- .../include/mgmt/mcumgr/transport/smp_internal.h | 6 +++--- subsys/mgmt/mcumgr/transport/src/smp.c | 4 ++-- 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/subsys/mgmt/mcumgr/smp_client/src/client.c b/subsys/mgmt/mcumgr/smp_client/src/client.c index 77b09eb1828..c74e6f1ba78 100644 --- a/subsys/mgmt/mcumgr/smp_client/src/client.c +++ b/subsys/mgmt/mcumgr/smp_client/src/client.c @@ -112,7 +112,7 @@ static void smp_client_transport_work_fn(struct k_work *work) entry->retry_cnt--; entry->timestamp = time_stamp_ref + CONFIG_SMP_CMD_RETRY_TIME; k_fifo_put(&entry->smp_client->tx_fifo, entry->nb); - smp_tx_req(&entry->smp_client->work); + k_work_submit_to_queue(smp_get_wq(), &entry->smp_client->work); continue; } @@ -126,7 +126,8 @@ static void smp_client_transport_work_fn(struct k_work *work) if (!sys_slist_is_empty(&smp_client_data.cmd_list)) { /* Re-schedule new timeout to next */ - k_work_reschedule(&smp_client_data.work_delay, K_MSEC(backoff_ms)); + k_work_reschedule_for_queue(smp_get_wq(), &smp_client_data.work_delay, + K_MSEC(backoff_ms)); } } @@ -160,7 +161,8 @@ static void smp_cmd_add_to_list(struct smp_client_cmd_req *cmd_req) { if (sys_slist_is_empty(&smp_client_data.cmd_list)) { /* Enable timer */ - k_work_reschedule(&smp_client_data.work_delay, K_MSEC(CONFIG_SMP_CMD_RETRY_TIME)); + k_work_reschedule_for_queue(smp_get_wq(), &smp_client_data.work_delay, + K_MSEC(CONFIG_SMP_CMD_RETRY_TIME)); } sys_slist_append(&smp_client_data.cmd_list, &cmd_req->node); } @@ -320,7 +322,7 @@ int smp_client_send_cmd(struct smp_client_object *smp_client, struct net_buf *nb nb = net_buf_ref(nb); smp_cmd_add_to_list(cmd_req); k_fifo_put(&smp_client->tx_fifo, nb); - smp_tx_req(&smp_client->work); + k_work_submit_to_queue(smp_get_wq(), &smp_client->work); return MGMT_ERR_EOK; } diff --git a/subsys/mgmt/mcumgr/transport/include/mgmt/mcumgr/transport/smp_internal.h b/subsys/mgmt/mcumgr/transport/include/mgmt/mcumgr/transport/smp_internal.h index 822ca2f5ace..b2318558928 100644 --- a/subsys/mgmt/mcumgr/transport/include/mgmt/mcumgr/transport/smp_internal.h +++ b/subsys/mgmt/mcumgr/transport/include/mgmt/mcumgr/transport/smp_internal.h @@ -50,11 +50,11 @@ void smp_rx_req(struct smp_transport *smtp, struct net_buf *nb); #ifdef CONFIG_SMP_CLIENT /** - * @brief Trig SMP client request packet for transmission. + * @brief Get work queue for SMP client. * - * @param work The transport to use to send the corresponding response(s). + * @return SMP work queue object. */ -void smp_tx_req(struct k_work *work); +struct k_work_q *smp_get_wq(void); #endif /** diff --git a/subsys/mgmt/mcumgr/transport/src/smp.c b/subsys/mgmt/mcumgr/transport/src/smp.c index 53aab569b23..19f02e39d85 100644 --- a/subsys/mgmt/mcumgr/transport/src/smp.c +++ b/subsys/mgmt/mcumgr/transport/src/smp.c @@ -204,9 +204,9 @@ smp_rx_req(struct smp_transport *smpt, struct net_buf *nb) } #ifdef CONFIG_SMP_CLIENT -void smp_tx_req(struct k_work *work) +struct k_work_q *smp_get_wq(void) { - k_work_submit_to_queue(&smp_work_queue, work); + return &smp_work_queue; } #endif