From 051c1f5fd61c81899d88d106088e2551b3232a77 Mon Sep 17 00:00:00 2001 From: Mariusz Skamra Date: Mon, 16 Jul 2018 19:22:05 +0200 Subject: [PATCH] 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 --- include/mgmt/smp.h | 35 ++++++++++++++++++++++++++++++++++- subsys/mgmt/smp.c | 38 +++++++++++++++++++++++++++++--------- subsys/mgmt/smp_bt.c | 28 ++++++++++++++++++++-------- subsys/mgmt/smp_shell.c | 2 +- subsys/mgmt/smp_uart.c | 2 +- 5 files changed, 85 insertions(+), 20 deletions(-) diff --git a/include/mgmt/smp.h b/include/mgmt/smp.h index 6916efc1a7c..eea98ae87f3 100644 --- a/include/mgmt/smp.h +++ b/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. diff --git a/subsys/mgmt/smp.c b/subsys/mgmt/smp.c index e595a329695..d1edd5b6de9 100644 --- a/subsys/mgmt/smp.c +++ b/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; } - 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)); + 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); diff --git a/subsys/mgmt/smp_bt.c b/subsys/mgmt/smp_bt.c index 44d5a55c2fc..6f019be86f6 100644 --- a/subsys/mgmt/smp_bt.c +++ b/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) { - bt_conn_unref(ud->conn); - ud->conn = NULL; + if (user_data->conn) { + bt_conn_unref(user_data->conn); + 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; } diff --git a/subsys/mgmt/smp_shell.c b/subsys/mgmt/smp_shell.c index ec999564e8a..e619fa3da1a 100644 --- a/subsys/mgmt/smp_shell.c +++ b/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; diff --git a/subsys/mgmt/smp_uart.c b/subsys/mgmt/smp_uart.c index 0b3c5cf111b..5ff5708a862 100644 --- a/subsys/mgmt/smp_uart.c +++ b/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;