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

[Simon Horman <horms@kernel.org>]

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.