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

[Simon Horman <horms@kernel.org>]

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?