From 0f7a7ccffce97eac0a5ea857f06983caee5cc542 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Narajowski?= Date: Tue, 10 Apr 2018 12:10:09 +0200 Subject: [PATCH] mesh: cfg_cli: Fix possible race condition If the thread that sends the configuration messages has low priority and is sending to the local node (a common use case currently) it's possible that the response arrives before the cli->op_* state variables are set, resulting in the message never getting properly processed and the client API call timing out. Split the initialization into a separete cli_prepare() call and add a cli_reset() to clean up the variables in case of premature completion of the client operation (e.g. due to message sending failure). --- nimble/host/mesh/src/cfg_cli.c | 114 +++++++++++++++++++----------- nimble/host/mesh/src/health_cli.c | 62 ++++++++++------ 2 files changed, 112 insertions(+), 64 deletions(-) diff --git a/nimble/host/mesh/src/cfg_cli.c b/nimble/host/mesh/src/cfg_cli.c index 49c0a4160..99def69ff 100644 --- a/nimble/host/mesh/src/cfg_cli.c +++ b/nimble/host/mesh/src/cfg_cli.c @@ -474,7 +474,7 @@ const struct bt_mesh_model_op bt_mesh_cfg_cli_op[] = { BT_MESH_MODEL_OP_END, }; -static int check_cli(void) +static int cli_prepare(void *param, u32_t op) { if (!cli) { BT_ERR("No available Configuration Client context!"); @@ -486,20 +486,25 @@ static int check_cli(void) return -EBUSY; } - return 0; -} - -static int cli_wait(void *param, u32_t op) -{ - int err; - cli->op_param = param; cli->op_pending = op; - err = k_sem_take(&cli->op_sync, msg_timeout); + return 0; +} +static void cli_reset(void) +{ cli->op_pending = 0; cli->op_param = NULL; +} + +static int cli_wait(void) +{ + int err; + + err = k_sem_take(&cli->op_sync, msg_timeout); + + cli_reset(); return err; } @@ -520,7 +525,7 @@ int bt_mesh_cfg_comp_data_get(u16_t net_idx, u16_t addr, u8_t page, }; int err; - err = check_cli(); + err = cli_prepare(¶m, OP_DEV_COMP_DATA_STATUS); if (err) { goto done; } @@ -531,10 +536,11 @@ int bt_mesh_cfg_comp_data_get(u16_t net_idx, u16_t addr, u8_t page, err = bt_mesh_model_send(cli->model, &ctx, msg, NULL, NULL); if (err) { BT_ERR("model_send() failed (err %d)", err); + cli_reset(); goto done; } - err = cli_wait(¶m, OP_DEV_COMP_DATA_STATUS); + err = cli_wait(); done: os_mbuf_free_chain(msg); return err; @@ -552,7 +558,7 @@ static int get_state_u8(u16_t net_idx, u16_t addr, u32_t op, u32_t rsp, }; int err; - err = check_cli(); + err = cli_prepare(val, rsp); if (err) { goto done; } @@ -562,10 +568,11 @@ static int get_state_u8(u16_t net_idx, u16_t addr, u32_t op, u32_t rsp, err = bt_mesh_model_send(cli->model, &ctx, msg, NULL, NULL); if (err) { BT_ERR("model_send() failed (err %d)", err); + cli_reset(); goto done; } - err = cli_wait(val, rsp); + err = cli_wait(); done: os_mbuf_free_chain(msg); return err; @@ -583,7 +590,7 @@ static int set_state_u8(u16_t net_idx, u16_t addr, u32_t op, u32_t rsp, }; int err; - err = check_cli(); + err = cli_prepare(val, rsp); if (err) { goto done; } @@ -594,10 +601,11 @@ static int set_state_u8(u16_t net_idx, u16_t addr, u32_t op, u32_t rsp, err = bt_mesh_model_send(cli->model, &ctx, msg, NULL, NULL); if (err) { BT_ERR("model_send() failed (err %d)", err); + cli_reset(); goto done; } - err = cli_wait(val, rsp); + err = cli_wait(); done: os_mbuf_free_chain(msg); return err; @@ -668,7 +676,7 @@ int bt_mesh_cfg_relay_get(u16_t net_idx, u16_t addr, u8_t *status, }; int err; - err = check_cli(); + err = cli_prepare(¶m, OP_RELAY_STATUS); if (err) { goto done; } @@ -678,10 +686,11 @@ int bt_mesh_cfg_relay_get(u16_t net_idx, u16_t addr, u8_t *status, err = bt_mesh_model_send(cli->model, &ctx, msg, NULL, NULL); if (err) { BT_ERR("model_send() failed (err %d)", err); + cli_reset(); goto done; } - err = cli_wait(¶m, OP_RELAY_STATUS); + err = cli_wait(); done: os_mbuf_free_chain(msg); return err; @@ -703,7 +712,7 @@ int bt_mesh_cfg_relay_set(u16_t net_idx, u16_t addr, u8_t new_relay, }; int err; - err = check_cli(); + err = cli_prepare(¶m, OP_RELAY_STATUS); if (err) { goto done; } @@ -715,10 +724,11 @@ int bt_mesh_cfg_relay_set(u16_t net_idx, u16_t addr, u8_t new_relay, err = bt_mesh_model_send(cli->model, &ctx, msg, NULL, NULL); if (err) { BT_ERR("model_send() failed (err %d)", err); + cli_reset(); goto done; } - err = cli_wait(¶m, OP_RELAY_STATUS); + err = cli_wait(); done: os_mbuf_free_chain(msg); return err; @@ -740,7 +750,7 @@ int bt_mesh_cfg_net_key_add(u16_t net_idx, u16_t addr, u16_t key_net_idx, }; int err; - err = check_cli(); + err = cli_prepare(¶m, OP_NET_KEY_STATUS); if (err) { goto done; } @@ -752,14 +762,16 @@ int bt_mesh_cfg_net_key_add(u16_t net_idx, u16_t addr, u16_t key_net_idx, err = bt_mesh_model_send(cli->model, &ctx, msg, NULL, NULL); if (err) { BT_ERR("model_send() failed (err %d)", err); + cli_reset(); goto done; } if (!status) { + cli_reset(); goto done; } - err = cli_wait(¶m, OP_NET_KEY_STATUS); + err = cli_wait(); done: os_mbuf_free_chain(msg); return err; @@ -783,7 +795,7 @@ int bt_mesh_cfg_app_key_add(u16_t net_idx, u16_t addr, u16_t key_net_idx, }; int err; - err = check_cli(); + err = cli_prepare(¶m, OP_APP_KEY_STATUS); if (err) { goto done; } @@ -795,14 +807,16 @@ int bt_mesh_cfg_app_key_add(u16_t net_idx, u16_t addr, u16_t key_net_idx, err = bt_mesh_model_send(cli->model, &ctx, msg, NULL, NULL); if (err) { BT_ERR("model_send() failed (err %d)", err); + cli_reset(); goto done; } if (!status) { + cli_reset(); goto done; } - err = cli_wait(¶m, OP_APP_KEY_STATUS); + err = cli_wait(); done: os_mbuf_free_chain(msg); return err; @@ -828,7 +842,7 @@ static int mod_app_bind(u16_t net_idx, u16_t addr, u16_t elem_addr, }; int err; - err = check_cli(); + err = cli_prepare(¶m, OP_MOD_APP_STATUS); if (err) { goto done; } @@ -846,14 +860,16 @@ static int mod_app_bind(u16_t net_idx, u16_t addr, u16_t elem_addr, err = bt_mesh_model_send(cli->model, &ctx, msg, NULL, NULL); if (err) { BT_ERR("model_send() failed (err %d)", err); + cli_reset(); goto done; } if (!status) { + cli_reset(); goto done; } - err = cli_wait(¶m, OP_MOD_APP_STATUS); + err = cli_wait(); done: os_mbuf_free_chain(msg); return err; @@ -897,7 +913,7 @@ static int mod_sub(u32_t op, u16_t net_idx, u16_t addr, u16_t elem_addr, }; int err; - err = check_cli(); + err = cli_prepare(¶m, OP_MOD_SUB_STATUS); if (err) { goto done; } @@ -915,14 +931,16 @@ static int mod_sub(u32_t op, u16_t net_idx, u16_t addr, u16_t elem_addr, err = bt_mesh_model_send(cli->model, &ctx, msg, NULL, NULL); if (err) { BT_ERR("model_send() failed (err %d)", err); + cli_reset(); goto done; } if (!status) { + cli_reset(); goto done; } - err = cli_wait(¶m, OP_MOD_SUB_STATUS); + err = cli_wait(); done: os_mbuf_free_chain(msg); return err; @@ -1005,7 +1023,7 @@ static int mod_sub_va(u32_t op, u16_t net_idx, u16_t addr, u16_t elem_addr, }; int err; - err = check_cli(); + err = cli_prepare(¶m, OP_MOD_SUB_STATUS); if (err) { goto done; } @@ -1027,14 +1045,16 @@ static int mod_sub_va(u32_t op, u16_t net_idx, u16_t addr, u16_t elem_addr, err = bt_mesh_model_send(cli->model, &ctx, msg, NULL, NULL); if (err) { BT_ERR("model_send() failed (err %d)", err); + cli_reset(); goto done; } if (!status) { + cli_reset(); goto done; } - err = cli_wait(¶m, OP_MOD_SUB_STATUS); + err = cli_wait(); done: os_mbuf_free_chain(msg); return err; @@ -1122,7 +1142,7 @@ static int mod_pub_get(u16_t net_idx, u16_t addr, u16_t elem_addr, }; int err; - err = check_cli(); + err = cli_prepare(¶m, OP_MOD_PUB_STATUS); if (err) { goto done; } @@ -1140,14 +1160,16 @@ static int mod_pub_get(u16_t net_idx, u16_t addr, u16_t elem_addr, err = bt_mesh_model_send(cli->model, &ctx, msg, NULL, NULL); if (err) { BT_ERR("model_send() failed (err %d)", err); + cli_reset(); goto done; } if (!status) { + cli_reset(); goto done; } - err = cli_wait(¶m, OP_MOD_PUB_STATUS); + err = cli_wait(); done: os_mbuf_free_chain(msg); return err; @@ -1192,7 +1214,7 @@ static int mod_pub_set(u16_t net_idx, u16_t addr, u16_t elem_addr, }; int err; - err = check_cli(); + err = cli_prepare(¶m, OP_MOD_PUB_STATUS); if (err) { goto done; } @@ -1215,14 +1237,16 @@ static int mod_pub_set(u16_t net_idx, u16_t addr, u16_t elem_addr, err = bt_mesh_model_send(cli->model, &ctx, msg, NULL, NULL); if (err) { BT_ERR("model_send() failed (err %d)", err); + cli_reset(); goto done; } if (!status) { + cli_reset(); goto done; } - err = cli_wait(¶m, OP_MOD_PUB_STATUS); + err = cli_wait(); done: os_mbuf_free_chain(msg); return err; @@ -1263,7 +1287,7 @@ int bt_mesh_cfg_hb_sub_set(u16_t net_idx, u16_t addr, }; int err; - err = check_cli(); + err = cli_prepare(¶m, OP_HEARTBEAT_SUB_STATUS); if (err) { goto done; } @@ -1276,14 +1300,16 @@ int bt_mesh_cfg_hb_sub_set(u16_t net_idx, u16_t addr, err = bt_mesh_model_send(cli->model, &ctx, msg, NULL, NULL); if (err) { BT_ERR("model_send() failed (err %d)", err); + cli_reset(); goto done; } if (!status) { + cli_reset(); goto done; } - err = cli_wait(¶m, OP_HEARTBEAT_SUB_STATUS); + err = cli_wait(); done: os_mbuf_free_chain(msg); return err; @@ -1305,7 +1331,7 @@ int bt_mesh_cfg_hb_sub_get(u16_t net_idx, u16_t addr, }; int err; - err = check_cli(); + err = cli_prepare(¶m, OP_HEARTBEAT_SUB_STATUS); if (err) { goto done; } @@ -1315,14 +1341,16 @@ int bt_mesh_cfg_hb_sub_get(u16_t net_idx, u16_t addr, err = bt_mesh_model_send(cli->model, &ctx, msg, NULL, NULL); if (err) { BT_ERR("model_send() failed (err %d)", err); + cli_reset(); goto done; } if (!status) { + cli_reset(); goto done; } - err = cli_wait(¶m, OP_HEARTBEAT_SUB_STATUS); + err = cli_wait(); done: os_mbuf_free_chain(msg); return err; @@ -1343,7 +1371,7 @@ int bt_mesh_cfg_hb_pub_set(u16_t net_idx, u16_t addr, }; int err; - err = check_cli(); + err = cli_prepare(¶m, OP_HEARTBEAT_PUB_STATUS); if (err) { goto done; } @@ -1359,14 +1387,16 @@ int bt_mesh_cfg_hb_pub_set(u16_t net_idx, u16_t addr, err = bt_mesh_model_send(cli->model, &ctx, msg, NULL, NULL); if (err) { BT_ERR("model_send() failed (err %d)", err); + cli_reset(); goto done; } if (!status) { + cli_reset(); goto done; } - err = cli_wait(¶m, OP_HEARTBEAT_PUB_STATUS); + err = cli_wait(); done: os_mbuf_free_chain(msg); return err; @@ -1388,7 +1418,7 @@ int bt_mesh_cfg_hb_pub_get(u16_t net_idx, u16_t addr, }; int err; - err = check_cli(); + err = cli_prepare(¶m, OP_HEARTBEAT_PUB_STATUS); if (err) { goto done; } @@ -1398,14 +1428,16 @@ int bt_mesh_cfg_hb_pub_get(u16_t net_idx, u16_t addr, err = bt_mesh_model_send(cli->model, &ctx, msg, NULL, NULL); if (err) { BT_ERR("model_send() failed (err %d)", err); + cli_reset(); goto done; } if (!status) { + cli_reset(); goto done; } - err = cli_wait(¶m, OP_HEARTBEAT_PUB_STATUS); + err = cli_wait(); done: os_mbuf_free_chain(msg); return err; diff --git a/nimble/host/mesh/src/health_cli.c b/nimble/host/mesh/src/health_cli.c index 2f6a209f9..38fc3438f 100644 --- a/nimble/host/mesh/src/health_cli.c +++ b/nimble/host/mesh/src/health_cli.c @@ -167,7 +167,7 @@ const struct bt_mesh_model_op bt_mesh_health_cli_op[] = { BT_MESH_MODEL_OP_END, }; -static int check_cli(void) +static int cli_prepare(void *param, u32_t op) { if (!health_cli) { BT_ERR("No available Health Client context!"); @@ -179,20 +179,25 @@ static int check_cli(void) return -EBUSY; } - return 0; -} - -static int cli_wait(void *param, u32_t op) -{ - int err; - health_cli->op_param = param; health_cli->op_pending = op; - err = k_sem_take(&health_cli->op_sync, msg_timeout); + return 0; +} +static void cli_reset(void) +{ health_cli->op_pending = 0; health_cli->op_param = NULL; +} + +static int cli_wait(void) +{ + int err; + + err = k_sem_take(&health_cli->op_sync, msg_timeout); + + cli_reset(); return err; } @@ -212,7 +217,7 @@ int bt_mesh_health_attention_get(u16_t net_idx, u16_t addr, u16_t app_idx, }; int err; - err = check_cli(); + err = cli_prepare(¶m, OP_ATTENTION_STATUS); if (err) { goto done; } @@ -222,10 +227,11 @@ int bt_mesh_health_attention_get(u16_t net_idx, u16_t addr, u16_t app_idx, err = bt_mesh_model_send(health_cli->model, &ctx, msg, NULL, NULL); if (err) { BT_ERR("model_send() failed (err %d)", err); + cli_reset(); goto done; } - err = cli_wait(¶m, OP_ATTENTION_STATUS); + err = cli_wait(); done: os_mbuf_free_chain(msg); return err; @@ -246,7 +252,7 @@ int bt_mesh_health_attention_set(u16_t net_idx, u16_t addr, u16_t app_idx, }; int err; - err = check_cli(); + err = cli_prepare(¶m, OP_ATTENTION_STATUS); if (err) { goto done; } @@ -262,14 +268,16 @@ int bt_mesh_health_attention_set(u16_t net_idx, u16_t addr, u16_t app_idx, err = bt_mesh_model_send(health_cli->model, &ctx, msg, NULL, NULL); if (err) { BT_ERR("model_send() failed (err %d)", err); + cli_reset(); goto done; } if (!updated_attention) { + cli_reset(); goto done; } - err = cli_wait(¶m, OP_ATTENTION_STATUS); + err = cli_wait(); done: os_mbuf_free_chain(msg); return err; @@ -290,7 +298,7 @@ int bt_mesh_health_period_get(u16_t net_idx, u16_t addr, u16_t app_idx, }; int err; - err = check_cli(); + err = cli_prepare(¶m, OP_HEALTH_PERIOD_STATUS); if (err) { goto done; } @@ -300,10 +308,11 @@ int bt_mesh_health_period_get(u16_t net_idx, u16_t addr, u16_t app_idx, err = bt_mesh_model_send(health_cli->model, &ctx, msg, NULL, NULL); if (err) { BT_ERR("model_send() failed (err %d)", err); + cli_reset(); goto done; } - err = cli_wait(¶m, OP_HEALTH_PERIOD_STATUS); + err = cli_wait(); done: os_mbuf_free_chain(msg); return err; @@ -324,7 +333,7 @@ int bt_mesh_health_period_set(u16_t net_idx, u16_t addr, u16_t app_idx, }; int err; - err = check_cli(); + err = cli_prepare(¶m, OP_HEALTH_PERIOD_STATUS); if (err) { goto done; } @@ -340,14 +349,16 @@ int bt_mesh_health_period_set(u16_t net_idx, u16_t addr, u16_t app_idx, err = bt_mesh_model_send(health_cli->model, &ctx, msg, NULL, NULL); if (err) { BT_ERR("model_send() failed (err %d)", err); + cli_reset(); goto done; } if (!updated_divisor) { + cli_reset(); goto done; } - err = cli_wait(¶m, OP_HEALTH_PERIOD_STATUS); + err = cli_wait(); done: os_mbuf_free_chain(msg); return err; @@ -372,7 +383,7 @@ int bt_mesh_health_fault_test(u16_t net_idx, u16_t addr, u16_t app_idx, }; int err; - err = check_cli(); + err = cli_prepare(¶m, OP_HEALTH_FAULT_STATUS); if (err) { goto done; } @@ -389,14 +400,16 @@ int bt_mesh_health_fault_test(u16_t net_idx, u16_t addr, u16_t app_idx, err = bt_mesh_model_send(health_cli->model, &ctx, msg, NULL, NULL); if (err) { BT_ERR("model_send() failed (err %d)", err); + cli_reset(); goto done; } if (!faults) { + cli_reset(); goto done; } - err = cli_wait(¶m, OP_HEALTH_FAULT_STATUS); + err = cli_wait(); done: os_mbuf_free_chain(msg); return err; @@ -421,7 +434,7 @@ int bt_mesh_health_fault_clear(u16_t net_idx, u16_t addr, u16_t app_idx, }; int err; - err = check_cli(); + err = cli_prepare(¶m, OP_HEALTH_FAULT_STATUS); if (err) { goto done; } @@ -437,14 +450,16 @@ int bt_mesh_health_fault_clear(u16_t net_idx, u16_t addr, u16_t app_idx, err = bt_mesh_model_send(health_cli->model, &ctx, msg, NULL, NULL); if (err) { BT_ERR("model_send() failed (err %d)", err); + cli_reset(); goto done; } if (!test_id) { + cli_reset(); goto done; } - err = cli_wait(¶m, OP_HEALTH_FAULT_STATUS); + err = cli_wait(); done: os_mbuf_free_chain(msg); return err; @@ -469,7 +484,7 @@ int bt_mesh_health_fault_get(u16_t net_idx, u16_t addr, u16_t app_idx, }; int err; - err = check_cli(); + err = cli_prepare(¶m, OP_HEALTH_FAULT_STATUS); if (err) { goto done; } @@ -480,10 +495,11 @@ int bt_mesh_health_fault_get(u16_t net_idx, u16_t addr, u16_t app_idx, err = bt_mesh_model_send(health_cli->model, &ctx, msg, NULL, NULL); if (err) { BT_ERR("model_send() failed (err %d)", err); + cli_reset(); goto done; } - err = cli_wait(¶m, OP_HEALTH_FAULT_STATUS); + err = cli_wait(); done: os_mbuf_free_chain(msg); return err;