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

[Jérémy Jean]

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