[PATCH net-next 0/2] net/sched: let hardware only actions drop per CPU stats

[PATCH net-next 0/2] net/sched: let hardware only actions drop per CPU stats

[Alexandre Cassen]

This series lets hardware only actions drop their per CPU stats:

- skbedit, police, mpls and sample honor TCA_ACT_FLAGS_NO_PERCPU_STATS.
- tunnel_key skips the dst cache for skip_sw actions.

With no_percpu set, the first four count like gact and mirred but also
share their race with hardware stats updates when an offloaded action
also runs in software. A separate net patch fixes that race:
"net/sched: act_api: take tcfa_lock for hardware stats updates".

Alexandre Cassen (2):
  net/sched: honor TCA_ACT_FLAGS_NO_PERCPU_STATS in more actions
  net/sched: act_tunnel_key: skip dst cache for skip_sw actions

 net/sched/act_mpls.c       |  8 ++++----
 net/sched/act_police.c     | 10 +++++-----
 net/sched/act_sample.c     |  6 +++---
 net/sched/act_skbedit.c    |  9 +++++----
 net/sched/act_tunnel_key.c | 10 +++++++---
 5 files changed, 24 insertions(+), 19 deletions(-)

-- 
2.43.0

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

[Alexandre Cassen]

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.

With the flag set, these actions update their counters the same way gact
and mirred already do via tcf_action_update_bstats() and the qstats
helpers, which take the action lock and use atomics when the per CPU
counters are absent. Those helpers are correct in the software path. The
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.

For example, a per subscriber policer offloaded to hardware:

  tc filter add dev $DEV ingress protocol ip flower skip_sw \
     dst_ip 10.0.0.1/32 \
     action police index 1 rate 1mbit burst 10k \
     conform-exceed drop/pipe no_percpu

Signed-off-by: Alexandre Cassen <acassen@corp.free.fr>
---
 net/sched/act_mpls.c    |  8 ++++----
 net/sched/act_police.c  | 10 +++++-----
 net/sched/act_sample.c  |  6 +++---
 net/sched/act_skbedit.c |  9 +++++----
 4 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/net/sched/act_mpls.c b/net/sched/act_mpls.c
