Re: [PATCH net] net/sched: act_api: take tcfa_lock for hardware stats updates

[netdev-bot+sashiko@kernel.org]

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

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

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).

- [High] The per-CPU branch of tcf_action_update_stats()
  (net/sched/act_api.c) is still racy after this patch.

--

net/sched: act_api: take tcfa_lock for hardware stats updates

This commit takes tcfa_lock with spin_lock_bh() around hardware stats
updates in tcf_action_update_stats() for actions without per-CPU stats.
It also moves the preempt_disable()/preempt_enable() pair from the two
callers into the per-CPU branch of tcf_action_update_stats(), so that
PREEMPT_RT does not take a spinlock_t with preemption disabled.

> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index e45a63be397ce..f6f8cd0a5af64 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c

[ ... ]

> @@ -1588,6 +1586,7 @@ void tcf_action_update_stats(struct tc_action *a, u64 bytes, u64 packets,
>  			     u64 drops, bool hw)
>  {
>  	if (a->cpu_bstats) {
> +		preempt_disable();
>  		_bstats_update(this_cpu_ptr(a->cpu_bstats), bytes, packets);
>  
>  		this_cpu_add(a->cpu_qstats->drops, drops);
> @@ -1595,13 +1594,16 @@ void tcf_action_update_stats(struct tc_action *a, u64 bytes, u64 packets,
>  		if (hw)
>  			_bstats_update(this_cpu_ptr(a->cpu_bstats_hw),
>  				       bytes, packets);
> +		preempt_enable();
>  		return;
>  	}

[Severity: High]
The commit message says pinning the CPU is enough for the per-CPU path. Is
it, given that BH stays enabled here?

This per-CPU branch is the default, used unless
TCA_ACT_FLAGS_NO_PERCPU_STATS is set. The hardware stats writer reaches it
from process context:

tcf_dump_walker()  (holds mutex idrinfo->lock)
  tcf_action_update_hw_stats()
    tcf_action_stats_update()
      tcf_gact_stats_update()
        tcf_action_update_stats()

fl_hw_update_stats() / mall_stats_hw_filter()
  tcf_exts_hw_stats_update()
    ...
      tcf_action_update_stats()

A softirq on the same CPU can run while this writer is between
u64_stats_update_begin() and u64_stats_update_end() in _bstats_update().
That softirq can write the same per-CPU syncp from the software datapath:

tcf_gact_act()
  tcf_action_update_bstats()
    bstats_update(this_cpu_ptr(a->cpu_bstats), skb)

It can also read it from the rate estimator:

est_timer()
  est_fetch_counters()
    gnet_stats_add_basic()
      gnet_stats_add_basic_cpu()
        u64_stats_fetch_begin(&bcpu->syncp)

On 32-bit, u64_stats_add() is a plain p->v += val, and
__u64_stats_update_begin() only does preempt_disable_nested() plus
write_seqcount_begin(). Neither of those keeps BH out.

Could a softirq bstats_update() that lands inside the interrupted
read-modify-write then be overwritten, losing updates?

And if est_timer fires on this CPU while the writer holds the seqcount
odd, would read_seqcount_begin() spin forever? The interrupted writer
can't resume until the softirq returns. The estimator takes tcfa_lock
through e->stats_lock in est_fetch_counters(), but the per-CPU writer
never takes that lock, so it doesn't help here.

These look like the same symptoms the commit message describes ("lose
counter updates", "seqcount odd and make its readers spin forever"). They
seem to be fixed only for the non-per-CPU branch.

The race was already present through the callers' preempt_disable()
before this patch. Since this hunk rewrites that block, would
local_bh_disable()/local_bh_enable() work instead of
preempt_disable()/preempt_enable() here? That would also keep the task
on the current CPU and is safe on PREEMPT_RT.

>  
> +	spin_lock_bh(&a->tcfa_lock);
>  	_bstats_update(&a->tcfa_bstats, bytes, packets);
>  	atomic_add(drops, &a->tcfa_drops);
>  	if (hw)
>  		_bstats_update(&a->tcfa_bstats_hw, bytes, packets);
> +	spin_unlock_bh(&a->tcfa_lock);
>  }
>  EXPORT_SYMBOL(tcf_action_update_stats);

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