netdev - Re: [PATCH net v4 1/1] net/sched: act_gate: protect parameters with RCU on replace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CA+NMeC9wKrU0PmLKe8k=MRsDk+T6F65Gqz4hpsvHP0=_-qjVLQ@mail.gmail.com>
Date: Thu, 5 Feb 2026 10:24:17 -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 v4 1/1] net/sched: act_gate: protect parameters with
 RCU on replace

> Convert act_gate parameters to an RCU protected snapshot. Allocate a new
> snapshot on CREATE and REPLACE, swap it under tcf_lock, and free the old
> snapshot via call_rcu() to avoid races with the hrtimer callback and the
> dump path.
> [...]
> @@ -323,23 +393,9 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
> [...]
> +               err = gate_clockid_to_offset(clockid, &tk_offset, extack);

I don't believe you really need this function.
You can get the tk_offset once after you've determined which
clockid will be used.

> [...]
> +
> +       if (use_old_entries) {
> +               cur_p = rcu_dereference_protected(gact->param,
> +                                                 lockdep_rtnl_is_held());
>  [...]
> +               if (!tb[TCA_GATE_BASE_TIME])
> +                       basetime = cur_p->tcfg_basetime;

This doesn't look right.
If you have an update that provides a new set of entries
and not basetime, you'll end up updating basetime to
the default variable's value (0). I believe the same is
happening to the other attributes you are looking at
here - prio, gflags, and etc.

>  [...]
> +
> +       if (ret != ACT_P_CREATED) {
> +               cur_p = rcu_dereference_protected(gact->param,
> +                                                 lockdep_rtnl_is_held());

Can you try to acquire cur_p only once and reuse it?
It will look cleaner.

> [...]
>  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, lockdep_rtnl_is_held());

You won't always have the rtnl_lock in this situation.
For example, if a gate action instance is attached to flower on an ingress
qdisc, this might be called without the rtnl_lock.
Take a look at what act_vlan is doing in the cleanup callback.

cheers,
Victor