[1/7] batman-adv: Correct rcu refcounting for gw_node

  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(-)
  

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
>