Message-ID: <4v-I_ZuHhZBLG3lGttZ9HHAT8n_AggP70Rw2IDrj5w6BK_Ol2VoPsR9eP-BKBlLToLNNCElTtbXdTRdD1wsR3QzlCoSaBi6R7SCPn6CDk5c=@1g4.org>
Date: Wed, 21 Jan 2026 01:00:46 +0000
From: Paul Moses <p@....org>
To: Victor Nogueira <victor@...atatu.com>
Cc: netdev@...r.kernel.org, Jamal Hadi Salim <jhs@...atatu.com>, Cong Wang <xiyou.wangcong@...il.com>, Jiri Pirko <jiri@...nulli.us>, "David S . Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH 1/2] net/sched: act_gate: fix schedule updates with RCU swap

> Also, the AI review [2] pointed out a real issue.
> It's easy to reproduce by running something like:
>
> tc action add action gate base-time 200000000000ns \
>   sched-entry close 0ns index 10

This was never allowed. A zero interval has always been invalid for a gate schedule entry. Clang pointed out a no-op branch I added by mistake and the AI review picked it up, but the intent was simply to mirror the existing base-time / cycle-time range checks we already have. Functionally it’s redundant because we were already rejecting this case via the existing validation e.g.:

    if (cycle > (u64)S64_MAX - entry->interval) { ... }

    if (interval == 0) {
        NL_SET_ERR_MSG(extack, "Invalid interval for schedule entry");
        return -EINVAL;
    }

I will prepare and test v3 with your first 8 suggestions and await further input on best practices for avoiding a monolithic patch and on appropriate levels of validation in this specific case.

Thanks
Paul


On Tuesday, January 20th, 2026 at 3:04 PM, Victor Nogueira <victor@...atatu.com> wrote:

