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

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

[Alexandre Cassen]

Actions without per CPU stats update their counters under tcfa_lock in
the software path but tcf_action_update_stats() adds hardware stats to
the same counters without it. An offloaded action that also runs in
software can then lose counter updates. On 32-bit the race can also
leave the u64_stats seqcount odd and make its readers spin forever.

Callers disable preemption around this update for the per CPU stats.
PREEMPT_RT forbids taking a spinlock_t with preemption disabled. Pin
the CPU in the per CPU path instead.

Tested for regressions on ConnectX-7 with firmware 28.48.1000.

Fixes: 5e174d5e73df ("net: sched: modify stats helper functions to support regular stats")
Signed-off-by: Alexandre Cassen <acassen@corp.free.fr>
---
 include/net/pkt_cls.h | 2 --
 net/sched/act_api.c   | 6 ++++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
index 5f5cb36439fe..9382405b2bab 100644
--- a/include/net/pkt_cls.h
+++ b/include/net/pkt_cls.h
@@ -316,10 +316,8 @@ tcf_exts_hw_stats_update(const struct tcf_exts *exts,
 				continue;
 		}
 
-		preempt_disable();
 		tcf_action_stats_update(a, stats->bytes, stats->pkts, stats->drops,
 					stats->lastused, true);
-		preempt_enable();
 
 		a->used_hw_stats = stats->used_hw_stats;
 		a->used_hw_stats_valid = stats->used_hw_stats_valid;
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 6e48b4bc2d75..f634c23d6a65 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -307,13 +307,11 @@ int tcf_action_update_hw_stats(struct tc_action *action)
 
 	err = tcf_action_offload_cmd(&fl_act, NULL, NULL, NULL);
 	if (!err) {
-		preempt_disable();
 		tcf_action_stats_update(action, fl_act.stats.bytes,
 					fl_act.stats.pkts,
 					fl_act.stats.drops,
 					fl_act.stats.lastused,
 					true);
-		preempt_enable();
 		action->used_hw_stats = fl_act.stats.used_hw_stats;
 		action->used_hw_stats_valid = true;
 	} else {
@@ -1597,6 +1595,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);
@@ -1604,13 +1603,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;
 	}
 
+	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);
 
-- 
2.43.0

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[Eric Dumazet]

Hi Alexandre

Le ven. 9 oct. 2026 à 07:17, Alexandre Cassen <acassen@corp.free.fr> a écrit :
>
> Actions without per CPU stats update their counters under tcfa_lock in
> the software path but tcf_action_update_stats() adds hardware stats to
> the same counters without it. An offloaded action that also runs in
> software can then lose counter updates. On 32-bit the race can also
> leave the u64_stats seqcount odd and make its readers spin forever.

The last sentence is not accurate.

Nobody reads the seqcount of tcfa_bstats / tcfa_bstats_hw:

- tcf_action_copy_stats() calls gnet_stats_copy_basic() and
  gnet_stats_copy_basic_hw() with running == false.

- The estimator is registered with running == false
  (gen_new_estimator() in tcf_idr_create()).

In both cases gnet_stats_read_basic() / gnet_stats_add_basic() skip
u64_stats_fetch_begin()/u64_stats_fetch_retry() and only rely on
tcfa_lock.

This has been the case since gnet_stats_basic_sync was introduced
in 5.16. Older kernels had no seqcount there at all.

So the seqcount can end up odd on 32-bit, but this is harmless.
What 32-bit readers can see is a torn 64-bit value, on top of the
lost updates.

Please fix the changelog in v2, something like:

  On 32-bit, readers holding tcfa_lock can also observe torn
  64-bit values.

The code change itself looks fine to me.

Reviewed-by: Eric Dumazet <edumazet@google.com>

pw-bot: cr

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

[Jamal Hadi Salim]

Hi Alexandre,

On Fri, Oct 9, 2026 at 1:17 AM Alexandre Cassen <acassen@corp.free.fr> wrote:
>
> Actions without per CPU stats update their counters under tcfa_lock in
> the software path but tcf_action_update_stats() adds hardware stats to
> the same counters without it. An offloaded action that also runs in
> software can then lose counter updates. On 32-bit the race can also
> leave the u64_stats seqcount odd and make its readers spin forever.
>
> Callers disable preemption around this update for the per CPU stats.
> PREEMPT_RT forbids taking a spinlock_t with preemption disabled. Pin
> the CPU in the per CPU path instead.
>
> Tested for regressions on ConnectX-7 with firmware 28.48.1000.
>

See the comments from Eric and the bot.
You must now an explanation for how you found the bug. In your case i
would probably cite it as an observed production issue (even if you
are experimenting before production)
You have to wait at least 24 hours before sending V2. When you do you
can add my reviewed by for the series as well.

cheers,
jamal

> Fixes: 5e174d5e73df ("net: sched: modify stats helper functions to support regular stats")
> Signed-off-by: Alexandre Cassen <acassen@corp.free.fr>
> ---
>  include/net/pkt_cls.h | 2 --
>  net/sched/act_api.c   | 6 ++++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index 5f5cb36439fe..9382405b2bab 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -316,10 +316,8 @@ tcf_exts_hw_stats_update(const struct tcf_exts *exts,
>                                 continue;
>                 }
>
> -               preempt_disable();
>                 tcf_action_stats_update(a, stats->bytes, stats->pkts, stats->drops,
>                                         stats->lastused, true);
> -               preempt_enable();
>
>                 a->used_hw_stats = stats->used_hw_stats;
>                 a->used_hw_stats_valid = stats->used_hw_stats_valid;
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 6e48b4bc2d75..f634c23d6a65 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -307,13 +307,11 @@ int tcf_action_update_hw_stats(struct tc_action *action)
>
>         err = tcf_action_offload_cmd(&fl_act, NULL, NULL, NULL);
>         if (!err) {
> -               preempt_disable();
>                 tcf_action_stats_update(action, fl_act.stats.bytes,
>                                         fl_act.stats.pkts,
>                                         fl_act.stats.drops,
>                                         fl_act.stats.lastused,
>                                         true);
> -               preempt_enable();
>                 action->used_hw_stats = fl_act.stats.used_hw_stats;
>                 action->used_hw_stats_valid = true;
>         } else {
> @@ -1597,6 +1595,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);
> @@ -1604,13 +1603,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;
>         }
>
> +       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);
>
> --
> 2.43.0
>

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

[netdev-bot+sashiko]

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