index aa56521b7179..a870386386d9 100644
--- a/net/sched/act_mpls.c
+++ b/net/sched/act_mpls.c
@@ -60,7 +60,7 @@ TC_INDIRECT_SCOPE int tcf_mpls_act(struct sk_buff *skb,
 	int mac_len;
 
 	tcf_lastuse_update(&m->tcf_tm);
-	bstats_update(this_cpu_ptr(m->common.cpu_bstats), skb);
+	tcf_action_update_bstats(&m->common, skb);
 
 	/* Ensure 'data' points at mac_header prior calling mpls manipulating
 	 * functions.
@@ -123,7 +123,7 @@ TC_INDIRECT_SCOPE int tcf_mpls_act(struct sk_buff *skb,
 	return p->action;
 
 drop:
-	qstats_cpu_drop_inc(m->common.cpu_qstats);
+	tcf_action_inc_drop_qstats(&m->common);
 	return TC_ACT_SHOT;
 }
 
@@ -196,8 +196,8 @@ static int tcf_mpls_init(struct net *net, struct nlattr *nla,
 		return ACT_P_BOUND;
 
 	if (!exists) {
-		ret = tcf_idr_create(tn, index, est, a, &act_mpls_ops, bind,
-				     true, flags);
+		ret = tcf_idr_create_from_flags(tn, index, est, a,
+						&act_mpls_ops, bind, flags);
 		if (ret) {
 			tcf_idr_cleanup(tn, index);
 			return ret;
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index 3f8147f37549..0b0c09e3dfd9 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);
 		if (ret) {
 			tcf_idr_cleanup(tn, index);
 			return ret;
@@ -259,7 +259,7 @@ TC_INDIRECT_SCOPE int tcf_police_act(struct sk_buff *skb,
 	int ret;
 
 	tcf_lastuse_update(&police->tcf_tm);
-	bstats_update(this_cpu_ptr(police->common.cpu_bstats), skb);
+	tcf_action_update_bstats(&police->common, skb);
 
 	p = rcu_dereference_bh(police->params);
 	ret = p->action;
@@ -313,10 +313,10 @@ TC_INDIRECT_SCOPE int tcf_police_act(struct sk_buff *skb,
 	}
 
 inc_overlimits:
-	qstats_cpu_overlimit_inc(police->common.cpu_qstats);
+	tcf_action_inc_overlimit_qstats(&police->common);
 inc_drops:
 	if (ret == TC_ACT_SHOT)
-		qstats_cpu_drop_inc(police->common.cpu_qstats);
+		tcf_action_inc_drop_qstats(&police->common);
 end:
 	return ret;
 }
diff --git a/net/sched/act_sample.c b/net/sched/act_sample.c
index 44319a159b55..3029e93c4dea 100644
--- a/net/sched/act_sample.c
+++ b/net/sched/act_sample.c
@@ -69,8 +69,8 @@ static int tcf_sample_init(struct net *net, struct nlattr *nla,
 		return ACT_P_BOUND;
 
 	if (!exists) {
-		ret = tcf_idr_create(tn, index, est, a,
-				     &act_sample_ops, bind, true, flags);
+		ret = tcf_idr_create_from_flags(tn, index, est, a,
+						&act_sample_ops, bind, flags);
 		if (ret) {
 			tcf_idr_cleanup(tn, index);
 			return ret;
@@ -173,7 +173,7 @@ TC_INDIRECT_SCOPE int tcf_sample_act(struct sk_buff *skb,
 	int retval;
 
 	tcf_lastuse_update(&s->tcf_tm);
-	bstats_update(this_cpu_ptr(s->common.cpu_bstats), skb);
+	tcf_action_update_bstats(&s->common, skb);
 	retval = READ_ONCE(s->tcf_action);
 
 	psample_group = rcu_dereference_bh(s->psample_group);
diff --git a/net/sched/act_skbedit.c b/net/sched/act_skbedit.c
index 26258b353cde..1f958b7d2a60 100644
--- a/net/sched/act_skbedit.c
+++ b/net/sched/act_skbedit.c
@@ -45,7 +45,7 @@ TC_INDIRECT_SCOPE int tcf_skbedit_act(struct sk_buff *skb,
 	struct tcf_skbedit_params *params;
 
 	tcf_lastuse_update(&d->tcf_tm);
-	bstats_update(this_cpu_ptr(d->common.cpu_bstats), skb);
+	tcf_action_update_bstats(&d->common, skb);
 
 	params = rcu_dereference_bh(d->params);
 
@@ -85,7 +85,7 @@ TC_INDIRECT_SCOPE int tcf_skbedit_act(struct sk_buff *skb,
 	return params->action;
 
 err:
-	qstats_cpu_drop_inc(d->common.cpu_qstats);
+	tcf_action_inc_drop_qstats(&d->common);
 	return TC_ACT_SHOT;
 }
 
@@ -221,8 +221,9 @@ static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
 	}
 
 	if (!exists) {
-		ret = tcf_idr_create(tn, index, est, a,
-				     &act_skbedit_ops, bind, true, act_flags);
+		ret = tcf_idr_create_from_flags(tn, index, est, a,
+						&act_skbedit_ops, bind,
+						act_flags);
 		if (ret) {
 			tcf_idr_cleanup(tn, index);
 			return ret;
-- 
2.43.0

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

[netdev-bot+sashiko]

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

[PATCH net-next 2/2] net/sched: act_tunnel_key: skip dst cache for skip_sw actions

[Alexandre Cassen]

A tunnel_key action flagged skip_sw never routes packets and the
dst_cache helpers handle a missing cache. A replace leaves the flags of
an existing action unchanged, check these rather than the request ones.

Signed-off-by: Alexandre Cassen <acassen@corp.free.fr>
---
 net/sched/act_tunnel_key.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index 503834853306..28d9cc080101 100644
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -481,9 +481,13 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
 		}
 
 #ifdef CONFIG_DST_CACHE
-		ret = dst_cache_init(&metadata->u.tun_info.dst_cache, GFP_KERNEL);
-		if (ret)
-			goto release_tun_meta;
+		if (!((exists ? (*a)->tcfa_flags : act_flags) &
+		      TCA_ACT_FLAGS_SKIP_SW)) {
+			ret = dst_cache_init(&metadata->u.tun_info.dst_cache,
+					     GFP_KERNEL);
+			if (ret)
+				goto release_tun_meta;
+		}
 #endif
 
 		if (opts_len) {
-- 
2.43.0

Re: [PATCH net-next 2/2] net/sched: act_tunnel_key: skip dst cache for skip_sw actions

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · 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).

- [Medium] The patch only saves the dst_cache when TCA_ACT_FLAGS_SKIP_SW
  is already in act_flags while tunnel_key_init() runs.

--

net/sched: act_tunnel_key: skip dst cache for skip_sw actions

This commit makes tunnel_key_init() skip dst_cache_init() when the
tunnel_key action is flagged skip_sw, because such an action never routes
packets. On replace it checks the flags of the existing action rather
than the flags in the request.

> diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
> index 5038348533060..28d9cc080101a 100644
> --- a/net/sched/act_tunnel_key.c
> +++ b/net/sched/act_tunnel_key.c
> @@ -481,9 +481,13 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
>  		}
>  
>  #ifdef CONFIG_DST_CACHE
> -		ret = dst_cache_init(&metadata->u.tun_info.dst_cache, GFP_KERNEL);
> -		if (ret)
> -			goto release_tun_meta;
> +		if (!((exists ? (*a)->tcfa_flags : act_flags) &
> +		      TCA_ACT_FLAGS_SKIP_SW)) {
> +			ret = dst_cache_init(&metadata->u.tun_info.dst_cache,
> +					     GFP_KERNEL);
> +			if (ret)
> +				goto release_tun_meta;
> +		}
>  #endif

[Severity: Medium]
Does this cover the usual hardware offload setup? In that setup skip_sw
is set on the filter, not on the action:

  tc filter add ... flower skip_sw ... action tunnel_key set ...

For a new action, exists is false, so act_flags is checked. act_flags
comes from tcf_action_init_1() and holds only the per-action user flags
plus the BIND/REPLACE flags:

net/sched/act_api.c:tcf_action_init_1() {
    ...
	err = a_o->init(net, tb[TCA_ACT_OPTIONS], est, &a, tp,
			userflags.value | flags, extack);
    ...
}

fl_change() passes the filter flags (fl_flags) through
tcf_exts_validate_ex(). tcf_action_init() copies them onto the action
only after ->init() has returned:

net/sched/act_api.c:tcf_action_init() {
    ...
		act = tcf_action_init_1(net, tp, tb[i], est, ops[i - 1],
					&init_res[i - 1], flags, extack);
    ...
				if (skip_sw)
					act->tcfa_flags |= TCA_ACT_FLAGS_SKIP_SW;
    ...
}

By that point tunnel_key_init() has already called dst_cache_init().
tcf_action_exec() always skips the resulting action, but the action
still holds a per-CPU dst_cache.

This matches the baseline behaviour, so nothing breaks. Still, the
subject says "skip dst cache for skip_sw actions". Only actions with an
explicit skip_sw in TCA_ACT_FLAGS get the saving: standalone actions, or
in-filter actions that set their own flags.

Could the filter's skip_sw reach ->init()? One way would be to OR
TCA_ACT_FLAGS_SKIP_SW into the flags given to tcf_action_init_1() when
tc_skip_sw(fl_flags) is set. Otherwise, could the commit message mention
this limit?

The commit message also doesn't say why the change is wanted (presumably
to save per-CPU memory). Could that be added?

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

Re: [PATCH net-next 0/2] net/sched: let hardware only actions drop per CPU stats

[Jamal Hadi Salim]

On Fri, Oct 9, 2026 at 1:19 AM Alexandre Cassen <acassen@corp.free.fr> wrote:
>
> This series lets hardware only actions drop their per CPU stats:
>
> - skbedit, police, mpls and sample honor TCA_ACT_FLAGS_NO_PERCPU_STATS.
> - tunnel_key skips the dst cache for skip_sw actions.
>
> With no_percpu set, the first four count like gact and mirred but also
> share their race with hardware stats updates when an offloaded action
> also runs in software. A separate net patch fixes that race:
> "net/sched: act_api: take tcfa_lock for hardware stats updates".
>
> Alexandre Cassen (2):
>   net/sched: honor TCA_ACT_FLAGS_NO_PERCPU_STATS in more actions
>   net/sched: act_tunnel_key: skip dst cache for skip_sw actions
>
>  net/sched/act_mpls.c       |  8 ++++----
>  net/sched/act_police.c     | 10 +++++-----
>  net/sched/act_sample.c     |  6 +++---
>  net/sched/act_skbedit.c    |  9 +++++----
>  net/sched/act_tunnel_key.c | 10 +++++++---
>  5 files changed, 24 insertions(+), 19 deletions(-)

For the series:
Reviewed-by: Jamal Hadi Salim <jhs@mojatatu.com>

cheers,
jamal

> --
> 2.43.0
>