Bug #244
Updated by Sven Eckelmann over 4 years ago
"Andrew detected one instance of a kref_get_unless_zero":https://lists.open-mesh.org/mailman3/hyperkitty/list/b.a.t.m.a.n@lists.open-mesh.org/message/CLLUZNQDZKLLPSRZTTKIB3GDE2OTFTMF/ kref_get_unless_zero":https://lists.open-mesh.org/pipermail/b.a.t.m.a.n/2016-March/014588.html which didn't seem to belong to a rcu_read_lock protected section. I went through the rest of the code and annotated things which look similar. <pre> diff --git a/net/batman-adv/bat_iv_ogm.c b/net/batman-adv/bat_iv_ogm.c index 2c65668..a27fda0 100644 --- a/net/batman-adv/bat_iv_ogm.c +++ b/net/batman-adv/bat_iv_ogm.c @@ -679,9 +679,11 @@ static void batadv_iv_ogm_aggregate_new(const unsigned char *packet_buff, unsigned char *skb_buff; unsigned int skb_size; + /* TODO */ if (!kref_get_unless_zero(&if_incoming->refcount)) return; + /* TODO */ if (!kref_get_unless_zero(&if_outgoing->refcount)) goto out_free_incoming; diff --git a/net/batman-adv/gateway_client.c b/net/batman-adv/gateway_client.c index c59aff5..fafc053 100644 --- a/net/batman-adv/gateway_client.c +++ b/net/batman-adv/gateway_client.c @@ -135,6 +135,7 @@ static void batadv_gw_select(struct batadv_priv *bat_priv, spin_lock_bh(&bat_priv->gw.list_lock); + /* TODO */ if (new_gw_node && !kref_get_unless_zero(&new_gw_node->refcount)) new_gw_node = NULL; @@ -440,6 +441,7 @@ static void batadv_gw_node_add(struct batadv_priv *bat_priv, if (gateway->bandwidth_down == 0) return; + /* TODO */ if (!kref_get_unless_zero(&orig_node->refcount)) return; diff --git a/net/batman-adv/hard-interface.c b/net/batman-adv/hard-interface.c index 3240a67..a4c488d 100644 --- a/net/batman-adv/hard-interface.c +++ b/net/batman-adv/hard-interface.c @@ -236,6 +236,7 @@ static void batadv_primary_if_select(struct batadv_priv *bat_priv, ASSERT_RTNL(); + /* TODO */ if (new_hard_iface && !kref_get_unless_zero(&new_hard_iface->refcount)) new_hard_iface = NULL; @@ -464,6 +465,7 @@ int batadv_hardif_enable_interface(struct batadv_hard_iface *hard_iface, if (hard_iface->if_status != BATADV_IF_NOT_IN_USE) goto out; + /* TODO */ if (!kref_get_unless_zero(&hard_iface->refcount)) goto out; diff --git a/net/batman-adv/main.c b/net/batman-adv/main.c index d64ddb9..89bc5b1 100644 --- a/net/batman-adv/main.c +++ b/net/batman-adv/main.c @@ -736,6 +736,7 @@ static struct batadv_tvlv_container if (tvlv_tmp->tvlv_hdr.version != version) continue; + /* TODO */ if (!kref_get_unless_zero(&tvlv_tmp->refcount)) continue; diff --git a/net/batman-adv/network-coding.c b/net/batman-adv/network-coding.c index 32f9fa1..e6dc208 100644 --- a/net/batman-adv/network-coding.c +++ b/net/batman-adv/network-coding.c @@ -856,6 +856,7 @@ static struct batadv_nc_node if (!nc_node) return NULL; + /* TODO */ if (!kref_get_unless_zero(&orig_neigh_node->refcount)) goto free; diff --git a/net/batman-adv/originator.c b/net/batman-adv/originator.c index e63d6a5..92e6a41 100644 --- a/net/batman-adv/originator.c +++ b/net/batman-adv/originator.c @@ -381,6 +381,7 @@ batadv_orig_ifinfo_new(struct batadv_orig_node *orig_node, if (!orig_ifinfo) goto out; + /* TODO */ if (if_outgoing != BATADV_IF_DEFAULT && !kref_get_unless_zero(&if_outgoing->refcount)) { kfree(orig_ifinfo); @@ -462,6 +463,7 @@ batadv_neigh_ifinfo_new(struct batadv_neigh_node *neigh, if (!neigh_ifinfo) goto out; + /* TODO */ if (if_outgoing && !kref_get_unless_zero(&if_outgoing->refcount)) { kfree(neigh_ifinfo); neigh_ifinfo = NULL; @@ -539,6 +541,7 @@ batadv_hardif_neigh_create(struct batadv_hard_iface *hard_iface, if (hardif_neigh) goto out; + /* TODO */ if (!kref_get_unless_zero(&hard_iface->refcount)) goto out; @@ -650,6 +653,7 @@ batadv_neigh_node_new(struct batadv_orig_node *orig_node, if (!neigh_node) goto out; + /* TODO */ if (!kref_get_unless_zero(&hard_iface->refcount)) { kfree(neigh_node); neigh_node = NULL; diff --git a/net/batman-adv/routing.c b/net/batman-adv/routing.c index 45093c6..39a2e04 100644 --- a/net/batman-adv/routing.c +++ b/net/batman-adv/routing.c @@ -101,6 +101,7 @@ static void _batadv_update_route(struct batadv_priv *bat_priv, batadv_neigh_node_put(curr_router); /* increase refcount of new best neighbor */ + /* TODO */ if (neigh_node && !kref_get_unless_zero(&neigh_node->refcount)) neigh_node = NULL; </pre> "My proposal":https://lists.open-mesh.org/mailman3/hyperkitty/list/b.a.t.m.a.n@lists.open-mesh.org/message/ZJL4NCMJ6KPDAUDZG4MXVGO5ABQYL2MG/ proposal":https://lists.open-mesh.org/pipermail/b.a.t.m.a.n/2016-March/014591.html was to use kref_get when we already have a valid reference and to move the kref_get_unless_zero up to the rcu_read_lock section code (when there is one). This has the benefit that kref_get will warn us when the reference counting broke end we try to get a reference when the reference counter already reached zero (which should not happen in this situation).