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 <[email protected]>
zephyrproject-rtos · Dec 6, 2024 · 9fa82f0 · 9fa82f0
1 parent 3163325
commit 9fa82f0
Show file tree

Hide file tree

Showing 3 changed files with 11 additions and 9 deletions.
diff --git 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
@@ -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
@@ -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