Re: [PATCH net 1/1] net/sched: cls_route: fix fastmap use-after-free on filter

[Paolo Abeni <pabeni@redhat.com>]

On 7/28/26 2:37 PM, Jamal Hadi Salim wrote:
> On Tue, Jul 28, 2026 at 6:34 AM Paolo Abeni <pabeni@redhat.com> wrote:
>>
>> On 7/23/26 12:52 PM, Jamal Hadi Salim wrote:
>>> diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
>>> index bd6f945bd388..700f878fc3cd 100644
>>> --- a/net/sched/cls_route.c
>>> +++ b/net/sched/cls_route.c
>>> @@ -297,16 +297,29 @@ static void route4_destroy(struct tcf_proto *tp, bool rtnl_held,
>>>                                       next = rtnl_dereference(f->next);
>>>                                       RCU_INIT_POINTER(b->ht[h2], next);
>>>                                       tcf_unbind_filter(tp, &f->res);
>>> -                                     if (tcf_exts_get_net(&f->exts))
>>> -                                             route4_queue_work(f);
>>> -                                     else
>>> -                                             __route4_delete_filter(f);
>>> +                                     /* Always defer the free.  The direct
>>> +                                      * __route4_delete_filter() path has no
>>> +                                      * grace period and races with readers
>>> +                                      * that cached f in the fastmap.
>>> +                                      */
>>> +                                     tcf_exts_get_net(&f->exts);
>>> +                                     route4_queue_work(f);
>>
>> Sashiko fears the above would be race prone:
>>
>> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260723105210.817079-1-jhs%40mojatatu.com
>>
> 
> That was fixed in v2. Is there something i can do next time (message
> patchwork?) to not waste your time looking at an older version?

Usually the tool properly updates the old revision status, but sometimes
it fails do to that.

Double checking that the old revision is marked as superseded in PW
after posting the new one could help.

>>>                               }
>>>                       }
>>>                       RCU_INIT_POINTER(head->table[h1], NULL);
>>>                       kfree_rcu(b, rcu);
>>>               }
>>>       }
>>> +
>>> +     /* All filters are unlinked; no new reader can find them on the
>>> +      * chain.  Wait for in-flight readers that may still hold a filter
>>> +      * pointer and have published it into the fastmap after we unlinked.
>>> +      * Then flush the stale entries while head is still valid under
>>> +      * RTNL, matching the route4_delete() pattern.
>>> +      */
>>> +     synchronize_rcu();
>>> +     route4_reset_fastmap(head);
>>
>> This really looks like quite a big hammer.
> 
> It is.
> I viewed  it as bug fix to original intent of 1109c00547fc - that
> commit even though had the intent of protecting the fast map as as
> well (based on the wording of comment i saw).
> If performance is a big concern: another approach that maybe worth
> considering is to totally remove the fastpath. It maybe more
> performant than imposing the rcus.

Given the amount of effort involved in rtnl lock contention reduction, I
think it would be great if we could avoid the synchronize_rcu(), if it
does not block the fix for an unreasonable amount of time.

>> Since there is already a
>> synchronization point for both reader and write while acquiring the
>> fastmap_lock, I'm wondering if something alike the following could work ?!?
>> (completely untested!!!):
>>
> 
> Your approach has some holes, example misses the most dangerous part -
> destroy(), etc
> I will mull over it and see if it can be fixed to handle all the
> issues. I think it's possible i just need to put time to
> validate/test.
I would appreciate that, thanks!

Paolo