subsys: mgmt: Fix broken OTA firmware update

This fixes freeing net_buf without bt_conn_unref call.
As the result, the OTA was broken.

Fixes 8636
Signed-off-by: Mariusz Skamra <mariusz.skamra@codecoup.pl>

include/mgmt/smp.h

@@ -42,6 +42,33 @@ typedef int zephyr_smp_transport_out_fn(struct zephyr_smp_transport *zst,
  */
 typedef uint16_t zephyr_smp_transport_get_mtu_fn(const struct net_buf *nb);
 
+/** @typedef zephyr_smp_transport_ud_copy_fn
+ * @brief SMP copy buffer user_data function for Zephyr.
+ *
+ * The supplied src net_buf should contain a user_data that cannot be copied
+ * using regular memcpy function (e.g., the BLE transport net_buf user_data
+ * stores the connection reference that has to be incremented when is going
+ * to be used by another buffer).
+ *
+ * @param dst                   Source buffer user_data pointer.
+ * @param src                   Destination buffer user_data pointer.
+ *
+ * @return                      0 on success, MGMT_ERR_[...] code on failure.
+ */
+typedef int zephyr_smp_transport_ud_copy_fn(struct net_buf *dst,
+					    const struct net_buf *src);
+
+/** @typedef zephyr_smp_transport_ud_free_fn
+ * @brief SMP free buffer user_data function for Zephyr.
+ *
+ * This function frees net_buf user data, because some transports store
+ * connection-specific information in the net_buf user data (e.g., the BLE
+ * transport stores the connection reference that has to be decreased).
+ *
+ * @param nb                    Contains a user_data pointer to be free'd.
+ */
+typedef void zephyr_smp_transport_ud_free_fn(void *ud);
+
 /**
  * @brief Provides Zephyr-specific functionality for sending SMP responses.
  */
@@ -54,6 +81,8 @@ struct zephyr_smp_transport {
 
 	zephyr_smp_transport_out_fn *zst_output;
 	zephyr_smp_transport_get_mtu_fn *zst_get_mtu;
+	zephyr_smp_transport_ud_copy_fn *zst_ud_copy;
+	zephyr_smp_transport_ud_free_fn *zst_ud_free;
 };
 
 /**
@@ -62,12 +91,16 @@ struct zephyr_smp_transport {
  * @param zst                   The transport to construct.
  * @param output_func           The transport's send function.
  * @param get_mtu_func          The transport's get-MTU function.
+ * @param ud_copy_func          The transport buffer user_data copy function.
+ * @param ud_free_func          The transport buffer user_data free function.
  *
  * @return                      0 on success, MGMT_ERR_[...] code on failure.
  */
 void zephyr_smp_transport_init(struct zephyr_smp_transport *zst,
 			       zephyr_smp_transport_out_fn *output_func,
-			       zephyr_smp_transport_get_mtu_fn *get_mtu_func);
+			       zephyr_smp_transport_get_mtu_fn *get_mtu_func,
+			       zephyr_smp_transport_ud_copy_fn *ud_copy_func,
+			       zephyr_smp_transport_ud_free_fn *ud_free_func);
 
 /**
  * @brief Enqueues an incoming SMP request packet for processing.

subsys/mgmt/smp.c

@@ -36,6 +36,7 @@ zephyr_smp_alloc_rsp(const void *req, void *arg)
 	const struct net_buf_pool *pool;
 	const struct net_buf *req_nb;
 	struct net_buf *rsp_nb;
+	struct zephyr_smp_transport *zst = arg;
 
 	req_nb = req;
 
@@ -44,10 +45,14 @@ zephyr_smp_alloc_rsp(const void *req, void *arg)
 		return NULL;
 	}
 
+	if (zst->zst_ud_copy) {
+		zst->zst_ud_copy(rsp_nb, req_nb);
+	} else {
 		pool = net_buf_pool_get(req_nb->pool_id);
 		memcpy(net_buf_user_data(rsp_nb),
 		       net_buf_user_data((void *)req_nb),
 		       sizeof(req_nb->user_data));
+	}
 
 	return rsp_nb;
 }
@@ -80,7 +85,7 @@ zephyr_smp_trim_front(void *buf, size_t len, void *arg)
  *     struct net_buf *rsp;
  *     // [...]
  *     while (rsp != NULL) {
- *         frag = zephyr_smp_split_frag(&rsp, get_mtu());
+ *         frag = zephyr_smp_split_frag(&rsp, zst, get_mtu());
  *         if (frag == NULL) {
  *             net_buf_unref(nb);
  *             return SYS_ENOMEM;
@@ -93,13 +98,14 @@ zephyr_smp_trim_front(void *buf, size_t len, void *arg)
  *                                  fragment data is removed.  If the packet
  *                                  constitutes a single fragment, this gets
  *                                  set to NULL on success.
+ * @param arg                   The zephyr SMP transport pointer.
  * @param mtu                   The maximum payload size of a fragment.
  *
  * @return                      The next fragment to send on success;
  *                              NULL on failure.
  */
 static struct net_buf *
