Message-ID: <c40daf35-7ab0-35cb-9356-17f2e8fc0c75@gmail.com>
Date:   Wed, 24 Apr 2019 04:38:09 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Florian Westphal <fw@...len.de>, vakul.garg@....com
Cc:     steffen.klassert@...unet.com, netdev@...r.kernel.org
Subject: Re: [RFC HACK] xfrm: make state refcounting percpu



On 04/24/2019 03:40 AM, Florian Westphal wrote:
> I'm not sure this is a good idea to begin with, refcount
> is right next to state spinlock which is taken for both tx and rx ops,
> plus this complicates debugging quite a bit.
> 


Why not simply using include/linux/percpu-refcount.h ?

> Also expect UAF or refcount bugs, especially xfrm_states that won't get free'd.
> 
> In case someone wants to experiment with this, I've placed it here:
> https://git.breakpoint.cc/cgit/fw/net-next.git/log/?h=xfrm_pcpu_gc_04
> 
> Summary of old/new locking model:
> 
> Old locking rules:
>  From control plane:
>    - new states get refcount of 1
>    - calls xfrm_state_put() when done
>    - calls xfrm_state_hold() to make sure
>      state won't go away
> 
>  From data plane:
>   - calls xfrm_state_hold_rcu, which will tell when
>     refcount is already 0 (i.e, state is being destroyed)
>   - calls xfrm_state_put when done
> 
>  From gc worker:
>   - steals current gc list head
>   - call synchronize_rcu
>   - can free all entries in the list after this
> 
> Last xfrm_state_put will observe refcount transition to 0,
> this will place xfrm_state on gc list, where its picked
> up (and free'd) by xfrm state gc worker.
> 
> New locking rules:
>  From control plane:
>    - (NEW) must *explicitly* call xfrm_state_delete()
>      when state should be removed
>    - otherwise same as above
>  From data plane:
>    - calls xfrm_state_hold() to get ref,
>      xfrm_state_put when done
>  From gc worker:
>   - steals current gc list head
>   - call synchronize_rcu
>   - entries on list MAY STILL BE IN USE, so
>     * must check refcount for each (expensive, as it
>       summing pcpu refcounts for each state)
>     * needs to place entries that it can't free yet
>       back on gc list and resched itself
> 
> XXX:
> - survives minimal testing (netns + veth + esp tunnel mode)
> - refcount for newly allocated states should probably be
>   changed to 0, and only do 0 -> 1 increment right before
>   state is published (added to state hashes).
> - probably misses several error handling spots that now would
>   have to call xfrm_state_delete() rather than xfrm_state_put().
> 
> Signed-off-by: Florian Westphal <fw@...len.de>
> ---
>  include/net/xfrm.h    | 20 ++--------
>  net/key/af_key.c      |  4 +-
>  net/xfrm/xfrm_state.c | 90 ++++++++++++++++++++++++-------------------
>  net/xfrm/xfrm_user.c  |  2 +-
>  4 files changed, 57 insertions(+), 59 deletions(-)
> 
> diff --git a/include/net/xfrm.h b/include/net/xfrm.h
> index 728d4e7b82c0..f9be94a3a029 100644
> --- a/include/net/xfrm.h
> +++ b/include/net/xfrm.h
> @@ -153,7 +153,7 @@ struct xfrm_state {
>  	struct hlist_node	bysrc;
>  	struct hlist_node	byspi;
>  
> -	refcount_t		refcnt;
> +	int __percpu		*pcpu_refcnt;
>  	spinlock_t		lock;
>  
>  	struct xfrm_id		id;
> @@ -790,26 +790,14 @@ static inline void xfrm_pols_put(struct xfrm_policy **pols, int npols)
>  
>  void __xfrm_state_destroy(struct xfrm_state *, bool);
>  
> -static inline void __xfrm_state_put(struct xfrm_state *x)
> +static inline void xfrm_state_hold(struct xfrm_state *x)
>  {
> -	refcount_dec(&x->refcnt);
> +	this_cpu_inc(*x->pcpu_refcnt);
>  }
>  
>  static inline void xfrm_state_put(struct xfrm_state *x)
>  {
> -	if (refcount_dec_and_test(&x->refcnt))
> -		__xfrm_state_destroy(x, false);
> -}
> -
> -static inline void xfrm_state_put_sync(struct xfrm_state *x)
> -{
> -	if (refcount_dec_and_test(&x->refcnt))
> -		__xfrm_state_destroy(x, true);
> -}
> -
> -static inline void xfrm_state_hold(struct xfrm_state *x)
> -{
> -	refcount_inc(&x->refcnt);
> +	this_cpu_dec(*x->pcpu_refcnt);
>  }
>  
>  static inline bool addr_match(const void *token1, const void *token2,
> diff --git a/net/key/af_key.c b/net/key/af_key.c
> index 5651c29cb5bd..21d4776b5431 100644
> --- a/net/key/af_key.c
> +++ b/net/key/af_key.c
> @@ -1303,7 +1303,7 @@ static struct xfrm_state * pfkey_msg2xfrm_state(struct net *net,
>  
>  out:
>  	x->km.state = XFRM_STATE_DEAD;
> -	xfrm_state_put(x);
> +	xfrm_state_delete(x);
>  	return ERR_PTR(err);
>  }
>  
> @@ -1525,7 +1525,7 @@ static int pfkey_add(struct sock *sk, struct sk_buff *skb, const struct sadb_msg
>  
>  	if (err < 0) {
>  		x->km.state = XFRM_STATE_DEAD;
> -		__xfrm_state_put(x);
> +		xfrm_state_delete(x);
>  		goto out;
>  	}
>  
> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index 8951c09ae9e9..4614df21ff5c 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -31,6 +31,8 @@
>  #define xfrm_state_deref_prot(table, net) \
>  	rcu_dereference_protected((table), lockdep_is_held(&(net)->xfrm.xfrm_state_lock))
>  
> +#define XFRM_GC_WAIT_JIFFIES	2
> +
>  static void xfrm_state_gc_task(struct work_struct *work);
>  
>  /* Each xfrm_state may be linked to two tables:
> @@ -44,14 +46,9 @@ static unsigned int xfrm_state_hashmax __read_mostly = 1 * 1024 * 1024;
>  static __read_mostly seqcount_t xfrm_state_hash_generation = SEQCNT_ZERO(xfrm_state_hash_generation);
>  static struct kmem_cache *xfrm_state_cache __ro_after_init;
>  
> -static DECLARE_WORK(xfrm_state_gc_work, xfrm_state_gc_task);
> +static DECLARE_DELAYED_WORK(xfrm_state_gc_work, xfrm_state_gc_task);
>  static HLIST_HEAD(xfrm_state_gc_list);
>  
> -static inline bool xfrm_state_hold_rcu(struct xfrm_state __rcu *x)
> -{
> -	return refcount_inc_not_zero(&x->refcnt);
> -}
> -
>  static inline unsigned int xfrm_dst_hash(struct net *net,
>  					 const xfrm_address_t *daddr,
>  					 const xfrm_address_t *saddr,
> @@ -472,6 +469,7 @@ static const struct xfrm_mode *xfrm_get_mode(unsigned int encap, int family)
>  
>  void xfrm_state_free(struct xfrm_state *x)
>  {
> +	free_percpu(x->pcpu_refcnt);
>  	kmem_cache_free(xfrm_state_cache, x);
>  }
>  EXPORT_SYMBOL(xfrm_state_free);
> @@ -499,6 +497,15 @@ static void ___xfrm_state_destroy(struct xfrm_state *x)
>  	xfrm_state_free(x);
>  }
>  
> +static int xfrm_state_refcnt_read(const struct xfrm_state *x)
> +{
> +	int i, refcnt = 0;
> +
> +	for_each_possible_cpu(i)
> +		refcnt += *per_cpu_ptr(x->pcpu_refcnt, i);
> +	return refcnt;
> +}
> +
>  static void xfrm_state_gc_task(struct work_struct *work)
>  {
>  	struct xfrm_state *x;
> @@ -511,8 +518,22 @@ static void xfrm_state_gc_task(struct work_struct *work)
>  
>  	synchronize_rcu();
>  
> -	hlist_for_each_entry_safe(x, tmp, &gc_list, gclist)
> +	hlist_for_each_entry_safe(x, tmp, &gc_list, gclist) {
> +		if (xfrm_state_refcnt_read(x))
> +			continue;
> +
> +		hlist_del(&x->gclist);
>  		___xfrm_state_destroy(x);
> +	}
> +
> +	if (!hlist_empty(&gc_list)) {
> +		spin_lock_bh(&xfrm_state_gc_lock);
> +		hlist_for_each_entry_safe(x, tmp, &gc_list, gclist)
> +			hlist_add_head(&x->gclist, &xfrm_state_gc_list);
> +		spin_unlock_bh(&xfrm_state_gc_lock);
> +
> +		schedule_delayed_work(&xfrm_state_gc_work, XFRM_GC_WAIT_JIFFIES);
> +	}
>  }
>  
>  static enum hrtimer_restart xfrm_timer_handler(struct hrtimer *me)
> @@ -611,8 +632,14 @@ struct xfrm_state *xfrm_state_alloc(struct net *net)
>  	x = kmem_cache_alloc(xfrm_state_cache, GFP_ATOMIC | __GFP_ZERO);
>  
>  	if (x) {
> +		x->pcpu_refcnt = alloc_percpu(int);
> +		if (!x->pcpu_refcnt) {
> +			kmem_cache_free(xfrm_state_cache, x);
> +			return NULL;
> +		}
> +
>  		write_pnet(&x->xs_net, net);
> -		refcount_set(&x->refcnt, 1);
> +		xfrm_state_hold(x);
>  		atomic_set(&x->tunnel_users, 0);
>  		INIT_LIST_HEAD(&x->km.all);
>  		INIT_HLIST_NODE(&x->bydst);
> @@ -634,22 +661,6 @@ struct xfrm_state *xfrm_state_alloc(struct net *net)
>  }
>  EXPORT_SYMBOL(xfrm_state_alloc);
>  
> -void __xfrm_state_destroy(struct xfrm_state *x, bool sync)
> -{
> -	WARN_ON(x->km.state != XFRM_STATE_DEAD);
> -
> -	if (sync) {
> -		synchronize_rcu();
> -		___xfrm_state_destroy(x);
> -	} else {
> -		spin_lock_bh(&xfrm_state_gc_lock);
> -		hlist_add_head(&x->gclist, &xfrm_state_gc_list);
> -		spin_unlock_bh(&xfrm_state_gc_lock);
> -		schedule_work(&xfrm_state_gc_work);
> -	}
> -}
> -EXPORT_SYMBOL(__xfrm_state_destroy);
> -
>  int __xfrm_state_delete(struct xfrm_state *x)
>  {
>  	struct net *net = xs_net(x);
> @@ -673,6 +684,12 @@ int __xfrm_state_delete(struct xfrm_state *x)
>  		 * is what we are dropping here.
>  		 */
>  		xfrm_state_put(x);
> +
> +		spin_lock_bh(&xfrm_state_gc_lock);
> +		hlist_add_head(&x->gclist, &xfrm_state_gc_list);
> +		spin_unlock_bh(&xfrm_state_gc_lock);
> +
> +		schedule_delayed_work(&xfrm_state_gc_work, 0);
>  		err = 0;
>  	}
>  
> @@ -771,10 +788,7 @@ int xfrm_state_flush(struct net *net, u8 proto, bool task_valid, bool sync)
>  				err = xfrm_state_delete(x);
>  				xfrm_audit_state_delete(x, err ? 0 : 1,
>  							task_valid);
> -				if (sync)
> -					xfrm_state_put_sync(x);
> -				else
> -					xfrm_state_put(x);
> +				xfrm_state_put(x);
>  				if (!err)
>  					cnt++;
>  
> @@ -937,8 +951,8 @@ static struct xfrm_state *__xfrm_state_lookup(struct net *net, u32 mark,
>  
>  		if ((mark & x->mark.m) != x->mark.v)
>  			continue;
> -		if (!xfrm_state_hold_rcu(x))
> -			continue;
> +
> +		xfrm_state_hold(x);
>  		return x;
>  	}
>  
> @@ -962,8 +976,7 @@ static struct xfrm_state *__xfrm_state_lookup_byaddr(struct net *net, u32 mark,
>  
>  		if ((mark & x->mark.m) != x->mark.v)
>  			continue;
> -		if (!xfrm_state_hold_rcu(x))
> -			continue;
> +		xfrm_state_hold(x);
>  		return x;
>  	}
>  
> @@ -1151,10 +1164,7 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr,
>  	}
>  out:
>  	if (x) {
> -		if (!xfrm_state_hold_rcu(x)) {
> -			*err = -EAGAIN;
> -			x = NULL;
> -		}
> +		xfrm_state_hold(x);
>  	} else {
>  		*err = acquire_in_progress ? -EAGAIN : error;
>  	}
> @@ -1681,7 +1691,7 @@ int xfrm_state_update(struct xfrm_state *x)
>  
>  		err = 0;
>  		x->km.state = XFRM_STATE_DEAD;
> -		__xfrm_state_put(x);
> +		xfrm_state_put(x);
>  	}
>  
>  fail:
> @@ -2372,7 +2382,7 @@ struct xfrm_state_afinfo *xfrm_state_get_afinfo(unsigned int family)
>  
>  void xfrm_flush_gc(void)
>  {
> -	flush_work(&xfrm_state_gc_work);
> +	flush_delayed_work(&xfrm_state_gc_work);
>  }
>  EXPORT_SYMBOL(xfrm_flush_gc);
>  
> @@ -2385,7 +2395,7 @@ void xfrm_state_delete_tunnel(struct xfrm_state *x)
>  		if (atomic_read(&t->tunnel_users) == 2)
>  			xfrm_state_delete(t);
>  		atomic_dec(&t->tunnel_users);
> -		xfrm_state_put_sync(t);
> +		xfrm_state_put(t);
>  		x->tunnel = NULL;
>  	}
>  }
> @@ -2531,7 +2541,7 @@ void xfrm_state_fini(struct net *net)
>  	unsigned int sz;
>  
>  	flush_work(&net->xfrm.state_hash_work);
> -	flush_work(&xfrm_state_gc_work);
> +	flush_delayed_work(&xfrm_state_gc_work);
>  	xfrm_state_flush(net, IPSEC_PROTO_ANY, false, true);
>  
>  	WARN_ON(!list_empty(&net->xfrm.state_all));
> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index a131f9ff979e..4587f1342af0 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -675,7 +675,7 @@ static int xfrm_add_sa(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	if (err < 0) {
>  		x->km.state = XFRM_STATE_DEAD;
>  		xfrm_dev_state_delete(x);
> -		__xfrm_state_put(x);
> +		xfrm_state_delete(x);
>  		goto out;
>  	}
>  
> 

Powered by blists - more mailing lists