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

[Victor Nogueira]

> 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