-zephyr_smp_split_frag(struct net_buf **nb, u16_t mtu)
+zephyr_smp_split_frag(struct net_buf **nb, void *arg, u16_t mtu)
 {
 	struct net_buf *frag;
 	struct net_buf *src;
@@ -110,7 +116,7 @@ zephyr_smp_split_frag(struct net_buf **nb, u16_t mtu)
 		*nb = NULL;
 		frag = src;
 	} else {
-		frag = zephyr_smp_alloc_rsp(src, NULL);
+		frag = zephyr_smp_alloc_rsp(src, arg);
 
 		/* Copy fragment payload into new buffer. */
 		net_buf_add_mem(frag, src->data, mtu);
@@ -176,7 +182,7 @@ zephyr_smp_tx_rsp(struct smp_streamer *ns, void *rsp, void *arg)
 
 	i = 0;
 	while (nb != NULL) {
-		frag = zephyr_smp_split_frag(&nb, mtu);
+		frag = zephyr_smp_split_frag(&nb, zst, mtu);
 		if (frag == NULL) {
 			return MGMT_ERR_ENOMEM;
 		}
@@ -193,6 +199,16 @@ zephyr_smp_tx_rsp(struct smp_streamer *ns, void *rsp, void *arg)
 static void
 zephyr_smp_free_buf(void *buf, void *arg)
 {
+	struct zephyr_smp_transport *zst = arg;
+
+	if (!buf) {
+		return;
+	}
+
+	if (zst->zst_ud_free) {
+		zst->zst_ud_free(net_buf_user_data((struct net_buf *)buf));
+	}
+
 	mcumgr_buf_free(buf);
 }
 
@@ -265,11 +281,15 @@ zephyr_smp_handle_reqs(struct k_work *work)
 void
 zephyr_smp_transport_init(struct zephyr_smp_transport *zst,
 			  zephyr_smp_transport_out_fn *output_func,
-			  zephyr_smp_transport_get_mtu_fn *get_mtu_func)
+			  zephyr_smp_transport_get_mtu_fn *get_mtu_func,
+			  zephyr_smp_transport_ud_copy_fn *ud_copy_func,
+			  zephyr_smp_transport_ud_free_fn *ud_free_func)
 {
 	*zst = (struct zephyr_smp_transport) {
 		.zst_output = output_func,
 		.zst_get_mtu = get_mtu_func,
+		.zst_ud_copy = ud_copy_func,
+		.zst_ud_free = ud_free_func,
 	};
 
 	k_work_init(&zst->zst_work, zephyr_smp_handle_reqs);

subsys/mgmt/smp_bt.c

@@ -127,16 +127,26 @@ static u16_t smp_bt_get_mtu(const struct net_buf *nb)
 	return mtu - 3;
 }
 
-static void smp_bt_buf_free(struct net_buf *nb)
+static void smp_bt_ud_free(void *ud)
 {
-	struct smp_bt_user_data *ud = net_buf_user_data(nb);
+	struct smp_bt_user_data *user_data = ud;
 
-	if (ud->conn) {
+	if (user_data->conn) {
-		bt_conn_unref(ud->conn);
+		bt_conn_unref(user_data->conn);
-		ud->conn = NULL;
+		user_data->conn = NULL;
+	}
+}
+
+static int smp_bt_ud_copy(struct net_buf *dst, const struct net_buf *src)
+{
+	struct smp_bt_user_data *src_ud = net_buf_user_data(src);
+	struct smp_bt_user_data *dst_ud = net_buf_user_data(dst);
+
+	if (src_ud->conn) {
+		dst_ud->conn = bt_conn_ref(src_ud->conn);
 	}
 
-	mcumgr_buf_free(nb);
+	return 0;
 }
 
 /**
@@ -155,7 +165,8 @@ static int smp_bt_tx_pkt(struct zephyr_smp_transport *zst, struct net_buf *nb)
 		bt_conn_unref(conn);
 	}
 
-	smp_bt_buf_free(nb);
+	smp_bt_ud_free(net_buf_user_data(nb));
+	mcumgr_buf_free(nb);
 
 	return rc;
 }
@@ -170,7 +181,8 @@ static int smp_bt_init(struct device *dev)
 	ARG_UNUSED(dev);
 
 	zephyr_smp_transport_init(&smp_bt_transport, smp_bt_tx_pkt,
-				  smp_bt_get_mtu);
+				  smp_bt_get_mtu, smp_bt_ud_copy,
+				  smp_bt_ud_free);
 	return 0;
 }
 

subsys/mgmt/smp_shell.c

@@ -71,7 +71,7 @@ static int smp_shell_init(struct device *dev)
 	ARG_UNUSED(dev);
 
 	zephyr_smp_transport_init(&smp_shell_transport, smp_shell_tx_pkt,
-				  smp_shell_get_mtu);
+				  smp_shell_get_mtu, NULL, NULL);
 	shell_register_mcumgr_handler(smp_shell_rx_line, NULL);
 
 	return 0;

subsys/mgmt/smp_uart.c

@@ -92,7 +92,7 @@ static int smp_uart_init(struct device *dev)
 	ARG_UNUSED(dev);
 
 	zephyr_smp_transport_init(&smp_uart_transport, smp_uart_tx_pkt,
-				  smp_uart_get_mtu);
+				  smp_uart_get_mtu, NULL, NULL);
 	uart_mcumgr_register(smp_uart_rx_frag);
 
 	return 0;