Re: [PATCH net] net/sched: act_gate: reject oversized dumps instead of wrapping them

[netdev-bot+sashiko@kernel.org]

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

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

- [High] The new nla_nest_end_safe() failures turn the filter
  notification path into a retry loop that cannot succeed.
- [Medium] The patch is incomplete on the classifier path, which is the
  only path where its inner-nest checks change behaviour.
- [Medium] For gates of about 1815 entries or more, the patch brings back
  the action lifecycle problem that cfa165cbfbed was meant to fix.
- [Low] The commit message and Fixes tag understate what the patch
  changes.

--

net/sched: act_gate: reject oversized dumps instead of wrapping them

This patch closes four nests that can wrap with nla_nest_end_safe():
TCA_GATE_ENTRY_LIST, TCA_ACT_OPTIONS, the per-action nest and
TCA_ACT_TAB. It follows up on cfa165cbfbed, which sized the gate reply
from the real dump without bounding the entry count. When a dump would
wrap, it now fails through the existing trim/error labels instead of
emitting a corrupt nla_len.

Closes: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824153903.4143642-1-victor@mojatatu.com

> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index e45a63be397ce..c7e87491a9fce 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c

[ ... ]

> @@ -1270,7 +1271,8 @@ int tcf_action_dump(struct sk_buff *skb, struct tc_action *actions[],
>  			tcf_action_dump_1(skb, a, bind, ref);
>  		if (err < 0)
>  			goto errout;
> -		nla_nest_end(skb, nest);
> +		if (nla_nest_end_safe(skb, nest) < 0)
> +			goto nla_put_failure;

[Severity: High]
Can this check make the filter notification path retry forever without
ever succeeding?

Suppose a gate's per-action nest (about 1815 entries) or its
TCA_GATE_ENTRY_LIST (about 1821 entries) is larger than U16_MAX. Then
tcf_action_dump() here, or tcf_gate_dump(), fails no matter how big the
skb is. On the classifier path that failure is reported as -EMSGSIZE:

fl_dump() / mall_dump()
  tcf_exts_dump()
    tcf_action_dump()
      nla_nest_end_safe() fails

net/sched/cls_api.c:tcf_fill_node() {
	...
	int ret = -EMSGSIZE;
	...
}

tfilter_notify_prep() then treats that as a buffer that is too small:

net/sched/cls_api.c:tfilter_notify_prep() {
	...
	if (ret <= 0) {
		kfree_skb(skb);
		if (ret == -EMSGSIZE) {
			size += NLMSG_GOODSIZE;
			goto retry;
		}
		return ERR_PTR(-EINVAL);
	}
	...
}

The number of retries has no limit. The loop only ends when alloc_skb()
fails quietly (__GFP_NOWARN) above MAX_PAGE_ORDER, and then it returns
-ENOBUFS. On 4K pages that is around 4MB, or roughly 1100 iterations.
Each one makes a larger GFP_KERNEL high-order allocation and serializes
every gate entry again. With 64K pages the loop runs much longer.

Before this patch the same loop stopped after about 18 iterations, once
the roughly 66KB dump fit.

RTM_GETTFILTER needs no capability. Once such a filter exists, any
process in the netns can trigger this, repeatedly and from many threads.

tfilter_del_notify() also builds the notification before it deletes:

	skb = tfilter_notify_prep(net, oskb, n, tp, block, q, parent, fh,
				  RTM_DELTFILTER, portid, rtnl_held, extack);
	if (IS_ERR(skb)) {
		NL_SET_ERR_MSG(extack, "Failed to build del event notification");
		return PTR_ERR(skb);
	}

	err = tp->ops->delete(tp, fh, last, rtnl_held, extack);

RTNLGRP_TC accepts listeners without privileges. One such listener would
now make RTM_DELTFILTER for this kind of filter fail after the long
loop, where it used to succeed. RTM_NEWTFILTER notifications would also
be lost.

Doesn't this contradict "no configuration that works today changes
behavior" in the commit message?

One option is to report nest overflow with an errno other than -EMSGSIZE
and pass it up through tcf_exts_dump() and tcf_fill_node(). Another is
to cap the retries in tfilter_notify_prep(). Would either of those work
here?

[Severity: Medium]
Can the classifier path still emit wrapped nest lengths?

This checks each action's nest on its own. The enclosing nests on the
filter path are still closed with plain nla_nest_end():

net/sched/cls_api.c:tcf_exts_dump() {
	...
			if (tcf_action_dump(skb, exts->actions, 0, 0, false)
			    < 0)
				goto nla_put_failure;
			nla_nest_end(skb, nest);
	...
}

fl_dump() and mall_dump() close TCA_OPTIONS the same way.

Take a flower or matchall filter with two gates of about 1000 entries
each. Each per-action nest is about 36KB and passes the new check, but
the exts->action nest adds up to about 73KB.

A single gate of about 1805-1814 entries is well formed on
RTM_GETACTION. On a flower filter, though, the extra key attributes can
push TCA_FLOWER_ACT/TCA_OPTIONS past U16_MAX.

tfilter_notify_prep() keeps growing the skb until the fill succeeds. So
RTM_GETTFILTER would still return a gate dump inside a nest with a
wrapped nla_len.

The wrap itself is older than this patch; it comes from the retry loop
added in 369609fc6272c2. Still, the new inner checks only change
behavior on this classifier path. tca_get_fill() already checks
TCA_ACT_TAB, and tcf_dump_walker() uses skbs of about 32KB.

Should tcf_exts_dump(), fl_dump() and mall_dump() also use
nla_nest_end_safe(), so that "reject oversized dumps instead of wrapping
them" also holds for filter dumps?

>  	}
>  
>  	return 0;
> @@ -1684,7 +1686,8 @@ static int tca_get_fill(struct sk_buff *skb, struct tc_action *actions[],
>  	if (tcf_action_dump(skb, actions, bind, ref, false) < 0)
>  		goto out_nlmsg_trim;
>  
> -	nla_nest_end(skb, nest);
> +	if (nla_nest_end_safe(skb, nest) < 0)
> +		goto out_nlmsg_trim;

[Severity: Medium]
For gates of about 1815 entries or more, does this bring back the
add/delete lifecycle problem that cfa165cbfbed was meant to fix?

In tcf_action_add(), tcf_action_init() has already committed the gate
to the IDR before the notification is built:

	ret = tcf_add_notify(net, n, actions, portid, attr_size, extack);

	/* only put bound actions */
	tca_put_bound_many(actions, init_res);

Consider the case where there is an RTNLGRP_TC listener (tc monitor, for
example) or NLM_F_ECHO is set. tcf_add_notify_msg()->tca_get_fill() now
fails here, so user space gets -EINVAL but the gate stays installed.

The message of cfa165cbfbed describes this exact case: "RTM_NEWACTION
then fails the add-notify with -EINVAL while the action is already
committed to the IDR".

The delete side also builds the message before it deletes:

net/sched/act_api.c:tcf_del_notify() {
	...
		skb = tcf_del_notify_msg(net, n, actions, portid, attr_size,
					 extack);
		if (IS_ERR(skb))
			return PTR_ERR(skb);
	}

	/* now do the delete */
	ret = tcf_action_delete(net, actions);
	...
}

