Re: [PATCH net-next v3 5/5] net/sched: act_api: report an oversized action dump instead of silently truncating it

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch has no Fixes: tag for the old silent-truncation path in
  tcf_dump_walker() (`index--; nlmsg_trim; goto done; return n_i`).
- [Low] The new comment above `return n_i ?: err;` in tcf_dump_walker()
  (net/sched/act_api.c:625-630) does not match the code or af_netlink's…

--

net/sched: act_api: report an oversized action dump instead of silently truncating it

When an action does not fit even into an empty dump skb, tcf_dump_walker()
now returns -EMSGSIZE with an extack instead of 0. tc_dump_action() now
passes the walker's error up instead of returning skb->len. Before this
change, "tc actions ls" silently left out that action and every action
after it.

Closes: https://lore.kernel.org/netdev/vPFrhbkwAui8zFPcGQTUDazQfsFDlr5GYuaYQ5BxJcrDiT6x1EljEze4cYadHqbeUW2ivLdpa8eC_cXC6ceACswgb-lW47aSVkHvHjhn_X8=@1g4.org/

> That case is reachable inside the gate entry cap this series adds: a gate
> entry serializes to 36 bytes (40 with the gate-open flag) while a dump skb
> is clamped to SKB_WITH_OVERHEAD(32768) by netlink_recvmsg()/netlink_dump(),
> so the top of the range the cap admits installs but cannot be dumped -
> and, before this change, fails silently.

[ ... ]

> This is a long-standing silent failure, not a regression, so it
> is net-next hardening.

[Severity: Low]
Would a Fixes: tag for the old silent truncation path in tcf_dump_walker()
be useful here, even though this goes to net-next?

The commit message ties the trigger to the gate entry cap in this series.
The same truncation can also be hit without the series. act_bpf_policy in
net/sched/act_bpf.c allows:

	[TCA_ACT_BPF_OPS]	= { .type = NLA_BINARY,
				    .len = sizeof(struct sock_filter) * BPF_MAXINSNS },

That is 32768 bytes of ops, which is more than SKB_WITH_OVERHEAD(32768).
So a single classic bpf action can already be too large for the dump skb.

Leaving out Fixes: may be on purpose, because the patch turns a silent
success into an error and is kept out of stable. If so, could the commit
message say that the problem is not limited to the gate series?

> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index fd7f60e0f62ba..33dba34364541 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c

[ ... ]

> @@ -626,10 +621,21 @@ static int tcf_dump_walker(struct tcf_idrinfo *idrinfo, struct sk_buff *skb,
>  		if (act_flags & TCA_ACT_FLAG_LARGE_DUMP_ON)
>  			cb->args[1] = n_i;
>  	}
> -	return n_i;
> +
> +	/* A positive count means the skb filled up before this action was
> +	 * complete, so netlink retries it in a fresh skb. An action that
> +	 * does not fit even in an empty dump skb (nothing dumped) must be
> +	 * reported as -EMSGSIZE by netlink instead of ending the dump.
> +	 */
> +	return n_i ?: err;

[Severity: Low]
Is this comment right for every path that reaches this return in
tcf_dump_walker()?

A positive n_i also comes back when the idr walk finishes after dumping
every action. It also comes back when the loop stops here:

		if (!(act_flags & TCA_ACT_FLAG_LARGE_DUMP_ON) &&
		    n_i >= TCA_ACT_MAX_PRIO)
			goto done;

The skb did not fill up in either case. After a normal finish there is
also nothing left to retry.

The second sentence says -EMSGSIZE is reported "instead of ending the
dump". In netlink_dump() in net/netlink/af_netlink.c, a negative return
with an empty skb fails this check:

	if (nlk->dump_done_errno > 0 ||
	    skb_tailroom(skb) < nlmsg_total_size(sizeof(nlk->dump_done_errno))) {

It then goes on to netlink_dump_done(), which sends NLMSG_DONE with the
error and extack, and the dump ends. So the dump still ends, but now with
an error instead of silently.

Could the comment be reworded to match the return contract that
tc_dump_action() now relies on?

[ ... ]

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