Re: [PATCH net] net/sched: fix use-after-free in __tcf_action_put()

["Jérémy Jean" <jeremy.jean@oss.cyber.gouv.fr>]

On 2026-10-08 02:35, Jakub Kicinski wrote:
> On Wed, 30 Sep 2026 21:18:40 +0000 Jérémy Jean wrote:
>>  {
>>  	struct tcf_idrinfo *idrinfo = p->idrinfo;
>> 
>> -	if (refcount_dec_and_mutex_lock(&p->tcfa_refcnt, &idrinfo->lock)) {
>> -		if (bind)
>> -			atomic_dec(&p->tcfa_bindcnt);
>> -		idr_remove(&idrinfo->action_idr, p->tcfa_index);
>> +	mutex_lock(&idrinfo->lock);
>> +	if (bind)
>> +		atomic_dec(&p->tcfa_bindcnt);
>> +	if (!refcount_dec_and_test(&p->tcfa_refcnt)) {
>>  		mutex_unlock(&idrinfo->lock);
>> -
>> -		tcf_action_cleanup(p);
>> -		return 1;
>> +		return 0;
>>  	}
>> 
>> -	if (bind)
>> -		atomic_dec(&p->tcfa_bindcnt);
>> +	idr_remove(&idrinfo->action_idr, p->tcfa_index);
>> +	mutex_unlock(&idrinfo->lock);
>> 
>> -	return 0;
>> +	tcf_action_cleanup(p);
>> +	return 1;
>>  }
> 
> Why did you decide to invert the condition?
> If nothing else the diff would be more readable without that

Indeed, you are right: there was no specific reason for the inversion.
Below is a new fix suggestion.
If you agree with it, I could send it as a v2.

Regards,
Jérémy

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index e45a63be397c..9eb1dec14cb1 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -372,9 +372,10 @@ static int __tcf_action_put(struct tc_action *p, 
bool bind)
  {
         struct tcf_idrinfo *idrinfo = p->idrinfo;

-       if (refcount_dec_and_mutex_lock(&p->tcfa_refcnt, 
&idrinfo->lock)) {
-               if (bind)
-                       atomic_dec(&p->tcfa_bindcnt);
+       mutex_lock(&idrinfo->lock);
+       if (bind)
+               atomic_dec(&p->tcfa_bindcnt);
+       if (refcount_dec_and_test(&p->tcfa_refcnt)) {
                 idr_remove(&idrinfo->action_idr, p->tcfa_index);
                 mutex_unlock(&idrinfo->lock);

@@ -382,9 +383,7 @@ static int __tcf_action_put(struct tc_action *p, 
bool bind)
                 return 1;
         }

-       if (bind)
-               atomic_dec(&p->tcfa_bindcnt);
-
+       mutex_unlock(&idrinfo->lock);
         return 0;
  }