[PATCH net 1/1] net/sched: act_api: use RCU with deferred freeing for action lifecycle

[Jamal Hadi Salim]

When NEWTFILTER and DELFILTER are run concurently it is possible to create a
race with an associated action.

Let's illustrate with CPU0 running NEWTFILTER and CPU1 running DELFILTER:

 0: mutex_lock() <-- holds the idr lock
 0: rcu_read_lock()
 0: p = idr_find(idr, index) <-- action p is valid (RCU protects IDR)
 0: mutex_unlock() <-- releases the idr lock
 1: refcount_dec_and_mutex_lock() <-- refcnt 1->0, mutex held
 1: idr_remove(idr, index) <-- Action removed from IDR
 1: mutex_unlock() <-- mutex released allowing us to delete the action
 1: tcf_action_cleanup(p) <-- Kfrees p immediately, no deferral
 0: refcount_inc_not_zero(&p->tcfa_refcnt) <-- ouch, UAF p points to freed memory

This patch fixes the race condition between NEWTFILTER and DELTFILTER by
adding struct rcu_head to tc_action used in the deferral and introducing a
kfree_cpu() in the delete path to defer the delete.

Let's illustrate the new code path:

 0: rcu_read_lock()
 1: refcount_dec_and_mutex_lock() <-- refcnt 1->0, mutex held
 1: idr_remove(idr, index)
 1: mutex_unlock()
 1: kfree_rcu(p, rcu_head) <-- defer to eventually call tcf_action_cleanup()
 0: p = idr_find(idr, index)
 0: refcount_inc_not_zero(&p->tcfa_refcnt) <-- refcnt 0->1 (object stay alive)
 1: rcu_read_unlock() <-- release so freeing can run after grace period

Suggested-by: Jakub Kicinski <kuba@xxxxxxxxxx>
Reported-by: Kyle Zeng <kylebot@xxxxxxxxxx>
Tested-by: Victor Nogueira <victor@xxxxxxxxxxxx>
Tested-by: Kyle Zeng <kylebot@xxxxxxxxxx>
Signed-off-by: Jamal Hadi Salim <jhs@xxxxxxxxxxxx>
---
 include/net/act_api.h |  1 +
 net/sched/act_api.c   | 14 +++++++++++---
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/include/net/act_api.h b/include/net/act_api.h
index d11b79107930..fd2967ee08f7 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -45,6 +45,7 @@ struct tc_action {
 	struct tc_cookie	__rcu *user_cookie;
 	struct tcf_chain	__rcu *goto_chain;
 	u32			tcfa_flags;
+	struct rcu_head         tcfa_rcu;
 	u8			hw_stats;
 	u8			used_hw_stats;
 	bool			used_hw_stats_valid;
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 332fd9695e54..3148142bb543 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -370,6 +370,14 @@ static void tcf_action_cleanup(struct tc_action *p)
 	free_tcf(p);
 }
 
+/* RCU callback to free action after grace period */
+static void tcf_action_rcu_free(struct rcu_head *rcu)
+{
+	struct tc_action *p = container_of(rcu, struct tc_action, tcfa_rcu);
+
+	tcf_action_cleanup(p);
+}
+
 static int __tcf_action_put(struct tc_action *p, bool bind)
 {
 	struct tcf_idrinfo *idrinfo = p->idrinfo;
@@ -379,8 +387,8 @@ static int __tcf_action_put(struct tc_action *p, bool bind)
 			atomic_dec(&p->tcfa_bindcnt);
 		idr_remove(&idrinfo->action_idr, p->tcfa_index);
 		mutex_unlock(&idrinfo->lock);
+		call_rcu(&p->tcfa_rcu, tcf_action_rcu_free);
 
-		tcf_action_cleanup(p);
 		return 1;
 	}
 
@@ -620,7 +628,7 @@ static int tcf_idr_release_unsafe(struct tc_action *p)
 
 	if (refcount_dec_and_test(&p->tcfa_refcnt)) {
 		idr_remove(&p->idrinfo->action_idr, p->tcfa_index);
-		tcf_action_cleanup(p);
+		call_rcu(&p->tcfa_rcu, tcf_action_rcu_free);
 		return ACT_P_DELETED;
 	}
 
@@ -761,7 +769,7 @@ static int tcf_idr_delete_index(struct tcf_idrinfo *idrinfo, u32 index)
 						p->tcfa_index));
 			mutex_unlock(&idrinfo->lock);
 
-			tcf_action_cleanup(p);
+			call_rcu(&p->tcfa_rcu, tcf_action_rcu_free);
 			module_put(owner);
 			return 0;
 		}
-- 
2.34.1