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 Reported-by: Kyle Zeng Tested-by: Victor Nogueira Tested-by: Kyle Zeng Signed-off-by: Jamal Hadi Salim --- 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