Message-ID: <CA+NMeC_v8bQo2tFUYiD1faMJ0Gd9FFbqmPHCvBUD7HW_yoCx0A@mail.gmail.com>
Date: Fri, 6 Feb 2026 07:36:37 -0300
From: Victor Nogueira <victor@...atatu.com>
To: Paul Moses <p@....org>
Cc: 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>, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, 
	stable@...r.kernel.org
Subject: Re: [PATCH net v5 1/1] net/sched: act_gate: snapshot parameters with
 RCU on replace

> 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

Powered by blists - more mailing lists