Re: [PATCH net-next 1/2] 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 5 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 (1) · Low: 3

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] This patch makes an unbuildable notification block
  RTM_DELACTION.
- [Medium] RTM_NEWACTION for an action set whose dump exceeds U16_MAX now
  returns -EINVAL even though the actions have already been created or…
- [Low] On the -EMSGSIZE branch, tfilter_del_notify()
  (net/sched/cls_api.c) sets the extack message "Filter deleted; del…
- [Low] The commit message misdescribes what the code did before the
  patch.
- [Low] In tfilter_del_notify(), -EMSGSIZE is taken to mean that the dump
  cannot be represented in a u16 nest ('no notification can ever be…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  tfilter_del_notify() still returns without calling ->delete() when…

--

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

This commit switches the nest closes on the tc action and classifier dump
paths to nla_nest_end_safe(). A dump larger than U16_MAX now reports
-EMSGSIZE instead of writing a wrapped nla_len. It also bounds the
tfilter_notify_prep() retry loop, and makes tfilter_del_notify() drop a
notification it cannot build and delete the filter anyway.

Link: https://lore.kernel.org/netdev/QDISC-H19Z.v1.20261001053234@mojatatu.com/
Link: https://lore.kernel.org/netdev/179111893181.434549.13449660670267048994@kernel.org/

> tfilter_notify_prep() treats that as socket-buffer exhaustion and
> retries with an ever larger alloc_skb(); a dump that does not fit a u16
> nest cannot be built at any skb size, so the loop only spins until the
> allocation itself fails.

[Severity: Low]
Does this describe the code as it was before this patch?

Before, nla_nest_end() could not fail:

include/net/netlink.h:nla_nest_end() {
	unsigned long diff = skb_tail_pointer(skb) - (unsigned char *)start;

	DEBUG_NET_WARN_ON_ONCE(diff > U16_MAX);
	start->nla_len = diff;
	return skb->len;
}

Nothing under net/sched used nla_nest_end_safe(). So tcf_fill_node()
returned -EMSGSIZE only while the skb was short on tailroom. Once
tfilter_notify_prep() had grown the skb enough, the fill succeeded with a
wrapped length and the loop stopped.

The loop that runs until alloc_skb() fails seems to exist only after this
patch turns the u16 overflow into a fill failure. That makes the new bound
in tfilter_notify_prep() a requirement of the nla_nest_end_safe()
conversions, not a fix for an existing loop.

Could the commit message say this? A backport that takes the conversions
without the bound would bring in the allocation escalation loop.

> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 6e48b4bc2d75..747d91ae6446 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c

[ ... ]

> @@ -1693,7 +1696,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: High]
With this change, tca_get_fill() fails for an action set whose TCA_ACT_TAB
is larger than U16_MAX. Before, the same fill succeeded with a wrapped
length. Does this make a failed notification build refuse an
RTM_DELACTION?

tcf_del_notify() builds the notification before it deletes anything:

net/sched/act_api.c:tcf_del_notify() {
    ...
	} else {
		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);
    ...
}

tcf_del_notify_msg() turns any tca_get_fill() failure into -EINVAL:

	if (tca_get_fill(skb, actions, portid, n->nlmsg_seq, 0, RTM_DELACTION,
			 0, 2, extack) <= 0) {
		NL_SET_ERR_MSG(extack, "Failed to fill netlink TC action attributes");
		kfree_skb(skb);
		return ERR_PTR(-EINVAL);
	}

The full path is:

tc_ctl_action(RTM_DELACTION)
  tca_action_gd()
    tcf_del_notify()
      tcf_del_notify_msg()
        tca_get_fill()
          nla_nest_end_safe() < 0

tcf_action_delete() is never reached.

With the 1024-entry cap applied, "tc actions del action gate index 1
action gate index 2" on two 1024-entry gate actions looks like it would
fail with -EINVAL and leave both actions installed. The commit message
says this two-action shape survives the cap.

The cap patch later in this series only adds GATE_ENTRIES_MAX in
parse_gate_list(). It does not touch tcf_del_notify(). Without the cap
patch, a single gate action with about 1815 or more entries could not be
deleted by index.

This needs rtnl_notify_needed() to be true, meaning an RTNLGRP_TC
listener or NLM_F_ECHO. rtnetlink allows non-root receive, so an
unprivileged listener in the netns seems to be enough to make an admin's
delete fail.

The commit message says "A notification that cannot be serialised must
not veto the state change the caller asked for", and tfilter_del_notify()
is changed to match. tcf_reoffload_del_notify() in this file already
drops a notification it cannot build:

		if (IS_ERR(skb))
			skb = NULL;

