Date:   Wed, 24 Apr 2019 12:40:23 +0200
From:   Florian Westphal <fw@...len.de>
To:     vakul.garg@....com
Cc:     steffen.klassert@...unet.com, <netdev@...r.kernel.org>,
        Florian Westphal <fw@...len.de>
Subject: [RFC HACK] xfrm: make state refcounting percpu

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.

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;
 	}
 
-- 
2.21.0

Powered by blists - more mailing lists