3.16.37-rc1 review patch. If anyone has any objections, please let me know. ------------------ From: Sven Eckelmann <sven@xxxxxxxxxxxxx> commit 9c4604a298e0a9807eaf2cd912d1ebf24d98fbeb upstream. The tt_req_node is added and removed from a list inside a spinlock. But the locking is sometimes removed even when the object is still referenced and will be used later via this reference. For example batadv_send_tt_request can create a new tt_req_node (including add to a list) and later re-acquires the lock to remove it from the list and to free it. But at this time another context could have already removed this tt_req_node from the list and freed it. CPU#0 batadv_batman_skb_recv from net_device 0 -> batadv_iv_ogm_receive -> batadv_iv_ogm_process -> batadv_iv_ogm_process_per_outif -> batadv_tvlv_ogm_receive -> batadv_tvlv_ogm_receive -> batadv_tvlv_containers_process -> batadv_tvlv_call_handler -> batadv_tt_tvlv_ogm_handler_v1 -> batadv_tt_update_orig -> batadv_send_tt_request -> batadv_tt_req_node_new spin_lock(...) allocates new tt_req_node and adds it to list spin_unlock(...) return tt_req_node CPU#1 batadv_batman_skb_recv from net_device 1 -> batadv_recv_unicast_tvlv -> batadv_tvlv_containers_process -> batadv_tvlv_call_handler -> batadv_tt_tvlv_unicast_handler_v1 -> batadv_handle_tt_response spin_lock(...) tt_req_node gets removed from list and is freed spin_unlock(...) CPU#0 <- returned to batadv_send_tt_request spin_lock(...) tt_req_node gets removed from list and is freed MEMORY CORRUPTION/SEGFAULT/... spin_unlock(...) This can only be solved via reference counting to allow multiple contexts to handle the list manipulation while making sure that only the last context holding a reference will free the object. Fixes: a73105b8d4c7 ("batman-adv: improved client announcement mechanism") Signed-off-by: Sven Eckelmann <sven@xxxxxxxxxxxxx> Tested-by: Martin Weinelt <martin@xxxxxxxxxxxxxxxxxxxxxx> Tested-by: Amadeus Alfa <amadeus@xxxxxxxxxxxxxxxxxxxxx> Signed-off-by: Marek Lindner <mareklindner@xxxxxxxxxxxxx> Signed-off-by: David S. Miller <davem@xxxxxxxxxxxxx> [bwh: Backported to 3.16: - Adjust context - Use list_empty() instead of hlist_unhashed()] Signed-off-by: Ben Hutchings <ben@xxxxxxxxxxxxxxx> --- net/batman-adv/translation-table.c | 43 ++++++++++++++++++++++++++++++++------ net/batman-adv/types.h | 2 ++ 2 files changed, 39 insertions(+), 6 deletions(-) --- a/net/batman-adv/translation-table.c +++ b/net/batman-adv/translation-table.c @@ -2165,6 +2165,29 @@ static uint32_t batadv_tt_local_crc(stru return crc; } +/** + * batadv_tt_req_node_release - free tt_req node entry + * @ref: kref pointer of the tt req_node entry + */ +static void batadv_tt_req_node_release(struct kref *ref) +{ + struct batadv_tt_req_node *tt_req_node; + + tt_req_node = container_of(ref, struct batadv_tt_req_node, refcount); + + kfree(tt_req_node); +} + +/** + * batadv_tt_req_node_put - decrement the tt_req_node refcounter and + * possibly release it + * @tt_req_node: tt_req_node to be free'd + */ +static void batadv_tt_req_node_put(struct batadv_tt_req_node *tt_req_node) +{ + kref_put(&tt_req_node->refcount, batadv_tt_req_node_release); +} + static void batadv_tt_req_list_free(struct batadv_priv *bat_priv) { struct batadv_tt_req_node *node, *safe; @@ -2173,7 +2196,7 @@ static void batadv_tt_req_list_free(stru list_for_each_entry_safe(node, safe, &bat_priv->tt.req_list, list) { list_del(&node->list); - kfree(node); + batadv_tt_req_node_put(node); } spin_unlock_bh(&bat_priv->tt.req_list_lock); @@ -2209,7 +2232,7 @@ static void batadv_tt_req_purge(struct b if (batadv_has_timed_out(node->issued_at, BATADV_TT_REQUEST_TIMEOUT)) { list_del(&node->list); - kfree(node); + batadv_tt_req_node_put(node); } } spin_unlock_bh(&bat_priv->tt.req_list_lock); @@ -2236,9 +2259,11 @@ batadv_new_tt_req_node(struct batadv_pri if (!tt_req_node) goto unlock; + kref_init(&tt_req_node->refcount); ether_addr_copy(tt_req_node->addr, orig_node->orig); tt_req_node->issued_at = jiffies; + kref_get(&tt_req_node->refcount); list_add(&tt_req_node->list, &bat_priv->tt.req_list); unlock: spin_unlock_bh(&bat_priv->tt.req_list_lock); @@ -2488,12 +2513,19 @@ static int batadv_send_tt_request(struct out: if (primary_if) batadv_hardif_free_ref(primary_if); + if (ret && tt_req_node) { spin_lock_bh(&bat_priv->tt.req_list_lock); - list_del(&tt_req_node->list); + if (!list_empty(&tt_req_node->list)) { + list_del(&tt_req_node->list); + batadv_tt_req_node_put(tt_req_node); + } spin_unlock_bh(&bat_priv->tt.req_list_lock); - kfree(tt_req_node); } + + if (tt_req_node) + batadv_tt_req_node_put(tt_req_node); + kfree(tvlv_tt_data); return ret; } @@ -2929,7 +2961,7 @@ static void batadv_handle_tt_response(st if (!batadv_compare_eth(node->addr, resp_src)) continue; list_del(&node->list); - kfree(node); + batadv_tt_req_node_put(node); } spin_unlock_bh(&bat_priv->tt.req_list_lock); --- a/net/batman-adv/types.h +++ b/net/batman-adv/types.h @@ -988,11 +988,13 @@ struct batadv_tt_change_node { * struct batadv_tt_req_node - data to keep track of the tt requests in flight * @addr: mac address address of the originator this request was sent to * @issued_at: timestamp used for purging stale tt requests + * @refcount: number of contexts the object is used by * @list: list node for batadv_priv_tt::req_list */ struct batadv_tt_req_node { uint8_t addr[ETH_ALEN]; unsigned long issued_at; + struct kref refcount; struct list_head list; }; -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html