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

[Victor Nogueira]

> 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..917974b68c3fd 100644
> --- a/net/sched/act_gate.c
> +++ b/net/sched/act_gate.c
> [...]
> @@ -375,25 +472,26 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
> [...]
> +       if (need_cancel)
> +               hrtimer_cancel(&gact->hitimer);
> +
> +       spin_lock_bh(&gact->tcf_lock);
>
>         if (!cycletime) {
>                 struct tcfg_gate_entry *entry;
> @@ -404,20 +502,22 @@ static int tcf_gate_init(struct net *net, struct nlattr *nla,
>                 cycletime = cycle;
>                 if (!cycletime) {
>                         err = -EINVAL;
> -                       goto chain_put;
> +                       goto unlock;
>                 }
> [...]

The AI says:

"If this error path is taken after hrtimer_cancel() was called above
(when need_cancel is true), does the timer get restarted with the old
parameters?

Looking at the unlock path:

unlock:
        spin_unlock_bh(&gact->tcf_lock);
err_free:
        release_entry_list(&p->entries);
        kfree(p);
chain_put:
        if (goto_ch)
                tcf_chain_put_by_act(goto_ch);
release_idr:
        if (ret == ACT_P_CREATED)
                gate_setup_timer(gact, timer_clockid, tko);
        tcf_idr_release(*a, bind);
        return err;

The gate_setup_timer() call at release_idr only runs for ACT_P_CREATED,
not for REPLACE operations. When a REPLACE operation changes
basetime/clockid/tk_offset (making need_cancel true), hrtimer_cancel()
is called. If the subsequent cycletime validation fails, this error path
never restarts the timer, leaving the gate action with a permanently
stopped hrtimer.

Should the error path restart the timer with the old parameters when
need_cancel was true?"

What it describes won't happen (at least from what I understand)
because your current code won't allow zero entries and the interval
parameter can't be 0. However it has a point because, if cycletime
can't be 0 anymore, you should remove this error path altogether.

cheers,
Victor