Re: [B.A.T.M.A.N.] [PATCH 1/7] batman-adv: Correct rcu refcounting for gw_node

Linus Lüssing
2011-02-10T11:01:39Z

So, had a second look at the way the gw_node rcu-locking
refcounting stuff is done and looks sane to me with the
rcu_dereference/assign_pointer additions. Just one thing I noticed
in gw_select() which probably is not an issue:

It's not harmful to hold a rcu_read_lock() while calling this as
we are using call_rcu() gw_node_free_ref(), right?
Do we need to increase the new_gw_node refcount? Of course we
cannot call the whole gw_select() without
any rcu locking due to the atomic_inc_not_zero(), but just
wondering if the rcu_read_unlock() should be called earlier.

So if some one can confirm that this is not an issue, then the
gw_node rcu refcount patches get my ok for being applied to the
trunk.

Cheers, Linus

On Fri, Feb 04, 2011 at 04:21:30PM +0100, Linus Lüssing wrote:
> From: Sven Eckelmann <sven@narfation.org>
>
> It might be possible that 2 threads access the same data in the same
> rcu grace period. The first thread calls call_rcu() to decrement the
> refcount and free the data while the second thread increases the
> refcount to use the data. To avoid this race condition all refcount
> operations have to be atomic.
>
> Reported-by: Sven Eckelmann <sven@narfation.org>
> Signed-off-by: Marek Lindner <lindner_marek@yahoo.de>
> ---
> gateway_client.c | 41 ++++++++++++++++++-----------------------
> types.h | 2 +-
> 2 files changed, 19 insertions(+), 24 deletions(-)
>
> diff --git a/batman-adv/gateway_client.c b/batman-adv/gateway_client.c
> index 429a013..517e001 100644
> --- a/batman-adv/gateway_client.c
> +++ b/batman-adv/gateway_client.c
> @@ -28,20 +28,18 @@
> #include <linux/udp.h>
> #include <linux/if_vlan.h>
>
> -static void gw_node_free_ref(struct kref *refcount)
> -{
> - struct gw_node *gw_node;
> -
> - gw_node = container_of(refcount, struct gw_node, refcount);
> - kfree(gw_node);
> -}
> -
> static void gw_node_free_rcu(struct rcu_head *rcu)
> {
> struct gw_node *gw_node;
>
> gw_node = container_of(rcu, struct gw_node, rcu);
> - kref_put(&gw_node->refcount, gw_node_free_ref);
> + kfree(gw_node);
> +}
> +
> +static void gw_node_free_ref(struct gw_node *gw_node)
> +{
> + if (atomic_dec_and_test(&gw_node->refcount))
> + call_rcu(&gw_node->rcu, gw_node_free_rcu);
> }
>
> void *gw_get_selected(struct bat_priv *bat_priv)
> @@ -61,25 +59,26 @@ void gw_deselect(struct bat_priv *bat_priv)
> bat_priv->curr_gw = NULL;
>
> if (gw_node)
> - kref_put(&gw_node->refcount, gw_node_free_ref);
> + gw_node_free_ref(gw_node);
> }
>
> -static struct gw_node *gw_select(struct bat_priv *bat_priv,
> - struct gw_node *new_gw_node)
> +static void gw_select(struct bat_priv *bat_priv, struct gw_node *new_gw_node)
> {
> struct gw_node *curr_gw_node = bat_priv->curr_gw;
>
> - if (new_gw_node)
> - kref_get(&new_gw_node->refcount);
> + if (new_gw_node && !atomic_inc_not_zero(&new_gw_node->refcount))
> + new_gw_node = NULL;
>
> bat_priv->curr_gw = new_gw_node;
> - return curr_gw_node;
> +
> + if (curr_gw_node)
> + gw_node_free_ref(curr_gw_node);
> }
>
> void gw_election(struct bat_priv *bat_priv)
> {
> struct hlist_node *node;
> - struct gw_node *gw_node, *curr_gw_tmp = NULL, *old_gw_node = NULL;
> + struct gw_node *gw_node, *curr_gw_tmp = NULL;
> uint8_t max_tq = 0;
> uint32_t max_gw_factor = 0, tmp_gw_factor = 0;
> int down, up;
> @@ -174,14 +173,10 @@ void gw_election(struct bat_priv *bat_priv)
> curr_gw_tmp->orig_node->gw_flags,
> curr_gw_tmp->orig_node->router->tq_avg);
>
> - old_gw_node = gw_select(bat_priv, curr_gw_tmp);
> + gw_select(bat_priv, curr_gw_tmp);
> }
>
> rcu_read_unlock();
> -
> - /* the kfree() has to be outside of the rcu lock */
> - if (old_gw_node)
> - kref_put(&old_gw_node->refcount, gw_node_free_ref);
> }
>
> void gw_check_election(struct bat_priv *bat_priv, struct orig_node *orig_node)
> @@ -242,7 +237,7 @@ static void gw_node_add(struct bat_priv *bat_priv,
> memset(gw_node, 0, sizeof(struct gw_node));
> INIT_HLIST_NODE(&gw_node->list);
> gw_node->orig_node = orig_node;
> - kref_init(&gw_node->refcount);
> + atomic_set(&gw_node->refcount, 1);
>
> spin_lock_bh(&bat_priv->gw_list_lock);
> hlist_add_head_rcu(&gw_node->list, &bat_priv->gw_list);
> @@ -325,7 +320,7 @@ void gw_node_purge(struct bat_priv *bat_priv)
> gw_deselect(bat_priv);
>
> hlist_del_rcu(&gw_node->list);
> - call_rcu(&gw_node->rcu, gw_node_free_rcu);
> + gw_node_free_ref(gw_node);
> }
>
>
> diff --git a/batman-adv/types.h b/batman-adv/types.h
> index e4a0462..ca5f20a 100644
> --- a/batman-adv/types.h
> +++ b/batman-adv/types.h
> @@ -100,7 +100,7 @@ struct gw_node {
> struct hlist_node list;
> struct orig_node *orig_node;
> unsigned long deleted;
> - struct kref refcount;
> + atomic_t refcount;
> struct rcu_head rcu;
> };
>
> --
> 1.7.2.3
>