Should tcf_del_notify() be changed the same way? tca_get_fill() returns a
bare -1, so -EMSGSIZE would also need to be passed up. Otherwise the
delete path cannot tell the structural overflow from other failures,
which tfilter_del_notify() now relies on.

[Severity: Medium]
The same failure also shows up on the add side.
tcf_action_add()->tcf_action_init() has already inserted or replaced the
actions by the time tcf_add_notify()->tcf_add_notify_msg() calls
tca_get_fill():

	if (tca_get_fill(skb, actions, portid, n->nlmsg_seq, n->nlmsg_flags,
			 RTM_NEWACTION, 0, 0, extack) <= 0) {
		NL_SET_ERR_MSG(extack, "Failed to fill netlink attributes while adding TC action");
		kfree_skb(skb);
		return ERR_PTR(-EINVAL);
	}

The commit message notes this case and leaves it for later. However,
before this patch an oversized set still filled (with wrapped lengths)
and the add reported success. Isn't the "-EINVAL after the actions were
created" result new for these sets?

For example, with the cap applied, adding two 1024-entry gate actions in
one request now returns -EINVAL with the actions installed. This happens
when an RTNLGRP_TC listener exists or NLM_F_ECHO is set. A retry would
then hit -EEXIST.

>  
>  	nlh->nlmsg_len = skb_tail_pointer(skb) - b;
>  
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index a9f54988561f..3f0ce567fa2d 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c

[ ... ]

> @@ -2200,8 +2209,16 @@ static int tfilter_del_notify(struct net *net, struct sk_buff *oskb,
>  	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);
> +		if (PTR_ERR(skb) != -EMSGSIZE) {
> +			NL_SET_ERR_MSG(extack, "Failed to build del event notification");
> +			return PTR_ERR(skb);
> +		}

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. This
branch still returns without calling ->delete() when
tfilter_notify_prep() fails with something other than -EMSGSIZE, such as
-ENOBUFS from alloc_skb() or -EINVAL. Does that leak the flower filter
reference taken during the lookup?

In tc_del_tfilter(), fh comes from tp->ops->get(). For flower,
fl_get()->__fl_get() takes a reference:

	if (f && !refcount_inc_not_zero(&f->refcnt))
		f = NULL;

fl_delete() normally drops it with __fl_put(f). On this error path,
tc_del_tfilter() does:

		err = tfilter_del_notify(net, skb, n, tp, block, q, parent, fh,
					 &last, rtnl_held, extack);

		if (err)
			goto errout;

errout releases tp, chain and block, but never calls tfilter_put(tp, fh).
After that, even a later successful delete would not free the filter or
the actions bound to it.

The alloc_skb() failure seems realistic here. The retry loop grows the
skb toward U16_MAX + NLMSG_GOODSIZE, which needs order 4-5 allocations.

> +		/* The filter's dump cannot be represented in a u16 nest, so no
> +		 * notification can ever be built for it. Drop the notification
> +		 * rather than refusing to delete the filter.
> +		 */

[Severity: Low]
Does -EMSGSIZE always mean the dump cannot fit in a u16 nest?
tcf_fill_node() starts with:

	int ret = -EMSGSIZE;

and returns that for every ->dump() failure. Some dumps fail for reasons
that have nothing to do with size. One example is tcf_pedit_dump():

	opt = kzalloc(s, GFP_ATOMIC);
	if (unlikely(!opt)) {
		rcu_read_unlock();
		return -ENOBUFS;

u32_dump() also allocates with kzalloc_flex(), and tcf_action_dump_1()
reduces any error to -1.

If such a failure repeats through every retry in tfilter_notify_prep()
(about 17 with 4K pages), the loop returns ERR_PTR(-EMSGSIZE). This
branch then drops an RTM_DELTFILTER event that could have been built. The
comment in tfilter_notify_prep() only mentions structural overflow and
genuine capacity exhaustion.

> +		NL_SET_ERR_MSG(extack, "Filter deleted; del event notification could not be built");
> +		return tp->ops->delete(tp, fh, last, rtnl_held, extack);

[Severity: Low]
The extack text is set before ->delete() runs. If ->delete() then fails
without writing its own extack message, won't userspace get "Filter
deleted" together with a negative errno?

Flower, for example, runs without rtnl on an unlocked qdisc. Two
concurrent RTM_DELTFILTER requests for the same oversized filter can both
get through fl_get() and both hit -EMSGSIZE in prep. The second one then
reaches __fl_delete():

	if (f->deleted) {
		spin_unlock(&tp->lock);
		return -ENOENT;
	}

That path does not set extack. fw_delete() can return -EINVAL the same
way.

Could the message be set only after ->delete() returns 0?

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

[ ... ]

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