> 
> 
> On 19/01/2026 21:48, Paul Moses wrote:
> 
> > Switch act_gate parameters to an RCU-protected pointer and update schedule
> > changes using a prepare-then-swap pattern. This avoids races between the
> > timer/data paths and configuration updates, and cancels the hrtimer
> > before swapping schedules.
> > 
> > A gate action replace could free and swap schedules while the hrtimer
> > callback or data path still dereferences the old entries, leaving a
> > use-after-free window during updates. The deferred swap and RCU free
> > close that window. A reproducer is available on request.
> > 
> > Also clear params on early error for newly created actions to avoid
> > leaving a dangling reference.
> > [...]
> > diff --git a/net/sched/act_gate.c b/net/sched/act_gate.c
> > index c1f75f2727576..3ee07c3deaf97 100644
> > --- a/net/sched/act_gate.c
> > +++ b/net/sched/act_gate.c
> > @@ -6,6 +6,7 @@
> > #include <linux/kernel.h>
> > #include <linux/string.h>
> > #include <linux/errno.h>
> > +#include <linux/limits.h>
> 
> 
> Do you really need to include this?
> 
> > [...]
> > @@ -69,12 +71,14 @@ static enum hrtimer_restart gate_timer_func(struct hrtimer *timer)
> > {
> > struct tcf_gate *gact = container_of(timer, struct tcf_gate,
> > hitimer);
> > - struct tcf_gate_params *p = &gact->param;
> > + struct tcf_gate_params *p;
> 
> 
> When adding/editing local variables, you should adhere to the
> reverse xmas tree style [1].
> 
> > spin_lock(&gact->tcf_lock);
> 
> 
> Shouldn't you call rcu_read_lock before this line now?
> 
> > + p = rcu_dereference_protected(gact->param,
> > + lockdep_is_held(&gact->tcf_lock));
> > [...]
> > static int tcf_gate_init(struct net *net, struct nlattr *nla,
> > @@ -296,20 +296,26 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
> > struct netlink_ext_ack *extack)
> > {
> > struct tc_action_net *tn = net_generic(net, act_gate_ops.net_id);
> > - enum tk_offsets tk_offset = TK_OFFS_TAI;
> > - bool bind = flags & TCA_ACT_FLAGS_BIND;
> > struct nlattr *tb[TCA_GATE_MAX + 1];
> > struct tcf_chain *goto_ch = NULL;
> > - u64 cycletime = 0, basetime = 0;
> > - struct tcf_gate_params *p;
> > - s32 clockid = CLOCK_TAI;
> > + struct tcf_gate_params *p, *oldp;
> > struct tcf_gate *gact;
> > struct tc_gate *parm;
> > - int ret = 0, err;
> > - u32 gflags = 0;
> > - s32 prio = -1;
> > + struct tcf_gate_params newp = { };
> 
> 
> Abide by reverse xmas tree when adding local variables.
> 
> > [...]
> > + bool clockid_set = false;
> 
> 
> I could be missing something, but I don't believe you need this
> boolean.
> 
> > [...]
> > @@ -323,6 +329,7 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
> > 
> > if (tb[TCA_GATE_CLOCKID]) {
> > clockid = nla_get_s32(tb[TCA_GATE_CLOCKID]);
> > + clockid_set = true;
> > switch (clockid) {
> 
> 
> Instead of using clockid_set and repeating the switch statament.
> You could put this if-statement after you already have oldp and do the
> following:
> 
> if (tb[TCA_GATE_CLOCKID]) {
> clockid = nla_get_s32(tb[TCA_GATE_CLOCKID]);
> switch (clockid) {
> case CLOCK_REALTIME:
> tk_offset = TK_OFFS_REAL;
> break;
> case CLOCK_MONOTONIC:
> tk_offset = TK_OFFS_MAX;
> break;
> case CLOCK_BOOTTIME:
> tk_offset = TK_OFFS_BOOT;
> break;
> case CLOCK_TAI:
> tk_offset = TK_OFFS_TAI;
> break;
> default:
> NL_SET_ERR_MSG(extack, "Invalid 'clockid'");
> return -EINVAL;
> }
> } else if (ret != ACT_P_CREATED) {
> clockid = oldp->tcfg_clockid;
> 
> tk_offset = gact->tk_offset;
> 
> }
> 
> > [...]
> > - if (tb[TCA_GATE_CYCLE_TIME])
> > + if (ret == ACT_P_CREATED)
> > + update_timer = true;
> > [...]
> 
> 
> Here you are assigning update_timer to true when the op is a create...
> 
> > [...]
> > + if (update_timer && ret != ACT_P_CREATED)
> > + hrtimer_cancel(&gact->hitimer);
> 
> 
> .. however in the if-statement where it is used you are only allowing
> updates. This looks weird.
> 
> > [...]
> > +free_p:
> > + release_entry_list(&p->entries);
> > + kfree(p);
> 
> 
> The 2 lines of code above are being repeated below and in
> tcf_gate_params_release. You should put them in a common function.
> 
> > +release_new_entries:
> > + release_entry_list(&newp.entries);
> > +put_chain:
> > if (goto_ch)
> > tcf_chain_put_by_act(goto_ch);
> > release_idr:
> > - /* action is not inserted in any list: it's safe to init hitimer
> > - * without taking tcf_lock.
> > - */
> > - if (ret == ACT_P_CREATED)
> > - gate_setup_timer(gact, gact->param.tcfg_basetime,
> > - gact->tk_offset, gact->param.tcfg_clockid,
> > - true);
> > + if (ret == ACT_P_CREATED) {
> > + p = rcu_dereference_protected(gact->param, 1);
> > + if (p) {
> > + release_entry_list(&p->entries);
> > + kfree(p);
> > + rcu_assign_pointer(gact->param, NULL);
> > + }
> > + }
> > tcf_idr_release(*a, bind);
> 
> 
> Also, the AI review [2] pointed out a real issue.
> It's easy to reproduce by running something like:
> 
> tc action add action gate base-time 200000000000ns \
> sched-entry close 0ns index 10
> 
> I think overall you have the right idea - RCU seems like a good fit here.
> The issue is that this patch is confusing because it seems like you are
> trying to fix the bug and perform cleanups at the same time.
> If that is the case, can you try breaking this patch into two? Do one to
> fix the bug (introducing RCU and etc) and another for the cleanups.
> 
> [1]
> https://www.kernel.org/doc/html/v6.3/process/maintainer-netdev.html#local-variable-ordering-reverse-xmas-tree-rcs
> [2]
> https://netdev-ai.bots.linux.dev/ai-review.html?id=cdc17d0d-fd59-41a8-9c8d-1a42699167fd#patch-0
> 
> cheers,
> Victor

Powered by blists - more mailing lists