With a listener present, RTM_DELACTION by index can no longer remove the
gate. Only a flush (NLM_F_ROOT) or netns teardown frees it. RTNLGRP_TC
accepts members without privileges, so an unprivileged listener is
enough to cause this.

In the parent tree both operations succeeded, though they sent a wrapped
notification. Is the statement "no configuration that works today
changes behavior" accurate for this case?

The init-time entry cap that would avoid this has been deferred to
net-next.

[Severity: Low]
This isn't a bug, but the commit message and Fixes tag look narrower
than what this hunk changes. The commit message says:

  this patch closes only the malformed uAPI the sizing fix introduced.

The TCA_ACT_TAB check in tca_get_fill() is core act_api code, so it
applies to every action type. That wrap could already happen without
cfa165cbfbed. Since 8e2efb3f45a5c6 ("net/sched: add get_fill_size
callbacks for actions missing them"), tcf_bpf_get_fill_size() budgets
this:

		size += nla_total_size(prog->bpf_num_ops *
				       sizeof(struct sock_filter));

For a classic BPF action with 4096 instructions that is up to 32KB.

tca_action_gd() adds up the fill sizes of up to TCA_ACT_MAX_PRIO
actions. tcf_get_notify() and tcf_del_notify_msg() then allocate
max(attr_size, NLMSG_GOODSIZE).

So an RTM_GETACTION or RTM_DELACTION that names two large classic BPF
actions by index already overflowed TCA_ACT_TAB. This patch now turns
those replies into -EINVAL.

The filter-path wrap of the gate's inner nests also goes back to
369609fc6272c2, not cfa165cbfbed.

Could the commit message mention the effect on other action types, and
could the Fixes tags point at the commits that introduced these wraps?
8e2efb3f45a5c6 and cfa165cbfbed both carry Fixes: 4e76e75d6aba and will
probably be backported together, so the practical impact is small.

>  
>  	nlh->nlmsg_len = skb_tail_pointer(skb) - b;
>  

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/QDISC-H19Z.v1.20261001053234%40mojatatu.com