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

[Simon Horman <horms@kernel.org>]

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?