Re: [B.A.T.M.A.N.] [PATCH 1/9] batman-adv: Check hard_iface refcnt before calling function

[Andrew Lunn <andrew@lunn.ch>]

Hi Sven

Thanks for the patches. I will test them out of Monday and let you
know if it fixes the crash.

     Andrew

On Sat, Mar 05, 2016 at 04:09:16PM +0100, Sven Eckelmann wrote:
> The batadv_hardif_list list is checked in many situations and the items
> in this list are given to specialized functions to modify the routing
> behavior. At the moment each of these called functions has to check
> itself whether the received batadv_hard_iface has a refcount > 0 before
> it can increase the reference counter and use it in other objects.
> 
> This can easily lead to problems because it is not easily visible where
> all callers of a function got the batadv_hard_iface object from and
> whether they already hold a valid reference.
> 
> Checking the reference counter directly before calling a subfunction
> with a pointer from the batadv_hardif_list avoids this problem.
> 
> Signed-off-by: Sven Eckelmann <sven@narfation.org>
> ---
>  net/batman-adv/bat_iv_ogm.c | 11 +++++++++++
>  net/batman-adv/bat_v_ogm.c  | 14 +++++++++++++-
>  net/batman-adv/originator.c |  5 +++++
>  net/batman-adv/send.c       |  6 ++++++
>  4 files changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/net/batman-adv/bat_iv_ogm.c b/net/batman-adv/bat_iv_ogm.c
> index 2c65668..b390ff9 100644
> --- a/net/batman-adv/bat_iv_ogm.c
> +++ b/net/batman-adv/bat_iv_ogm.c
> @@ -985,9 +985,15 @@ static void batadv_iv_ogm_schedule(struct batadv_hard_iface *hard_iface)
>  	list_for_each_entry_rcu(tmp_hard_iface, &batadv_hardif_list, list) {
>  		if (tmp_hard_iface->soft_iface != hard_iface->soft_iface)
>  			continue;
> +
> +		if (!kref_get_unless_zero(&tmp_hard_iface->refcount))
> +			continue;
> +
>  		batadv_iv_ogm_queue_add(bat_priv, *ogm_buff,
>  					*ogm_buff_len, hard_iface,
>  					tmp_hard_iface, 1, send_time);
> +
> +		batadv_hardif_put(tmp_hard_iface);
>  	}
>  	rcu_read_unlock();
>  
> @@ -1767,8 +1773,13 @@ static void batadv_iv_ogm_process(const struct sk_buff *skb, int ogm_offset,
>  		if (hard_iface->soft_iface != bat_priv->soft_iface)
>  			continue;
>  
> +		if (!kref_get_unless_zero(&hard_iface->refcount))
> +			continue;
> +
>  		batadv_iv_ogm_process_per_outif(skb, ogm_offset, orig_node,
>  						if_incoming, hard_iface);
> +
> +		batadv_hardif_put(hard_iface);
>  	}
>  	rcu_read_unlock();
>  
> diff --git a/net/batman-adv/bat_v_ogm.c b/net/batman-adv/bat_v_ogm.c
> index 4155fa5..473ebb9 100644
> --- a/net/batman-adv/bat_v_ogm.c
> +++ b/net/batman-adv/bat_v_ogm.c
> @@ -26,6 +26,7 @@
>  #include <linux/if_ether.h>
>  #include <linux/jiffies.h>
>  #include <linux/kernel.h>
> +#include <linux/kref.h>
>  #include <linux/list.h>
>  #include <linux/netdevice.h>
>  #include <linux/random.h>
> @@ -176,6 +177,9 @@ static void batadv_v_ogm_send(struct work_struct *work)
>  		if (hard_iface->soft_iface != bat_priv->soft_iface)
>  			continue;
>  
> +		if (!kref_get_unless_zero(&hard_iface->refcount))
> +			continue;
> +
>  		batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
>  			   "Sending own OGM2 packet (originator %pM, seqno %u, throughput %u, TTL %d) on interface %s [%pM]\n",
>  			   ogm_packet->orig, ntohl(ogm_packet->seqno),
> @@ -185,10 +189,13 @@ static void batadv_v_ogm_send(struct work_struct *work)
>  
>  		/* this skb gets consumed by batadv_v_ogm_send_to_if() */
>  		skb_tmp = skb_clone(skb, GFP_ATOMIC);
> -		if (!skb_tmp)
> +		if (!skb_tmp) {
> +			batadv_hardif_put(hard_iface);
>  			break;
> +		}
>  
>  		batadv_v_ogm_send_to_if(skb_tmp, hard_iface);
> +		batadv_hardif_put(hard_iface);
>  	}
>  	rcu_read_unlock();
>  
> @@ -704,9 +711,14 @@ static void batadv_v_ogm_process(const struct sk_buff *skb, int ogm_offset,
>  		if (hard_iface->soft_iface != bat_priv->soft_iface)
>  			continue;
>  
> +		if (!kref_get_unless_zero(&hard_iface->refcount))
> +			continue;
> +
>  		batadv_v_ogm_process_per_outif(bat_priv, ethhdr, ogm_packet,
>  					       orig_node, neigh_node,
>  					       if_incoming, hard_iface);
> +
> +		batadv_hardif_put(hard_iface);
>  	}
>  	rcu_read_unlock();
>  out:
> diff --git a/net/batman-adv/originator.c b/net/batman-adv/originator.c
> index e63d6a5..44c508a 100644
> --- a/net/batman-adv/originator.c
> +++ b/net/batman-adv/originator.c
> @@ -1165,6 +1165,9 @@ static bool batadv_purge_orig_node(struct batadv_priv *bat_priv,
>  		if (hard_iface->soft_iface != bat_priv->soft_iface)
>  			continue;
>  
> +		if (!kref_get_unless_zero(&hard_iface->refcount))
> +			continue;
> +
>  		best_neigh_node = batadv_find_best_neighbor(bat_priv,
>  							    orig_node,
>  							    hard_iface);
> @@ -1172,6 +1175,8 @@ static bool batadv_purge_orig_node(struct batadv_priv *bat_priv,
>  				    best_neigh_node);
>  		if (best_neigh_node)
>  			batadv_neigh_node_put(best_neigh_node);
> +
> +		batadv_hardif_put(hard_iface);
>  	}
>  	rcu_read_unlock();
>  
> diff --git a/net/batman-adv/send.c b/net/batman-adv/send.c
> index 3ce06e0..43a950e 100644
> --- a/net/batman-adv/send.c
> +++ b/net/batman-adv/send.c
> @@ -26,6 +26,7 @@
>  #include <linux/if.h>
>  #include <linux/jiffies.h>
>  #include <linux/kernel.h>
> +#include <linux/kref.h>
>  #include <linux/list.h>
>  #include <linux/netdevice.h>
>  #include <linux/printk.h>
> @@ -577,10 +578,15 @@ static void batadv_send_outstanding_bcast_packet(struct work_struct *work)
>  		if (forw_packet->num_packets >= hard_iface->num_bcasts)
>  			continue;
>  
> +		if (!kref_get_unless_zero(&hard_iface->refcount))
> +			continue;
> +
>  		/* send a copy of the saved skb */
>  		skb1 = skb_clone(forw_packet->skb, GFP_ATOMIC);
>  		if (skb1)
>  			batadv_send_broadcast_skb(skb1, hard_iface);
> +
> +		batadv_hardif_put(hard_iface);
>  	}
>  	rcu_read_unlock();
>  
> -- 
> 2.7.0
>