Re: [PATCH net v6 1/1] net/sched: act_gate: snapshot parameters with RCU on replace

[Victor Nogueira]

On Fri, Feb 13, 2026 at 8:39 AM Paul Moses <p@xxxxxxx> wrote:
>
> The gate action can be replaced while the hrtimer callback or dump path is
> walking the schedule list.
>
> Convert the parameters to an RCU-protected snapshot and swap updates under
> tcf_lock, freeing the previous snapshot via call_rcu(). When REPLACE omits
> the entry list, preserve the existing schedule so the effective state is
> unchanged.
> [...]
> diff --git a/net/sched/act_gate.c b/net/sched/act_gate.c
> index c1f75f2727576..60c80e609ec3d 100644
> --- a/net/sched/act_gate.c
> +++ b/net/sched/act_gate.c
> [...]
> @@ -56,11 +59,10 @@ static void gate_start_timer(struct tcf_gate *gact, ktime_t start)
>  {
>         ktime_t expires;
>
> -       expires = hrtimer_get_expires(&gact->hitimer);
> -       if (expires == 0)
> -               expires = KTIME_MAX;
> -
> -       start = min_t(ktime_t, start, expires);
> +       if (hrtimer_active(&gact->hitimer)) {
> +               expires = hrtimer_get_expires(&gact->hitimer);
> +               start = min_t(ktime_t, start, expires);
> +       }

Is this change really necessary?

> [...]
>  static int parse_gate_list(struct nlattr *list_attr,
>                            struct tcf_gate_params *sched,
>                            struct netlink_ext_ack *extack)
> @@ -261,7 +294,6 @@ static int parse_gate_list(struct nlattr *list_attr,
>         }
>
>         sched->num_entries = i;
> -
>         return i;

Removing this line also seems unnecessary.

> [...]
> +static void gate_setup_timer(struct tcf_gate *gact, s32 clockid,
> +                            enum tk_offsets tko)
> +{
> +       WRITE_ONCE(gact->tk_offset, tko);

Why do you need this WRITE_ONCE?

>  static int tcf_gate_init(struct net *net, struct nlattr *nla,
> [...]
> @@ -366,6 +407,60 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
> [...]
> +       if (ret != ACT_P_CREATED) {
> [...]
> +               if (use_old_entries) {
> +                       err = tcf_gate_copy_entries(p, cur_p, extack);
> +                       if (!err && !tb[TCA_GATE_CYCLE_TIME])

This check for TCA_GATE_CYCLE_TIME seems unnecessary.
If I understand your code correctly, cycletime will be overwritten
further down if TCA_GATE_CYCLE_TIME was specified.

> +                               cycletime = cur_p->tcfg_cycletime;
> [...]
> @@ -434,33 +532,47 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
> [...]
> -chain_put:
> +unlock:
>         spin_unlock_bh(&gact->tcf_lock);
>
> +err_free:
> +       release_entry_list(&p->entries);
> +       kfree(p);
> +release_idr:
>         if (goto_ch)
>                 tcf_chain_put_by_act(goto_ch);
> -release_idr:
> [...]

This looks weird.
You will go to the release_idr label when tcf_action_check_ctrlact fails,
so the "if (goto_ch)" part of the code will be reached in that code path.
I believe it would be better to keep the "chain_put" label and keep
"release_idr" below it (as it was before your change).
Something like:

chain_put:
        if (goto_ch)
                tcf_chain_put_by_act(goto_ch);
release_idr:
        ...

cheers,
Victor