Re: [PATCH net-next 1/2] net/sched: honor TCA_ACT_FLAGS_NO_PERCPU_STATS in more actions

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 (1) · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] With TCA_ACT_FLAGS_NO_PERCPU_STATS set, police, skbedit and
  sample now create actions with a->cpu_bstats == NULL, using…
- [Low] The commit message gets the scope of the race wrong in several
  places: (1) It says only 'gact and mirred carry this same race today'.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but replacing a
  police action's existing rate estimator can deadlock.

--

net/sched: honor TCA_ACT_FLAGS_NO_PERCPU_STATS in more actions

This commit switches skbedit, police, mpls and sample to
tcf_idr_create_from_flags(). With TCA_ACT_FLAGS_NO_PERCPU_STATS set, these
actions no longer allocate per-CPU stats. Their datapath counter updates
are converted to tcf_action_update_bstats() and the qstats helpers, which
fall back to tcfa_lock and atomics when the per-CPU counters are absent.

> skbedit, police, mpls and sample store and dump
> TCA_ACT_FLAGS_NO_PERCPU_STATS since commit 40bd094d65fc ("flow_offload:
> fill flags to action structure"), but still allocate per CPU stats.

[ ... ]

> problem is the hardware stats path used when an action is offloaded but
> skip_sw is not set: tcf_action_update_stats() writes the same counters
> without holding tcfa_lock, so the two paths can race on tcfa_bstats.
> gact and mirred carry this same race today, these actions only come in
> line with them. I will send a separate patch to fix it.

[Severity: Low]
I have a few questions about the scope described here.

ct, gate, pedit and vlan also create actions with
tcf_idr_create_from_flags(), and their .stats_update callbacks call
tcf_action_update_stats(). Don't they have the same race as gact and
mirred?

act_mpls_ops has no .stats_update callback, so mpls never reaches
tcf_action_update_stats(). Should mpls be left out of the list of actions
that come "in line with" the racy ones?

The race also seems reachable for software-only filters, and for skip_sw
filters through concurrent filter gets, not only for offloaded actions
without skip_sw. The comment on tcf_police_init() below has the details.

"I will send a separate patch to fix it." reads like a cover-letter
remark. Should it go below the --- line so it doesn't end up in the git
history?

[ ... ]

> diff --git a/net/sched/act_police.c b/net/sched/act_police.c
> index 3f8147f375493..0b0c09e3dfd93 100644
> --- a/net/sched/act_police.c
> +++ b/net/sched/act_police.c
> @@ -80,8 +80,8 @@ static int tcf_police_init(struct net *net, struct nlattr *nla,
>  		return ACT_P_BOUND;
>  
>  	if (!exists) {
> -		ret = tcf_idr_create(tn, index, NULL, a,
> -				     &act_police_ops, bind, true, flags);
> +		ret = tcf_idr_create_from_flags(tn, index, NULL, a,
> +						&act_police_ops, bind, flags);

[Severity: Medium]
With no_percpu set, police, skbedit and sample now have
a->cpu_bstats == NULL. Each of them has a .stats_update callback
(tcf_police_stats_update(), tcf_skbedit_stats_update(),
tcf_sample_stats_update()) that ends up in tcf_action_update_stats().
The non-percpu branch of that function writes the shared counters without
taking tcfa_lock:

net/sched/act_api.c:tcf_action_update_stats() {
    ...
	_bstats_update(&a->tcfa_bstats, bytes, packets);
	atomic_add(drops, &a->tcfa_drops);
	if (hw)
		_bstats_update(&a->tcfa_bstats_hw, bytes, packets);
}

The datapath writer, tcf_action_update_bstats(), holds
spin_lock(&a->tcfa_lock). The hardware stats callers,
tcf_exts_hw_stats_update() and tcf_action_update_hw_stats(), only
disable preemption.

Since u64_stats_sync needs writers to be mutually exclusive, can this
lose byte/packet updates to tcfa_bstats, or tear them on 32-bit?

The race doesn't seem to need hardware offload. fl_dump() calls
fl_hw_update_stats() whenever skip_hw is clear, and mall_dump() does the
same. For a filter that isn't offloaded, tcf_exts_hw_stats_update() still
calls this for every action, with zero stats:

	preempt_disable();
	tcf_action_stats_update(a, stats->bytes, stats->pkts, stats->drops,
				stats->lastused, true);
	preempt_enable();

So a default software-only flower filter with a no_percpu police action
would do an unlocked read-modify-write of tcfa_bstats on every dump,
while packets on other CPUs update the same counter under tcfa_lock.

The skip_sw case from the commit message example is also exposed.
RTM_GETTFILTER is RTNL_FLAG_DOIT_UNLOCKED, and fl_hw_update_stats() runs
after tp->lock is dropped, so two concurrent filter gets can both write
tcfa_bstats.

Before this patch, each writer updated its own this_cpu_ptr() slot.
Should the non-percpu branch of tcf_action_update_stats() take tcfa_lock
before no_percpu is honored for these actions?

>  		if (ret) {
>  			tcf_idr_cleanup(tn, index);
>  			return ret;

[Severity: High]
This isn't a bug introduced by this patch, but can replacing the
estimator on an existing police action deadlock?

Further down, tcf_police_init() passes the action lock as the estimator
stats lock. It is the only gen_replace_estimator() caller that passes a
non-NULL lock:

	err = gen_replace_estimator(&police->tcf_bstats,
				    police->common.cpu_bstats,
				    &police->tcf_rate_est,
				    &police->tcf_lock,
				    false, est);

gen_new_estimator() then waits for the old timer while it holds that
lock:

net/core/gen_estimator.c:gen_new_estimator() {
    ...
	if (lock)
		spin_lock_bh(lock);
	old = rcu_dereference_protected(*rate_est, 1);
	if (old) {
		timer_delete_sync(&old->timer);
    ...
}

The old estimator's stats_lock is also &police->tcf_lock, and its timer
callback takes it:

est_timer()
  est_fetch_counters()
    spin_lock(e->stats_lock);

This sequence looks possible:

CPU A                                  CPU B
                                       est_timer() starts (old estimator)
tcf_police_init()
  gen_new_estimator()
    spin_lock_bh(&police->tcf_lock)
                                       est_fetch_counters()
                                         spin_lock(&police->tcf_lock)
                                         spins
    timer_delete_sync(&old->timer)
      __timer_delete_sync() loops
      waiting for est_timer()

Neither CPU can make progress. To trigger it, run "tc actions replace
action police ... estimator ... index N" on a police action that already
has an estimator.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009051930.3619697-1-acassen%40corp.free.fr