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

[Paul Moses]

Proposed changes from v5...v6:
1. Agreed
   -Fixed in (net/sched: act_gate: keep gate_setup_timer helper name)

2. Agreed
   -Fixed in (net/sched: act_gate: drop redundant clockid pre-validation)

3. I was not able to reproduce it. I tended to keep it since NULL became
   representable in the conversion and it was not an expensive branch.
   -Fixed in (net/sched: act_gate: assume params exist on replace path)

4. use_old_entries is true only when REPLACE does not provide a usable new entry
   list (missing or empty) and we copy the previous entries into p to preserve
   effective behavior. This block is skipped when new entries are provided, so
   old cycletime is not reused in that case. It could be clearer but I didn't
   think it was worth the diff increase.

5. Yes
   -Fixed in (net/sched: act_gate: deduplicate init error cleanup labels)

6. Agreed
   -Fixed in (net/sched: act_gate: align cleanup dereference with act_vlan)

7. Agreed
   -Fixed in (net/sched: act_gate: dump params under rcu read-side lock)

Thanks,
Paul


On Friday, February 6th, 2026 at 4:36 AM, Victor Nogueira <victor@xxxxxxxxxxxx> 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..4a1a10bfe3e62 100644
> > [...]
> > -static void gate_setup_timer(struct tcf_gate *gact, u64 basetime,
> > -                            enum tk_offsets tko, s32 clockid,
> > -                            bool do_init)
> > [...]
> > +static void gate_timer_setup(struct tcf_gate *gact, s32 clockid,
> > +                            enum tk_offsets tko)
> > [...]
> 
> I don't believe you need to change the function name here.
> 
> > [...]
> > @@ -323,20 +370,11 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
> >
> >         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:
> > +               clockid_provided = true;
> > +               if (clockid != CLOCK_REALTIME &&
> > +                   clockid != CLOCK_MONOTONIC &&
> > +                   clockid != CLOCK_BOOTTIME &&
> > +                   clockid != CLOCK_TAI) {
> >                         NL_SET_ERR_MSG(extack, "Invalid 'clockid'");
> >                         return -EINVAL;
> >                 }
> 
> This is better than what you had before, however it still
> is redundant given that you do the switch statement later
> and perform the same validation again. If there's no reason to
> keep this code, you probably can also get rid of "clockid_provided".
> 
> > @@ -366,6 +404,37 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
> > [...]
> > +
> > +       if (ret != ACT_P_CREATED) {
> > +               rcu_read_lock();
> > +               old_p = rcu_dereference(gact->param);
> > +               if (old_p) {
> 
> When do you believe old_p might be NULL here?
> From what I understand, you can't arrive here while
> a delete for the same action instance is happening in parallel.
> Were you able to create such scenario when testing gate?
> 
> > [...]
> > +       if (use_old_entries) {
> > +               err = tcf_gate_copy_entries(p, old_p, extack);
> > +               if (err)
> > +                       goto unlock;
> > +
> > +               if (!tb[TCA_GATE_CYCLE_TIME])
> > +                       cycletime = old_p->tcfg_cycletime;
> 
> Why did you keep this one as in v4?
> You don't want to reuse the old "cycletime" if the user
> specified new entries?
> Not saying you are necessarily wrong.
> Just trying to understand your logic.
> 
> > [...]
> > -chain_put:
> > +unlock:
> >         spin_unlock_bh(&gact->tcf_lock);
> >
> >         if (goto_ch)
> >                 tcf_chain_put_by_act(goto_ch);
> > +       release_entry_list(&p->entries);
> > +       kfree(p);
> 
> The 4 lines above look exactly like what you
> do in err_free. Can't you label them as err_free
> and remove the lines below?
> 
> > [...]
> > +err_free:
> > +       if (goto_ch)
> > +               tcf_chain_put_by_act(goto_ch);
> > +       release_entry_list(&p->entries);
> > +       kfree(p);
> > +       goto release_idr;
> > +}
> > [...]
> >  static void tcf_gate_cleanup(struct tc_action *a)
> > @@ -458,9 +594,10 @@ static void tcf_gate_cleanup(struct tc_action *a)
> >         struct tcf_gate *gact = to_gate(a);
> >         struct tcf_gate_params *p;
> >
> > -       p = &gact->param;
> >         hrtimer_cancel(&gact->hitimer);
> > -       release_entry_list(&p->entries);
> > +       p = rcu_replace_pointer(gact->param, NULL, 1);
> > +       if (p)
> > +               call_rcu(&p->rcu, tcf_gate_params_free_rcu);
> >  }
> 
> Sorry, I think I lacked precision in my last comment.
> I meant that you should've removed the rtnl requirement
> (which you did), but also use rcu_dereference_protected as
> act_vlan does. This relates to my previous comment on "old_p"
> being NULL. I don't believe you need to set this to NULL
> unless you were able to reproduce the scenario I described
> earlier.
> 
> >  static int dumping_entry(struct sk_buff *skb,
> > @@ -512,7 +649,8 @@ static int tcf_gate_dump(struct sk_buff *skb, struct tc_action *a,
> >         spin_lock_bh(&gact->tcf_lock);
> >         opt.action = gact->tcf_action;
> >
> > -       p = &gact->param;
> > +       p = rcu_dereference_protected(gact->param,
> > +                                     lockdep_is_held(&gact->tcf_lock));
> 
> You could've kept the rcu_read_lock approach here.
> One of the main advantages of making the params rcu
> is being able to dump without the tcf_lock.
> 
> cheers,
> Victor
>