[PATCH net 0/4] net/sched: Fix remaining actions notification accounting issues

[PATCH net 0/4] net/sched: Fix remaining actions notification accounting issues

[Victor Nogueira]

Commit 8e2efb3f45a5 ("net/sched: add get_fill_size callbacks for actions
missing them") fixed the reported echo/notify skb overrun and noted that
the pre-existing issues Sashiko pointed out [1] would be fixed separately.
This is that series.

Patch 1 makes tcf_action_shared_attrs_size() a real upper bound again.
TCA_ACT_IN_HW_COUNT and TCA_STATS_BASIC_HW are emitted on every action
dump and were never budgeted; TCA_STATS_PKT64 was budgeted once but can be
emitted twice; TCA_ACT_USED_HW_STATS and the rate estimator attributes are
conditional and also unaccounted.

Patch 2 makes the RTM_GETACTION path use the attr_size that tca_action_gd()
already computes and then throws away, so that "tc actions get" can read
back an action whose dump is larger than NLMSG_GOODSIZE.

Patch 3 wraps the reoffload delete notification in
tcf_action_full_attrs_size() like every other notification path, and stops
leaving a skip_sw action installed when that notification cannot be built.

Patch 4 accounts for the block in tcf_mirred_get_fill_size().
This was not in the Sashiko list, but should also be fixed.

A net-next patch adding get_fill_size() to the five actions that still lack
one (act_connmark, act_mpls, act_nat, act_simple, act_skbmod) will be sent
separately.

[1] https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260810164357.1653956-1-victor%40mojatatu.com

Victor Nogueira (4):
  net/sched: act_api: budget all shared attributes in notify skbs
  net/sched: act_api: size the RTM_GETACTION reply from the actions
  net/sched: act_api: fix skb sizing and action leak on reoffload delete
  net/sched: act_mirred: account for TCA_MIRRED_BLOCKID in get_fill_size

 net/sched/act_api.c    | 37 ++++++++++++++++++++++++++-----------
 net/sched/act_mirred.c |  3 ++-
 2 files changed, 28 insertions(+), 12 deletions(-)

-- 
2.55.0

[PATCH net 1/4] net/sched: act_api: budget all shared attributes in notify skbs

[Victor Nogueira]

tcf_action_shared_attrs_size() is supposed to return an upper bound on the
netlink attributes every action dump emits outside of TCA_ACT_OPTIONS, so
that tcf_add_notify_msg(), tcf_del_notify_msg() and friends can allocate
an skb large enough for the reply.  It has fallen behind the dump path and
is now an underestimate for every single action.

Attributes, such as, TCA_ACT_IN_HW_COUNT and TCA_STATS_BASIC_HW are
emitted unconditionally and never accounted for. TCA_STATS_PKT64,
TCA_ACT_USED_HW_STATS, TCA_STATS_RATE_EST, TCA_STATS_RATE_EST64 require
specific conditions, but are also not accounted for.

Fix the issue by budgeting all of them so that we have a legitimate
upper bound. Even tough for of them require specific conditions, they
are cheap so, to avoid overcomplicating, we opted to account for them
unconditionally as well to account for a real worst case scenario.

Fixes: 4e76e75d6aba ("net sched actions: calculate add/delete event message size")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260810164357.1653956-1-victor%40mojatatu.com
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Victor Nogueira <victor@mojatatu.com>
---
 net/sched/act_api.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index b4415d358c91..766162b0b810 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -443,12 +443,21 @@ static size_t tcf_action_shared_attrs_size(const struct tc_action *act)
 		+ nla_total_size(IFNAMSIZ) /* TCA_ACT_KIND */
 		+ cookie_len /* TCA_ACT_COOKIE */
 		+ nla_total_size(sizeof(struct nla_bitfield32)) /* TCA_ACT_HW_STATS */
+		/* TCA_ACT_USED_HW_STATS */
+		+ nla_total_size(sizeof(struct nla_bitfield32))
+		+ nla_total_size(sizeof(u32)) /* TCA_ACT_IN_HW_COUNT */
 		+ nla_total_size(0) /* TCA_ACT_STATS nested */
 		+ nla_total_size(sizeof(struct nla_bitfield32)) /* TCA_ACT_FLAGS */
 		/* TCA_STATS_BASIC */
 		+ nla_total_size_64bit(sizeof(struct gnet_stats_basic))
-		/* TCA_STATS_PKT64 */
-		+ nla_total_size_64bit(sizeof(u64))
+		/* TCA_STATS_BASIC_HW */
+		+ nla_total_size_64bit(sizeof(struct gnet_stats_basic))
+		/* TCA_STATS_PKT64, emitted by both of the basic copies above */
+		+ 2 * nla_total_size_64bit(sizeof(u64))
+		/* TCA_STATS_RATE_EST */
+		+ nla_total_size_64bit(sizeof(struct gnet_stats_rate_est))
+		/* TCA_STATS_RATE_EST64 */
+		+ nla_total_size_64bit(sizeof(struct gnet_stats_rate_est64))
 		/* TCA_STATS_QUEUE */
 		+ nla_total_size_64bit(sizeof(struct gnet_stats_queue))
 		+ nla_total_size(0) /* TCA_ACT_OPTIONS nested */
-- 
2.55.0

Re: [PATCH net 1/4] net/sched: act_api: budget all shared attributes in notify skbs

[Simon Horman]

On Mon, Aug 24, 2026 at 12:39:00PM -0300, Victor Nogueira wrote:
> tcf_action_shared_attrs_size() is supposed to return an upper bound on the
> netlink attributes every action dump emits outside of TCA_ACT_OPTIONS, so
> that tcf_add_notify_msg(), tcf_del_notify_msg() and friends can allocate
> an skb large enough for the reply.  It has fallen behind the dump path and
> is now an underestimate for every single action.
> 
> Attributes, such as, TCA_ACT_IN_HW_COUNT and TCA_STATS_BASIC_HW are
> emitted unconditionally and never accounted for. TCA_STATS_PKT64,
> TCA_ACT_USED_HW_STATS, TCA_STATS_RATE_EST, TCA_STATS_RATE_EST64 require
> specific conditions, but are also not accounted for.
> 
> Fix the issue by budgeting all of them so that we have a legitimate
> upper bound. Even tough for of them require specific conditions, they
> are cheap so, to avoid overcomplicating, we opted to account for them
> unconditionally as well to account for a real worst case scenario.
> 
> Fixes: 4e76e75d6aba ("net sched actions: calculate add/delete event message size")
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260810164357.1653956-1-victor%40mojatatu.com
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Victor Nogueira <victor@mojatatu.com>
> ---
>  net/sched/act_api.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index b4415d358c91..766162b0b810 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -443,12 +443,21 @@ static size_t tcf_action_shared_attrs_size(const struct tc_action *act)
>  		+ nla_total_size(IFNAMSIZ) /* TCA_ACT_KIND */
>  		+ cookie_len /* TCA_ACT_COOKIE */
>  		+ nla_total_size(sizeof(struct nla_bitfield32)) /* TCA_ACT_HW_STATS */
> +		/* TCA_ACT_USED_HW_STATS */
> +		+ nla_total_size(sizeof(struct nla_bitfield32))
> +		+ nla_total_size(sizeof(u32)) /* TCA_ACT_IN_HW_COUNT */
>  		+ nla_total_size(0) /* TCA_ACT_STATS nested */
>  		+ nla_total_size(sizeof(struct nla_bitfield32)) /* TCA_ACT_FLAGS */
>  		/* TCA_STATS_BASIC */
>  		+ nla_total_size_64bit(sizeof(struct gnet_stats_basic))
> -		/* TCA_STATS_PKT64 */
> -		+ nla_total_size_64bit(sizeof(u64))
> +		/* TCA_STATS_BASIC_HW */
> +		+ nla_total_size_64bit(sizeof(struct gnet_stats_basic))
> +		/* TCA_STATS_PKT64, emitted by both of the basic copies above */
> +		+ 2 * nla_total_size_64bit(sizeof(u64))
> +		/* TCA_STATS_RATE_EST */
> +		+ nla_total_size_64bit(sizeof(struct gnet_stats_rate_est))
> +		/* TCA_STATS_RATE_EST64 */
> +		+ nla_total_size_64bit(sizeof(struct gnet_stats_rate_est64))
>  		/* TCA_STATS_QUEUE */
>  		+ nla_total_size_64bit(sizeof(struct gnet_stats_queue))
>  		+ nla_total_size(0) /* TCA_ACT_OPTIONS nested */

Hi Victor,

As you are no doubt aware there is an AI-generated review
of this patchset available at
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824153903.4143642-1-victor%40mojatatu.com

And while I trust you will look over it, I did feel the following issue
was worth raising here for you to comment on.

I do see that it doesn't strictly effect this patch. But at the same time,
perhaps it ought to be fixed, either as part of this patch or as a
follow-up.

Text of the AI-generated review:

  This isn't a bug introduced by this patch, the TCA_ACT_STATS line is
  unchanged, but while the surrounding terms are being made exact, should this
  container use nla_total_size_64bit(0)?

  It is opened through nla_put_64bit():

  net/sched/act_api.c:tcf_action_copy_stats() {
		err = gnet_stats_start_copy(skb, TCA_ACT_STATS,
					    &p->tcfa_lock, &d, TCA_ACT_PAD);
  }

  which reaches gnet_stats_start_copy_compat() -> gnet_stats_copy(d, type,
  NULL, 0, padattr), and on !CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS a 4-byte
  TCA_ACT_PAD attribute can precede the nest.  gen_stats.c even fixes up
  d->tail for exactly that case:

  net/core/gen_stats.c:gnet_stats_start_copy_compat() {
		int ret = gnet_stats_copy(d, type, NULL, 0, padattr);
		/* The initial attribute added in gnet_stats_copy() may be
		 * preceded by a padding attribute, ...
		 */
		if (ret == 0 && d->tail->nla_type == padattr)
  }

  No overrun follows from this today: TCA_ACT_KIND is budgeted
  nla_total_size(IFNAMSIZ) = 20 bytes while the longest registered in-tree
  kind is "tunnel_key", emitted as 16, so every action carries at least 4 bytes
  of slack that absorbs the missing pad.  It is only the term-by-term upper
  bound property that is lost.

Re: [PATCH net 1/4] net/sched: act_api: budget all shared attributes in notify skbs

[Victor Nogueira]

On Thu, Aug 27, 2026 at 5:45 AM Simon Horman <horms@kernel.org> wrote:
>
> On Mon, Aug 24, 2026 at 12:39:00PM -0300, Victor Nogueira wrote:
> > tcf_action_shared_attrs_size() is supposed to return an upper bound on the
> > netlink attributes every action dump emits outside of TCA_ACT_OPTIONS, so
> > that tcf_add_notify_msg(), tcf_del_notify_msg() and friends can allocate
> > an skb large enough for the reply.  It has fallen behind the dump path and
> > is now an underestimate for every single action.
> >
> > Attributes, such as, TCA_ACT_IN_HW_COUNT and TCA_STATS_BASIC_HW are
> > emitted unconditionally and never accounted for. TCA_STATS_PKT64,
> > TCA_ACT_USED_HW_STATS, TCA_STATS_RATE_EST, TCA_STATS_RATE_EST64 require
> > specific conditions, but are also not accounted for.
> >
> > Fix the issue by budgeting all of them so that we have a legitimate
> > upper bound. Even tough for of them require specific conditions, they
> > are cheap so, to avoid overcomplicating, we opted to account for them
> > unconditionally as well to account for a real worst case scenario.
> >
> > Fixes: 4e76e75d6aba ("net sched actions: calculate add/delete event message size")
> > Reported-by: Sashiko <sashiko-bot@kernel.org>
> > Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260810164357.1653956-1-victor%40mojatatu.com
> > Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
> > Signed-off-by: Victor Nogueira <victor@mojatatu.com>
>> [...]
> Hi Victor,
>
> As you are no doubt aware there is an AI-generated review
> of this patchset available at
> https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824153903.4143642-1-victor%40mojatatu.com
>
> And while I trust you will look over it, I did feel the following issue
> was worth raising here for you to comment on.
>
> I do see that it doesn't strictly effect this patch. But at the same time,
> perhaps it ought to be fixed, either as part of this patch or as a
> follow-up.
>
> Text of the AI-generated review:
>
>   This isn't a bug introduced by this patch, the TCA_ACT_STATS line is
>   unchanged, but while the surrounding terms are being made exact, should this
>   container use nla_total_size_64bit(0)?
>
>   It is opened through nla_put_64bit():
>
>   net/sched/act_api.c:tcf_action_copy_stats() {
>                 err = gnet_stats_start_copy(skb, TCA_ACT_STATS,
>                                             &p->tcfa_lock, &d, TCA_ACT_PAD);
>   }
>
>   which reaches gnet_stats_start_copy_compat() -> gnet_stats_copy(d, type,
>   NULL, 0, padattr), and on !CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS a 4-byte
>   TCA_ACT_PAD attribute can precede the nest.  gen_stats.c even fixes up
>   d->tail for exactly that case:
>
>   net/core/gen_stats.c:gnet_stats_start_copy_compat() {
>                 int ret = gnet_stats_copy(d, type, NULL, 0, padattr);
>                 /* The initial attribute added in gnet_stats_copy() may be
>                  * preceded by a padding attribute, ...
>                  */
>                 if (ret == 0 && d->tail->nla_type == padattr)
>   }
>
>   No overrun follows from this today: TCA_ACT_KIND is budgeted
>   nla_total_size(IFNAMSIZ) = 20 bytes while the longest registered in-tree
>   kind is "tunnel_key", emitted as 16, so every action carries at least 4 bytes
>   of slack that absorbs the missing pad.  It is only the term-by-term upper
>   bound property that is lost.

Hi Simon,

Since this doesn't look like it causes any problems today, I think we
should, in the spirit of what Paolo suggested on patch 4, send this as
a net-next patch later.
WDYT?

cheers,
Victor

[PATCH net 2/4] net/sched: act_api: size the RTM_GETACTION reply from the actions

[Victor Nogueira]

tca_action_gd() already walks every requested action and accumulates
attr_size += tcf_action_fill_size(act), then wraps the result in
tcf_action_full_attrs_size().  For RTM_DELACTION that value is handed to
tcf_del_notify_msg(), which allocates max(attr_size, NLMSG_GOODSIZE).  For
RTM_GETACTION it is silently discarded and tcf_get_notify() allocates a
fixed NLMSG_GOODSIZE skb instead.

Any action whose dump exceeds that fixed budget therefore cannot be read
back. For example, act_pedit overruns the budget with 32 actions of four
munge keys each, act_police with 32 policers once the optional
rate/peakrate/result/avrate attributes are present

Fix this by passing attr_size through and allocate the reply the way the
add and delete paths do.

Note on exposure: RTM_GETACTION is the only one of the three action
commands that is not capability checked - tc_ctl_action() requires
CAP_NET_ADMIN for RTM_NEWACTION and RTM_DELACTION only - so this turns a
fixed NLMSG_GOODSIZE reply into a user sized allocation on an
unprivileged path.  It is bounded by TCA_ACT_MAX_PRIO actions per
request, and tca_action_gd() does not reject duplicate indices, so a
single large action can be requested 32 times; an act_bpf program near
BPF_MAXINSNS is about 32KB of dump, or roughly 1MB for one request.
Creating such an action still requires CAP_NET_ADMIN, and the add and
delete paths have sized their skbs this way since the Fixes commit.
Should this ever need bounding, GFP_KERNEL_ACCOUNT would charge the
reply to the caller's memcg.

Fixes: 4e76e75d6aba ("net sched actions: calculate add/delete event message size")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260810164357.1653956-1-victor%40mojatatu.com
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Victor Nogueira <victor@mojatatu.com>
---
 net/sched/act_api.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 766162b0b810..20b6501fd33b 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -1697,12 +1697,12 @@ static int tca_get_fill(struct sk_buff *skb, struct tc_action *actions[],
 
 static int
 tcf_get_notify(struct net *net, u32 portid, struct nlmsghdr *n,
-	       struct tc_action *actions[], int event,
+	       struct tc_action *actions[], size_t attr_size, int event,
 	       struct netlink_ext_ack *extack)
 {
 	struct sk_buff *skb;
 
-	skb = alloc_skb(NLMSG_GOODSIZE, GFP_KERNEL);
+	skb = alloc_skb(max(attr_size, NLMSG_GOODSIZE), GFP_KERNEL);
 	if (!skb)
 		return -ENOBUFS;
 	if (tca_get_fill(skb, actions, portid, n->nlmsg_seq, 0, event,
@@ -2053,7 +2053,8 @@ tca_action_gd(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
 	attr_size = tcf_action_full_attrs_size(attr_size);
 
 	if (event == RTM_GETACTION)
-		ret = tcf_get_notify(net, portid, n, actions, event, extack);
+		ret = tcf_get_notify(net, portid, n, actions, attr_size, event,
+				     extack);
 	else { /* delete */
 		ret = tcf_del_notify(net, n, actions, portid, attr_size, extack);
 		if (ret)
-- 
2.55.0

Re: [PATCH net 2/4] net/sched: act_api: size the RTM_GETACTION reply from the actions

[Simon Horman]

On Mon, Aug 24, 2026 at 12:39:01PM -0300, Victor Nogueira wrote:
> tca_action_gd() already walks every requested action and accumulates
> attr_size += tcf_action_fill_size(act), then wraps the result in
> tcf_action_full_attrs_size().  For RTM_DELACTION that value is handed to
> tcf_del_notify_msg(), which allocates max(attr_size, NLMSG_GOODSIZE).  For
> RTM_GETACTION it is silently discarded and tcf_get_notify() allocates a
> fixed NLMSG_GOODSIZE skb instead.
> 
> Any action whose dump exceeds that fixed budget therefore cannot be read
> back. For example, act_pedit overruns the budget with 32 actions of four
> munge keys each, act_police with 32 policers once the optional
> rate/peakrate/result/avrate attributes are present
> 
> Fix this by passing attr_size through and allocate the reply the way the
> add and delete paths do.
> 
> Note on exposure: RTM_GETACTION is the only one of the three action
> commands that is not capability checked - tc_ctl_action() requires
> CAP_NET_ADMIN for RTM_NEWACTION and RTM_DELACTION only - so this turns a
> fixed NLMSG_GOODSIZE reply into a user sized allocation on an
> unprivileged path.  It is bounded by TCA_ACT_MAX_PRIO actions per
> request, and tca_action_gd() does not reject duplicate indices, so a
> single large action can be requested 32 times; an act_bpf program near
> BPF_MAXINSNS is about 32KB of dump, or roughly 1MB for one request.
> Creating such an action still requires CAP_NET_ADMIN, and the add and
> delete paths have sized their skbs this way since the Fixes commit.
> Should this ever need bounding, GFP_KERNEL_ACCOUNT would charge the
> reply to the caller's memcg.

Hi Victor,

The AI-generated review of this patch [1] makes the case that this
allocation is a concern and ought to be addressed up-front.

While I do see that GFP_KERNEL_ACCOUNT is not used in TC,
and that does give me pause for thought, I am concerned
about the size of the allocation a user can indirectly request
the kernel to make.

[1] https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824153903.4143642-1-victor%40mojatatu.com

Text of AI-generated review:

  Should this allocation use GFP_KERNEL_ACCOUNT rather than plain
  GFP_KERNEL, given that attr_size here is caller influenced on a path with
  no capability check?

  tc_ctl_action() only gates the privileged commands:

  net/sched/act_api.c:tc_ctl_action() {
	if ((n->nlmsg_type != RTM_GETACTION) &&
	    !netlink_capable(skb, CAP_NET_ADMIN))
		return -EPERM;
	...
  }

  so any process that can open an rtnetlink socket in the netns reaches
  tca_action_gd() with RTM_GETACTION, and now controls the reply size
  through the accumulated attr_size instead of getting a fixed
  NLMSG_GOODSIZE skb.

  The commit message says this is "bounded by TCA_ACT_MAX_PRIO actions per
  request, and tca_action_gd() does not reject duplicate indices, so a
  single large action can be requested 32 times".  Is that bound per
  request only?  Nothing appears to limit how many such requests an
  unprivileged caller can have outstanding at once.

  With a classic BPF action near BPF_MAXINSNS, tcf_bpf_get_fill_size()
  budgets nla_total_size(bpf_num_ops * sizeof(struct sock_filter)), roughly
  32KB, and the 32 slots give roughly 1MB.  An alloc_skb() of that size
  exceeds KMALLOC_MAX_CACHE_SIZE, so kmalloc_reserve() falls through to a
  high order GFP_KERNEL page allocation, and the RTM_GETACTION handler is
  registered without RTNL_FLAG_DOIT_UNLOCKED:

  net/sched/act_api.c
	{.msgtype = RTM_GETACTION, .doit = tc_ctl_action,
	 .dumpit = tc_dump_action},

  Can direct reclaim and compaction for that order-8 request run under
  rtnl_lock here?

  On the retention side, netlink_attachskb() admits the first skb on an
  otherwise empty socket regardless of sk_rcvbuf:

  net/netlink/af_netlink.c:netlink_attachskb() {
	if ((rmem == skb->truesize || rmem <= READ_ONCE(sk->sk_rcvbuf)) &&
	    !test_bit(NETLINK_S_CONGESTED, &nlk->state)) {
	...
  }

  so a caller that never reads its socket can keep one oversized reply
  queued per socket.  Since the allocation is not GFP_KERNEL_ACCOUNT, none
  of that is charged to the requester's memcg.

  The commit message already notes "Should this ever need bounding,
  GFP_KERNEL_ACCOUNT would charge the reply to the caller's memcg" - is
  there a reason not to do that in this patch, considering the add and
  delete paths that set the precedent are behind CAP_NET_ADMIN while this
  one is not?

Re: [PATCH net 2/4] net/sched: act_api: size the RTM_GETACTION reply from the actions

[Victor Nogueira]

On Thu, Aug 27, 2026 at 5:49 AM Simon Horman <horms@kernel.org> wrote:
>
> On Mon, Aug 24, 2026 at 12:39:01PM -0300, Victor Nogueira wrote:
> > tca_action_gd() already walks every requested action and accumulates
> > attr_size += tcf_action_fill_size(act), then wraps the result in
> > tcf_action_full_attrs_size().  For RTM_DELACTION that value is handed to
> > tcf_del_notify_msg(), which allocates max(attr_size, NLMSG_GOODSIZE).  For
> > RTM_GETACTION it is silently discarded and tcf_get_notify() allocates a
> > fixed NLMSG_GOODSIZE skb instead.
> >
> > Any action whose dump exceeds that fixed budget therefore cannot be read
> > back. For example, act_pedit overruns the budget with 32 actions of four
> > munge keys each, act_police with 32 policers once the optional
> > rate/peakrate/result/avrate attributes are present
> >
> > Fix this by passing attr_size through and allocate the reply the way the
> > add and delete paths do.
> >
> > Note on exposure: RTM_GETACTION is the only one of the three action
> > commands that is not capability checked - tc_ctl_action() requires
> > CAP_NET_ADMIN for RTM_NEWACTION and RTM_DELACTION only - so this turns a
> > fixed NLMSG_GOODSIZE reply into a user sized allocation on an
> > unprivileged path.  It is bounded by TCA_ACT_MAX_PRIO actions per
> > request, and tca_action_gd() does not reject duplicate indices, so a
> > single large action can be requested 32 times; an act_bpf program near
> > BPF_MAXINSNS is about 32KB of dump, or roughly 1MB for one request.
> > Creating such an action still requires CAP_NET_ADMIN, and the add and
> > delete paths have sized their skbs this way since the Fixes commit.
> > Should this ever need bounding, GFP_KERNEL_ACCOUNT would charge the
> > reply to the caller's memcg.
>
> Hi Victor,
>
> The AI-generated review of this patch [1] makes the case that this
> allocation is a concern and ought to be addressed up-front.
>
> While I do see that GFP_KERNEL_ACCOUNT is not used in TC,
> and that does give me pause for thought, I am concerned
> about the size of the allocation a user can indirectly request
> the kernel to make.
>
> [1] https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824153903.4143642-1-victor%40mojatatu.com
>
> Text of AI-generated review:
>
>   Should this allocation use GFP_KERNEL_ACCOUNT rather than plain
>   GFP_KERNEL, given that attr_size here is caller influenced on a path with
>   no capability check?
>
>   tc_ctl_action() only gates the privileged commands:
>
>   net/sched/act_api.c:tc_ctl_action() {
>         if ((n->nlmsg_type != RTM_GETACTION) &&
>             !netlink_capable(skb, CAP_NET_ADMIN))
>                 return -EPERM;
>         ...
>   }
>
>   so any process that can open an rtnetlink socket in the netns reaches
>   tca_action_gd() with RTM_GETACTION, and now controls the reply size
>   through the accumulated attr_size instead of getting a fixed
>   NLMSG_GOODSIZE skb.
>
>   The commit message says this is "bounded by TCA_ACT_MAX_PRIO actions per
>   request, and tca_action_gd() does not reject duplicate indices, so a
>   single large action can be requested 32 times".  Is that bound per
>   request only?  Nothing appears to limit how many such requests an
>   unprivileged caller can have outstanding at once.
>
>   With a classic BPF action near BPF_MAXINSNS, tcf_bpf_get_fill_size()
>   budgets nla_total_size(bpf_num_ops * sizeof(struct sock_filter)), roughly
>   32KB, and the 32 slots give roughly 1MB.  An alloc_skb() of that size
>   exceeds KMALLOC_MAX_CACHE_SIZE, so kmalloc_reserve() falls through to a
>   high order GFP_KERNEL page allocation, and the RTM_GETACTION handler is
>   registered without RTNL_FLAG_DOIT_UNLOCKED:
>
>   net/sched/act_api.c
>         {.msgtype = RTM_GETACTION, .doit = tc_ctl_action,
>          .dumpit = tc_dump_action},
>
>   Can direct reclaim and compaction for that order-8 request run under
>   rtnl_lock here?
>
>   On the retention side, netlink_attachskb() admits the first skb on an
>   otherwise empty socket regardless of sk_rcvbuf:
>
>   net/netlink/af_netlink.c:netlink_attachskb() {
>         if ((rmem == skb->truesize || rmem <= READ_ONCE(sk->sk_rcvbuf)) &&
>             !test_bit(NETLINK_S_CONGESTED, &nlk->state)) {
>         ...
>   }
>
>   so a caller that never reads its socket can keep one oversized reply
>   queued per socket.  Since the allocation is not GFP_KERNEL_ACCOUNT, none
>   of that is charged to the requester's memcg.
>
>   The commit message already notes "Should this ever need bounding,
>   GFP_KERNEL_ACCOUNT would charge the reply to the caller's memcg" - is
>   there a reason not to do that in this patch, considering the add and
>   delete paths that set the precedent are behind CAP_NET_ADMIN while this
>   one is not?

I tend to agree with Paolo on this.
As of now, those actions will have to be created using CAP_NET_ADMIN
so I don't know whether the exposure is that much higher, but I could
be missing something.
WDYT?

cheers,
Victor

[PATCH net 3/4] net/sched: act_api: fix skb sizing and action leak on reoffload delete

[Victor Nogueira]

tcf_reoffload_del_notify_msg() sizes the RTM_DELACTION skb with
tcf_action_fill_size(action) alone.  Unlike every other notification path
it never wraps that in tcf_action_full_attrs_size(), so the nlmsg_put()
header, struct tcamsg and the TCA_ACT_TAB nest that tca_get_fill() emits -
24 bytes on x86_64 - are not budgeted.  As long as the single action stays
well under NLMSG_GOODSIZE the floor in alloc_skb() hides this, but once its
fill size crosses NLMSG_GOODSIZE the allocation is exactly 24 bytes short
and tca_get_fill() runs out of tailroom.  That is now easy to reach for an
offloadable act_pedit with a large tcfp_nkeys, which commit 8e2efb3f45a5
("net/sched: add get_fill_size callbacks for actions missing them") started
accounting for properly.

When that happens tcf_reoffload_del_notify() returns early, before
tcf_idr_release_unsafe(), and tcf_action_reoffload_cb() discards the return
value:

	if (tc_act_skip_sw(p->tcfa_flags) && !tc_act_in_hw(p))
		tcf_reoffload_del_notify(net, p);

The action has just lost its last hardware instance and is skip_sw, so it
is left installed while processing no packets, and with no notification to
tell userspace about it.  An -ENOBUFS from alloc_skb() gets the same
treatment.

Fix this by budgeting the message header the way the add and delete paths
do, and release the action even when the notification cannot be built -
dropping the notification is strictly better than leaking a dead action,
and there is no caller left to report the error to.

Fixes: 13926d19a11e ("flow_offload: add reoffload process to update hw_count")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260810164357.1653956-1-victor%40mojatatu.com
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Victor Nogueira <victor@mojatatu.com>
---
 net/sched/act_api.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 20b6501fd33b..37eced84dfa5 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -1867,11 +1867,13 @@ static int tcf_action_delete(struct net *net, struct tc_action *actions[])
 static struct sk_buff *tcf_reoffload_del_notify_msg(struct net *net,
 						    struct tc_action *action)
 {
-	size_t attr_size = tcf_action_fill_size(action);
 	struct tc_action *actions[TCA_ACT_MAX_PRIO] = {
 		[0] = action,
 	};
 	struct sk_buff *skb;
+	size_t attr_size;
+
+	attr_size = tcf_action_full_attrs_size(tcf_action_fill_size(action));
 
 	skb = alloc_skb(max(attr_size, NLMSG_GOODSIZE), GFP_KERNEL);
 	if (!skb)
@@ -1888,15 +1890,18 @@ static struct sk_buff *tcf_reoffload_del_notify_msg(struct net *net,
 static int tcf_reoffload_del_notify(struct net *net, struct tc_action *action)
 {
 	const struct tc_action_ops *ops = action->ops;
-	struct sk_buff *skb;
+	struct sk_buff *skb = NULL;
 	int ret;
 
-	if (!rtnl_notify_needed(net, 0, RTNLGRP_TC)) {
-		skb = NULL;
-	} else {
+	if (rtnl_notify_needed(net, 0, RTNLGRP_TC)) {
 		skb = tcf_reoffload_del_notify_msg(net, action);
+		/* The action has already lost its hardware instance and is
+		 * skip_sw, so it must be released whether or not the
+		 * notification can be built.  Drop the notification rather
+		 * than leave an action behind that processes no packets.
+		 */
 		if (IS_ERR(skb))
-			return PTR_ERR(skb);
+			skb = NULL;
 	}
 
 	ret = tcf_idr_release_unsafe(action);
-- 
2.55.0

Re: [PATCH net 3/4] net/sched: act_api: fix skb sizing and action leak on reoffload delete

[Pedro Tammela]

On 24/08/2026 12:39, Victor Nogueira wrote:
> tcf_reoffload_del_notify_msg() sizes the RTM_DELACTION skb with
> tcf_action_fill_size(action) alone.  Unlike every other notification path
> it never wraps that in tcf_action_full_attrs_size(), so the nlmsg_put()
> header, struct tcamsg and the TCA_ACT_TAB nest that tca_get_fill() emits -
> 24 bytes on x86_64 - are not budgeted.  As long as the single action stays
> well under NLMSG_GOODSIZE the floor in alloc_skb() hides this, but once its
> fill size crosses NLMSG_GOODSIZE the allocation is exactly 24 bytes short
> and tca_get_fill() runs out of tailroom.  That is now easy to reach for an
> offloadable act_pedit with a large tcfp_nkeys, which commit 8e2efb3f45a5
> ("net/sched: add get_fill_size callbacks for actions missing them") started
> accounting for properly.
> 
> When that happens tcf_reoffload_del_notify() returns early, before
> tcf_idr_release_unsafe(), and tcf_action_reoffload_cb() discards the return
> value:
> 
> 	if (tc_act_skip_sw(p->tcfa_flags) && !tc_act_in_hw(p))
> 		tcf_reoffload_del_notify(net, p);
> 
> The action has just lost its last hardware instance and is skip_sw, so it
> is left installed while processing no packets, and with no notification to
> tell userspace about it.  An -ENOBUFS from alloc_skb() gets the same
> treatment.
> 
> Fix this by budgeting the message header the way the add and delete paths
> do, and release the action even when the notification cannot be built -
> dropping the notification is strictly better than leaking a dead action,
> and there is no caller left to report the error to.
> 
> Fixes: 13926d19a11e ("flow_offload: add reoffload process to update hw_count")
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260810164357.1653956-1-victor%40mojatatu.com
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>

Reviewed-by: Pedro Tammela <pctammela@mojatatu.com>

> Signed-off-by: Victor Nogueira <victor@mojatatu.com>
> ---
>   net/sched/act_api.c | 17 +++++++++++------
>   1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 20b6501fd33b..37eced84dfa5 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -1867,11 +1867,13 @@ static int tcf_action_delete(struct net *net, struct tc_action *actions[])
>   static struct sk_buff *tcf_reoffload_del_notify_msg(struct net *net,
>   						    struct tc_action *action)
>   {
> -	size_t attr_size = tcf_action_fill_size(action);
>   	struct tc_action *actions[TCA_ACT_MAX_PRIO] = {
>   		[0] = action,
>   	};
>   	struct sk_buff *skb;
> +	size_t attr_size;
> +
> +	attr_size = tcf_action_full_attrs_size(tcf_action_fill_size(action));
>   
>   	skb = alloc_skb(max(attr_size, NLMSG_GOODSIZE), GFP_KERNEL);
>   	if (!skb)
> @@ -1888,15 +1890,18 @@ static struct sk_buff *tcf_reoffload_del_notify_msg(struct net *net,
>   static int tcf_reoffload_del_notify(struct net *net, struct tc_action *action)
>   {
>   	const struct tc_action_ops *ops = action->ops;
> -	struct sk_buff *skb;
> +	struct sk_buff *skb = NULL;
>   	int ret;
>   
> -	if (!rtnl_notify_needed(net, 0, RTNLGRP_TC)) {
> -		skb = NULL;
> -	} else {
> +	if (rtnl_notify_needed(net, 0, RTNLGRP_TC)) {
>   		skb = tcf_reoffload_del_notify_msg(net, action);
> +		/* The action has already lost its hardware instance and is
> +		 * skip_sw, so it must be released whether or not the
> +		 * notification can be built.  Drop the notification rather
> +		 * than leave an action behind that processes no packets.
> +		 */
>   		if (IS_ERR(skb))
> -			return PTR_ERR(skb);
> +			skb = NULL;
>   	}
>   
>   	ret = tcf_idr_release_unsafe(action);

[PATCH net 4/4] net/sched: act_mirred: account for TCA_MIRRED_BLOCKID in get_fill_size

[Victor Nogueira]

tcf_mirred_get_fill_size() only budgets TCA_MIRRED_PARMS, but
tcf_mirred_dump() also emits TCA_MIRRED_BLOCKID whenever the action was
created with a block instead of a device. So tcf_mirred_get_fill_size is
missing 8 bytes in its accounting.

Fix this issue by accounting for TCA_MIRRED_BLOCKID unconditionally:
it costs 8 bytes for device-mirred actions and avoids having to read
tcfm_blockid outside tcf_lock, where a concurrent replace could change
it between sizing and dumping.

Note: Dumping mirred with blocks currently works in most use cases, but
would break in some corner cases, for example, if there are 20 blockcast
mirred actions in one request, each with a non-ANY hw_stats and a
non-zero user flag, with a listener on RTNLGRP_TC (or NLM_F_ECHO):

budget 180 B/action -> attr_size = 20*180 + 24 = 3624
                    -> alloc_skb(3776) = 3776, tailroom exactly 3776
emitted 196 B/action -> 20*196 + 24 = 3944 > 3776
                     -> tca_get_fill() fails

Fixes: 42f39036cda8 ("net/sched: act_mirred: Allow mirred to block")
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
Signed-off-by: Victor Nogueira <victor@mojatatu.com>
---
 net/sched/act_mirred.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 553342c55cf7..1c98e4d81ebe 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -607,7 +607,8 @@ tcf_mirred_get_dev(const struct tc_action *a,
 
 static size_t tcf_mirred_get_fill_size(const struct tc_action *act)
 {
-	return nla_total_size(sizeof(struct tc_mirred));
+	return nla_total_size(sizeof(struct tc_mirred)) /* TCA_MIRRED_PARMS */
+		+ nla_total_size(sizeof(u32)); /* TCA_MIRRED_BLOCKID */
 }
 
 static void tcf_offload_mirred_get_dev(struct flow_action_entry *entry,
-- 
2.55.0

Re: [PATCH net 4/4] net/sched: act_mirred: account for TCA_MIRRED_BLOCKID in get_fill_size

[Simon Horman]

On Mon, Aug 24, 2026 at 12:39:03PM -0300, Victor Nogueira wrote:
> tcf_mirred_get_fill_size() only budgets TCA_MIRRED_PARMS, but
> tcf_mirred_dump() also emits TCA_MIRRED_BLOCKID whenever the action was
> created with a block instead of a device. So tcf_mirred_get_fill_size is
> missing 8 bytes in its accounting.
> 
> Fix this issue by accounting for TCA_MIRRED_BLOCKID unconditionally:
> it costs 8 bytes for device-mirred actions and avoids having to read
> tcfm_blockid outside tcf_lock, where a concurrent replace could change
> it between sizing and dumping.
> 
> Note: Dumping mirred with blocks currently works in most use cases, but
> would break in some corner cases, for example, if there are 20 blockcast
> mirred actions in one request, each with a non-ANY hw_stats and a
> non-zero user flag, with a listener on RTNLGRP_TC (or NLM_F_ECHO):
> 
> budget 180 B/action -> attr_size = 20*180 + 24 = 3624
>                     -> alloc_skb(3776) = 3776, tailroom exactly 3776
> emitted 196 B/action -> 20*196 + 24 = 3944 > 3776
>                      -> tca_get_fill() fails
> 
> Fixes: 42f39036cda8 ("net/sched: act_mirred: Allow mirred to block")
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Victor Nogueira <victor@mojatatu.com>

Hi Victor,

Although the issue flagged below by an AI-generated review of this patch[1]
may be strictly out of scope for this patchset I am pasting it here
because I see that quite some work is being undertaken to correct
get_fill_size issues and I wanted to be sure that this is considered
on the scope of that broader set of work.

[1] https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824153903.4143642-1-victor%40mojatatu.com

AI-generated review text:

  This is a pre-existing issue and not something this patch introduces, but
  while the audit is being done on get_fill_size callbacks, was act_gate
  looked at?  Its shortfall is unbounded rather than a fixed 8 bytes.

  net/sched/act_gate.c:tcf_gate_get_fill_size() {
	return nla_total_size(sizeof(struct tc_gate));
  }

  That is 24 bytes, while tcf_gate_dump() emits TCA_GATE_BASE_TIME,
  TCA_GATE_CYCLE_TIME and TCA_GATE_CYCLE_TIME_EXT via nla_put_u64_64bit(),
  plus TCA_GATE_CLOCKID, TCA_GATE_FLAGS, TCA_GATE_PRIORITY, TCA_GATE_TM and
  a per-entry nest:

  net/sched/act_gate.c:tcf_gate_dump() {
	...
	entry_list = nla_nest_start_noflag(skb, TCA_GATE_ENTRY_LIST);
	if (!entry_list)
		goto nla_put_failure;
	list_for_each_entry(entry, &p->entries, list) {
		if (dumping_entry(skb, entry) < 0)
			goto nla_put_failure;
	}
	...
  }

  Each TCA_GATE_ONE_ENTRY costs roughly 40 bytes, and parse_gate_list() puts
  no bound on the entry count:

  net/sched/act_gate.c:parse_gate_list() {
	nla_for_each_nested(n, list_attr, rem) {
		if (nla_type(n) != TCA_GATE_ONE_ENTRY) {
	...
  }

  So for one gate action the emitted dump is about 256 B plus 40 B per gate
  control list entry, against tcf_action_fill_size() returning 264 + 24 =
  288 B with an NLMSG_GOODSIZE floor of about 3776 B on x86_64.  Beyond
  roughly 88 entries, does tca_get_fill() start failing?

  If it does, two effects follow.  RTM_GETACTION on such an action returns
  -EINVAL unconditionally, since tcf_get_notify() builds the reply even with
  no listener.  And on RTM_NEWACTION with an RTNLGRP_TC listener or
  NLM_F_ECHO, the failure happens after the action is created:

  net/sched/act_api.c:tcf_action_add() {
	ret = tcf_add_notify(net, n, actions, portid, attr_size, extack);
	/* only put bound actions */
	tca_put_bound_many(actions, init_res);
	return ret;
  }

  leaving the gate action installed while userspace is told the command
  failed.  Would it make sense to mention act_gate in the series, or fix it
  in the same audit, so readers do not assume the sizing audit is complete?

Re: [PATCH net 4/4] net/sched: act_mirred: account for TCA_MIRRED_BLOCKID in get_fill_size

[Victor Nogueira]

On Thu, Aug 27, 2026 at 5:53 AM Simon Horman <horms@kernel.org> wrote:
>
> On Mon, Aug 24, 2026 at 12:39:03PM -0300, Victor Nogueira wrote:
> > tcf_mirred_get_fill_size() only budgets TCA_MIRRED_PARMS, but
> > tcf_mirred_dump() also emits TCA_MIRRED_BLOCKID whenever the action was
> > created with a block instead of a device. So tcf_mirred_get_fill_size is
> > missing 8 bytes in its accounting.
> >
> > Fix this issue by accounting for TCA_MIRRED_BLOCKID unconditionally:
> > it costs 8 bytes for device-mirred actions and avoids having to read
> > tcfm_blockid outside tcf_lock, where a concurrent replace could change
> > it between sizing and dumping.
> >
> > Note: Dumping mirred with blocks currently works in most use cases, but
> > would break in some corner cases, for example, if there are 20 blockcast
> > mirred actions in one request, each with a non-ANY hw_stats and a
> > non-zero user flag, with a listener on RTNLGRP_TC (or NLM_F_ECHO):
> >
> > budget 180 B/action -> attr_size = 20*180 + 24 = 3624
> >                     -> alloc_skb(3776) = 3776, tailroom exactly 3776
> > emitted 196 B/action -> 20*196 + 24 = 3944 > 3776
> >                      -> tca_get_fill() fails
> >
> > Fixes: 42f39036cda8 ("net/sched: act_mirred: Allow mirred to block")
> > Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
> > Signed-off-by: Victor Nogueira <victor@mojatatu.com>
>
> Hi Victor,
>
> Although the issue flagged below by an AI-generated review of this patch[1]
> may be strictly out of scope for this patchset I am pasting it here
> because I see that quite some work is being undertaken to correct
> get_fill_size issues and I wanted to be sure that this is considered
> on the scope of that broader set of work.
>
> [1] https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824153903.4143642-1-victor%40mojatatu.com
>
> AI-generated review text:
>
>   This is a pre-existing issue and not something this patch introduces, but
>   while the audit is being done on get_fill_size callbacks, was act_gate
>   looked at?  Its shortfall is unbounded rather than a fixed 8 bytes.
>
>   net/sched/act_gate.c:tcf_gate_get_fill_size() {
>         return nla_total_size(sizeof(struct tc_gate));
>   }
>
>   That is 24 bytes, while tcf_gate_dump() emits TCA_GATE_BASE_TIME,
>   TCA_GATE_CYCLE_TIME and TCA_GATE_CYCLE_TIME_EXT via nla_put_u64_64bit(),
>   plus TCA_GATE_CLOCKID, TCA_GATE_FLAGS, TCA_GATE_PRIORITY, TCA_GATE_TM and
>   a per-entry nest:
>
>   net/sched/act_gate.c:tcf_gate_dump() {
>         ...
>         entry_list = nla_nest_start_noflag(skb, TCA_GATE_ENTRY_LIST);
>         if (!entry_list)
>                 goto nla_put_failure;
>         list_for_each_entry(entry, &p->entries, list) {
>                 if (dumping_entry(skb, entry) < 0)
>                         goto nla_put_failure;
>         }
>         ...
>   }
>
>   Each TCA_GATE_ONE_ENTRY costs roughly 40 bytes, and parse_gate_list() puts
>   no bound on the entry count:
>
>   net/sched/act_gate.c:parse_gate_list() {
>         nla_for_each_nested(n, list_attr, rem) {
>                 if (nla_type(n) != TCA_GATE_ONE_ENTRY) {
>         ...
>   }
>
>   So for one gate action the emitted dump is about 256 B plus 40 B per gate
>   control list entry, against tcf_action_fill_size() returning 264 + 24 =
>   288 B with an NLMSG_GOODSIZE floor of about 3776 B on x86_64.  Beyond
>   roughly 88 entries, does tca_get_fill() start failing?
>
>   If it does, two effects follow.  RTM_GETACTION on such an action returns
>   -EINVAL unconditionally, since tcf_get_notify() builds the reply even with
>   no listener.  And on RTM_NEWACTION with an RTNLGRP_TC listener or
>   NLM_F_ECHO, the failure happens after the action is created:
>
>   net/sched/act_api.c:tcf_action_add() {
>         ret = tcf_add_notify(net, n, actions, portid, attr_size, extack);
>         /* only put bound actions */
>         tca_put_bound_many(actions, init_res);
>         return ret;
>   }
>
>   leaving the gate action installed while userspace is told the command
>   failed.  Would it make sense to mention act_gate in the series, or fix it
>   in the same audit, so readers do not assume the sizing audit is complete?

This looks like a legit issue, but I think it should be part of another series.
In the case of act_gate, I think we should limit the maximum number of
act_gate control list entries first.
Since those are unbounded today, the message size could grow
significantly, creating a clear path for excessive memory allocations.
Analogously, it seems like sch_taprio has a similar issue...
Since the entires there are also unbounded, it might not fit on a dump
which always allocates NLMSG_GOODSIZE.
Will also take a look later.

cheers,
Victor

Re: [PATCH net 4/4] net/sched: act_mirred: account for TCA_MIRRED_BLOCKID in get_fill_size

[Paolo Abeni]

On 8/24/26 5:39 PM, Victor Nogueira wrote:
> tcf_mirred_get_fill_size() only budgets TCA_MIRRED_PARMS, but
> tcf_mirred_dump() also emits TCA_MIRRED_BLOCKID whenever the action was
> created with a block instead of a device. So tcf_mirred_get_fill_size is
> missing 8 bytes in its accounting.
> 
> Fix this issue by accounting for TCA_MIRRED_BLOCKID unconditionally:
> it costs 8 bytes for device-mirred actions and avoids having to read
> tcfm_blockid outside tcf_lock, where a concurrent replace could change
> it between sizing and dumping.
> 
> Note: Dumping mirred with blocks currently works in most use cases, but
> would break in some corner cases, for example, if there are 20 blockcast
> mirred actions in one request, each with a non-ANY hw_stats and a
> non-zero user flag, with a listener on RTNLGRP_TC (or NLM_F_ECHO):
> 
> budget 180 B/action -> attr_size = 20*180 + 24 = 3624
>                     -> alloc_skb(3776) = 3776, tailroom exactly 3776
> emitted 196 B/action -> 20*196 + 24 = 3944 > 3776
>                      -> tca_get_fill() fails

Sashiko also noted that the above should not be reachable in practice on
top of the previous patches. If so, would it make sense to divert this
patch to net-next with a slightly different changelog (and no fixes tag)?

I think the first 3 patches are good as is (i.e. I agree with no need
for accounting in patch 2/4, ATM).

/P

Re: [PATCH net 4/4] net/sched: act_mirred: account for TCA_MIRRED_BLOCKID in get_fill_size

[Victor Nogueira]

On Thu, Aug 27, 2026 at 6:24 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On 8/24/26 5:39 PM, Victor Nogueira wrote:
> > tcf_mirred_get_fill_size() only budgets TCA_MIRRED_PARMS, but
> > tcf_mirred_dump() also emits TCA_MIRRED_BLOCKID whenever the action was
> > created with a block instead of a device. So tcf_mirred_get_fill_size is
> > missing 8 bytes in its accounting.
> >
> > Fix this issue by accounting for TCA_MIRRED_BLOCKID unconditionally:
> > it costs 8 bytes for device-mirred actions and avoids having to read
> > tcfm_blockid outside tcf_lock, where a concurrent replace could change
> > it between sizing and dumping.
> >
> > Note: Dumping mirred with blocks currently works in most use cases, but
> > would break in some corner cases, for example, if there are 20 blockcast
> > mirred actions in one request, each with a non-ANY hw_stats and a
> > non-zero user flag, with a listener on RTNLGRP_TC (or NLM_F_ECHO):
> >
> > budget 180 B/action -> attr_size = 20*180 + 24 = 3624
> >                     -> alloc_skb(3776) = 3776, tailroom exactly 3776
> > emitted 196 B/action -> 20*196 + 24 = 3944 > 3776
> >                      -> tca_get_fill() fails
>
> Sashiko also noted that the above should not be reachable in practice on
> top of the previous patches. If so, would it make sense to divert this
> patch to net-next with a slightly different changelog (and no fixes tag)?

Yes, I think Sashiko has a point there.
I can send this patch as a net-next patch once it reopens.

cheers,
Victor

Re: [PATCH net 0/4] net/sched: Fix remaining actions notification accounting issues

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 24 Aug 2026 12:38:59 -0300 you wrote:
> Commit 8e2efb3f45a5 ("net/sched: add get_fill_size callbacks for actions
> missing them") fixed the reported echo/notify skb overrun and noted that
> the pre-existing issues Sashiko pointed out [1] would be fixed separately.
> This is that series.
> 
> Patch 1 makes tcf_action_shared_attrs_size() a real upper bound again.
> TCA_ACT_IN_HW_COUNT and TCA_STATS_BASIC_HW are emitted on every action
> dump and were never budgeted; TCA_STATS_PKT64 was budgeted once but can be
> emitted twice; TCA_ACT_USED_HW_STATS and the rate estimator attributes are
> conditional and also unaccounted.
> 
> [...]

Here is the summary with links:
  - [net,1/4] net/sched: act_api: budget all shared attributes in notify skbs
    https://git.kernel.org/netdev/net/c/13eb543cebef
  - [net,2/4] net/sched: act_api: size the RTM_GETACTION reply from the actions
    https://git.kernel.org/netdev/net/c/e9ca46ebc326
  - [net,3/4] net/sched: act_api: fix skb sizing and action leak on reoffload delete
    https://git.kernel.org/netdev/net/c/251367a0a331
  - [net,4/4] net/sched: act_mirred: account for TCA_MIRRED_BLOCKID in get_fill_size
    (no matching commit)

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html