[PATCH net] net/sched: act_api: use mutex in tcf_idr_check_alloc

[PATCH net] net/sched: act_api: use mutex in tcf_idr_check_alloc

[Kyle Zeng]

Currently, the NEWTFILTER path uses RCU to guard action idr accesses while
the DELTFILTER path uses mutex to guard action accesses. This
inconsistency leads to a race condition scenario, which can lead to
erroneous operations on refcount, eventually leading to use-after-free
situation.
In this patch, we revert the introduction of RCU back to mutex in the
NEWFILTER path, which is consistent with the DELFILTER path, avoiding
the race condition.

Fixes: 4b55e86736d5 ("net/sched: act_api: rely on rcu in tcf_idr_check_alloc")
Signed-off-by: Kyle Zeng <kylebot@openai.com>
---
 net/sched/act_api.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 332fd9695e54..63841c3e4310 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -874,36 +874,34 @@ int tcf_idr_check_alloc(struct tc_action_net *tn, u32 *index,
 	u32 max;
 
 	if (*index) {
-		rcu_read_lock();
+		mutex_lock(&idrinfo->lock);
 		p = idr_find(&idrinfo->action_idr, *index);
 
 		if (IS_ERR(p)) {
 			/* This means that another process allocated
 			 * index but did not assign the pointer yet.
 			 */
-			rcu_read_unlock();
+			mutex_unlock(&idrinfo->lock);
 			return -EAGAIN;
 		}
 
 		if (!p) {
 			/* Empty slot, try to allocate it */
 			max = *index;
-			rcu_read_unlock();
+			mutex_unlock(&idrinfo->lock);
 			goto new;
 		}
 
 		if (!refcount_inc_not_zero(&p->tcfa_refcnt)) {
 			/* Action was deleted in parallel */
-			rcu_read_unlock();
+			mutex_unlock(&idrinfo->lock);
 			return -EAGAIN;
 		}
 
 		if (bind)
 			atomic_inc(&p->tcfa_bindcnt);
 		*a = p;
-
-		rcu_read_unlock();
-
+		mutex_unlock(&idrinfo->lock);
 		return 1;
 	} else {
 		/* Find a slot */
-- 
2.54.0

Re: [PATCH net] net/sched: act_api: use mutex in tcf_idr_check_alloc

[Jamal Hadi Salim]

On Tue, May 26, 2026 at 6:09 PM Kyle Zeng <kylebot@openai.com> wrote:
>
> Currently, the NEWTFILTER path uses RCU to guard action idr accesses while
> the DELTFILTER path uses mutex to guard action accesses. This
> inconsistency leads to a race condition scenario, which can lead to
> erroneous operations on refcount, eventually leading to use-after-free
> situation.
> In this patch, we revert the introduction of RCU back to mutex in the
> NEWFILTER path, which is consistent with the DELFILTER path, avoiding
> the race condition.
>
> Fixes: 4b55e86736d5 ("net/sched: act_api: rely on rcu in tcf_idr_check_alloc")
> Signed-off-by: Kyle Zeng <kylebot@openai.com>

Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

> ---
>  net/sched/act_api.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 332fd9695e54..63841c3e4310 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -874,36 +874,34 @@ int tcf_idr_check_alloc(struct tc_action_net *tn, u32 *index,
>         u32 max;
>
>         if (*index) {
> -               rcu_read_lock();
> +               mutex_lock(&idrinfo->lock);
>                 p = idr_find(&idrinfo->action_idr, *index);
>
>                 if (IS_ERR(p)) {
>                         /* This means that another process allocated
>                          * index but did not assign the pointer yet.
>                          */
> -                       rcu_read_unlock();
> +                       mutex_unlock(&idrinfo->lock);
>                         return -EAGAIN;
>                 }
>
>                 if (!p) {
>                         /* Empty slot, try to allocate it */
>                         max = *index;
> -                       rcu_read_unlock();
> +                       mutex_unlock(&idrinfo->lock);
>                         goto new;
>                 }
>
>                 if (!refcount_inc_not_zero(&p->tcfa_refcnt)) {
>                         /* Action was deleted in parallel */
> -                       rcu_read_unlock();
> +                       mutex_unlock(&idrinfo->lock);
>                         return -EAGAIN;
>                 }
>
>                 if (bind)
>                         atomic_inc(&p->tcfa_bindcnt);
>                 *a = p;
> -
> -               rcu_read_unlock();
> -
> +               mutex_unlock(&idrinfo->lock);
>                 return 1;
>         } else {
>                 /* Find a slot */
> --
> 2.54.0
>

Re: [PATCH net] net/sched: act_api: use mutex in tcf_idr_check_alloc

[Pedro Tammela]

On 26/05/2026 19:08, Kyle Zeng wrote:
> Currently, the NEWTFILTER path uses RCU to guard action idr accesses while
> the DELTFILTER path uses mutex to guard action accesses. This
> inconsistency leads to a race condition scenario, which can lead to
> erroneous operations on refcount, eventually leading to use-after-free
> situation.
> In this patch, we revert the introduction of RCU back to mutex in the
> NEWFILTER path, which is consistent with the DELFILTER path, avoiding
> the race condition.
> 
> Fixes: 4b55e86736d5 ("net/sched: act_api: rely on rcu in tcf_idr_check_alloc")
> Signed-off-by: Kyle Zeng <kylebot@openai.com>

Reviewed-by: Pedro Tammela <pctammela@mojatatu.com>

> ---
>   net/sched/act_api.c | 12 +++++-------
>   1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 332fd9695e54..63841c3e4310 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -874,36 +874,34 @@ int tcf_idr_check_alloc(struct tc_action_net *tn, u32 *index,
>   	u32 max;
>   
>   	if (*index) {
> -		rcu_read_lock();
> +		mutex_lock(&idrinfo->lock);
>   		p = idr_find(&idrinfo->action_idr, *index);
>   
>   		if (IS_ERR(p)) {
>   			/* This means that another process allocated
>   			 * index but did not assign the pointer yet.
>   			 */
> -			rcu_read_unlock();
> +			mutex_unlock(&idrinfo->lock);
>   			return -EAGAIN;
>   		}
>   
>   		if (!p) {
>   			/* Empty slot, try to allocate it */
>   			max = *index;
> -			rcu_read_unlock();
> +			mutex_unlock(&idrinfo->lock);
>   			goto new;
>   		}
>   
>   		if (!refcount_inc_not_zero(&p->tcfa_refcnt)) {
>   			/* Action was deleted in parallel */
> -			rcu_read_unlock();
> +			mutex_unlock(&idrinfo->lock);
>   			return -EAGAIN;
>   		}
>   
>   		if (bind)
>   			atomic_inc(&p->tcfa_bindcnt);
>   		*a = p;
> -
> -		rcu_read_unlock();
> -
> +		mutex_unlock(&idrinfo->lock);
>   		return 1;
>   	} else {
>   		/* Find a slot */

Re: [PATCH net] net/sched: act_api: use mutex in tcf_idr_check_alloc

[Victor Nogueira]

On 26/05/2026 19:08, Kyle Zeng wrote:
> Currently, the NEWTFILTER path uses RCU to guard action idr accesses while
> the DELTFILTER path uses mutex to guard action accesses. This
> inconsistency leads to a race condition scenario, which can lead to
> erroneous operations on refcount, eventually leading to use-after-free
> situation.
> In this patch, we revert the introduction of RCU back to mutex in the
> NEWFILTER path, which is consistent with the DELFILTER path, avoiding
> the race condition.
> 
> Fixes: 4b55e86736d5 ("net/sched: act_api: rely on rcu in tcf_idr_check_alloc")
> Signed-off-by: Kyle Zeng <kylebot@openai.com>

Reviewed-by: Victor Nogueira <victor@mojatatu.com>

Re: [PATCH net] net/sched: act_api: use mutex in tcf_idr_check_alloc

[Jakub Kicinski]

On Tue, 26 May 2026 15:08:47 -0700 Kyle Zeng wrote:
> Currently, the NEWTFILTER path uses RCU to guard action idr accesses while
> the DELTFILTER path uses mutex to guard action accesses. This
> inconsistency leads to a race condition scenario, which can lead to
> erroneous operations on refcount, eventually leading to use-after-free
> situation.
> In this patch, we revert the introduction of RCU back to mutex in the
> NEWFILTER path, which is consistent with the DELFILTER path, avoiding
> the race condition.

The commit message is quite inadequate here. Looks like a
run-of-the-mill UAF so you should explain the flow / race that leads 
to it properly.

Doing some extra digging with Jamal off-list we can't find the reason
why normal RCU protection wouldn't work here so maybe hold off reposting
until you hear from Jamal.

Re: [PATCH net] net/sched: act_api: use mutex in tcf_idr_check_alloc

[Jamal Hadi Salim]

[-- Attachment #1: Type: text/plain, Size: 1016 bytes --]

On Thu, May 28, 2026 at 9:13 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Tue, 26 May 2026 15:08:47 -0700 Kyle Zeng wrote:
> > Currently, the NEWTFILTER path uses RCU to guard action idr accesses while
> > the DELTFILTER path uses mutex to guard action accesses. This
> > inconsistency leads to a race condition scenario, which can lead to
> > erroneous operations on refcount, eventually leading to use-after-free
> > situation.
> > In this patch, we revert the introduction of RCU back to mutex in the
> > NEWFILTER path, which is consistent with the DELFILTER path, avoiding
> > the race condition.
>
> The commit message is quite inadequate here. Looks like a
> run-of-the-mill UAF so you should explain the flow / race that leads
> to it properly.
>
> Doing some extra digging with Jamal off-list we can't find the reason
> why normal RCU protection wouldn't work here so maybe hold off reposting
> until you hear from Jamal.

Kyle, can you try the attached patch?

cheers,
jamal

[-- Attachment #2: patchlet-action-rcu --]
[-- Type: application/octet-stream, Size: 1806 bytes --]

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