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 [off-list ref] wrote:

On 19/01/2026 21:48, Paul Moses wrote:

quoted

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?

quoted

[...]

@@ -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].

quoted

spin_lock(&gact->tcf_lock);

Shouldn't you call rcu_read_lock before this line now?

quoted

+ 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.

quoted

[...]
+ bool clockid_set = false;

I could be missing something, but I don't believe you need this
boolean.

quoted

[...]

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

}

quoted

[...]
- 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...

quoted

[...]
+ 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.

quoted

[...]
+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.

quoted

+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