[PATCH net-next] tc: Return an error if filters try to attach too many actions

[PATCH net-next] tc: Return an error if filters try to attach too many actions

[Toke Høiland-Jørgensen]

While developing the fix for the buffer sizing issue in [0], I noticed
that the kernel will happily accept a long list of actions for a filter,
and then just silently truncate that list down to a maximum of 32
actions.

That seems less than ideal, so this patch changes the action parsing to
return an error message and refuse to create the filter in this case.
This results in an error like:

 # ip link add type veth
 # tc qdisc replace dev veth0 root handle 1: fq_codel
 # tc -echo filter add dev veth0 parent 1: u32 match u32 0 0 $(for i in $(seq 33); do echo action pedit munge ip dport set 22; done)
Error: Only 32 actions supported per filter.
We have an error talking to the kernel

Instead of just creating a filter with 32 actions and dropping the last
one.

This is obviously a change in UAPI. But seeing as creating more than 32
filters has never actually *worked*, it seems that returning an explicit
error is better, and any use cases that get broken by this were already
broken just in more subtle ways.

[0] https://lore.kernel.org/r/20250407105542.16601-1-toke@redhat.com

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 net/sched/act_api.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 839790043256..057e20cef375 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -1461,17 +1461,29 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
 		    struct netlink_ext_ack *extack)
 {
 	struct tc_action_ops *ops[TCA_ACT_MAX_PRIO] = {};
-	struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
+	struct nlattr *tb[TCA_ACT_MAX_PRIO + 2];
 	struct tc_action *act;
 	size_t sz = 0;
 	int err;
 	int i;
 
-	err = nla_parse_nested_deprecated(tb, TCA_ACT_MAX_PRIO, nla, NULL,
+	err = nla_parse_nested_deprecated(tb, TCA_ACT_MAX_PRIO + 1, nla, NULL,
 					  extack);
 	if (err < 0)
 		return err;
 
+	/* The nested attributes are parsed as types, but they are really an
+	 * array of actions. So we parse one more than we can handle, and return
+	 * an error if the last one is set (as that indicates that the request
+	 * contained more than the maximum number of actions).
+	 */
+	if (tb[TCA_ACT_MAX_PRIO + 1]) {
+		NL_SET_ERR_MSG_FMT(extack,
+				   "Only %d actions supported per filter",
+				   TCA_ACT_MAX_PRIO);
+		return -EINVAL;
+	}
+
 	for (i = 1; i <= TCA_ACT_MAX_PRIO && tb[i]; i++) {
 		struct tc_action_ops *a_o;
 
-- 
2.49.0

Re: [PATCH net-next] tc: Return an error if filters try to attach too many actions

[Cong Wang]

On Wed, Apr 09, 2025 at 04:55:23PM +0200, Toke Høiland-Jørgensen wrote:
> While developing the fix for the buffer sizing issue in [0], I noticed
> that the kernel will happily accept a long list of actions for a filter,
> and then just silently truncate that list down to a maximum of 32
> actions.
> 
> That seems less than ideal, so this patch changes the action parsing to
> return an error message and refuse to create the filter in this case.
> This results in an error like:
> 
>  # ip link add type veth
>  # tc qdisc replace dev veth0 root handle 1: fq_codel
>  # tc -echo filter add dev veth0 parent 1: u32 match u32 0 0 $(for i in $(seq 33); do echo action pedit munge ip dport set 22; done)
> Error: Only 32 actions supported per filter.
> We have an error talking to the kernel
> 
> Instead of just creating a filter with 32 actions and dropping the last
> one.
> 
> This is obviously a change in UAPI. But seeing as creating more than 32
> filters has never actually *worked*, it seems that returning an explicit
> error is better, and any use cases that get broken by this were already
> broken just in more subtle ways.
> 
> [0] https://lore.kernel.org/r/20250407105542.16601-1-toke@redhat.com
> 
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>  net/sched/act_api.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index 839790043256..057e20cef375 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -1461,17 +1461,29 @@ int tcf_action_init(struct net *net, struct tcf_proto *tp, struct nlattr *nla,
>  		    struct netlink_ext_ack *extack)
>  {
>  	struct tc_action_ops *ops[TCA_ACT_MAX_PRIO] = {};
> -	struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
> +	struct nlattr *tb[TCA_ACT_MAX_PRIO + 2];
>  	struct tc_action *act;
>  	size_t sz = 0;
>  	int err;
>  	int i;
>  
> -	err = nla_parse_nested_deprecated(tb, TCA_ACT_MAX_PRIO, nla, NULL,
> +	err = nla_parse_nested_deprecated(tb, TCA_ACT_MAX_PRIO + 1, nla, NULL,
>  					  extack);
>  	if (err < 0)
>  		return err;
>  
> +	/* The nested attributes are parsed as types, but they are really an
> +	 * array of actions. So we parse one more than we can handle, and return
> +	 * an error if the last one is set (as that indicates that the request
> +	 * contained more than the maximum number of actions).
> +	 */
> +	if (tb[TCA_ACT_MAX_PRIO + 1]) {
> +		NL_SET_ERR_MSG_FMT(extack,
> +				   "Only %d actions supported per filter",
> +				   TCA_ACT_MAX_PRIO);
> +		return -EINVAL;

I wonder ENOSPC is a better errno than EINVAL here?

Thanks.

Re: [PATCH net-next] tc: Return an error if filters try to attach too many actions

[Jakub Kicinski]

On Wed, 9 Apr 2025 09:44:32 -0700 Cong Wang wrote:
> > +	if (tb[TCA_ACT_MAX_PRIO + 1]) {
> > +		NL_SET_ERR_MSG_FMT(extack,
> > +				   "Only %d actions supported per filter",
> > +				   TCA_ACT_MAX_PRIO);
> > +		return -EINVAL;  
> 
> I wonder ENOSPC is a better errno than EINVAL here?

I think EINVAL is fine, it's the generic "netlink says no" error code. 
The string error should be clear enough.

Re: [PATCH net-next] tc: Return an error if filters try to attach too many actions

[Cong Wang]

On Wed, Apr 09, 2025 at 05:10:16PM -0700, Jakub Kicinski wrote:
> On Wed, 9 Apr 2025 09:44:32 -0700 Cong Wang wrote:
> > > +	if (tb[TCA_ACT_MAX_PRIO + 1]) {
> > > +		NL_SET_ERR_MSG_FMT(extack,
> > > +				   "Only %d actions supported per filter",
> > > +				   TCA_ACT_MAX_PRIO);
> > > +		return -EINVAL;  
> > 
> > I wonder ENOSPC is a better errno than EINVAL here?
> 
> I think EINVAL is fine, it's the generic "netlink says no" error code. 
> The string error should be clear enough.

IMHO, EINVAL is abused (which is probably why we introduced extack). I
prefer to find a better errno than EINVAL whenever possible.

Extack is available but it is mostly for human to read, not technically
an API for programs to interpret.

Thanks.

Re: [PATCH net-next] tc: Return an error if filters try to attach too many actions

[Jakub Kicinski]

On Mon, 14 Apr 2025 14:03:31 -0700 Cong Wang wrote:
> > > I wonder ENOSPC is a better errno than EINVAL here?  
> > 
> > I think EINVAL is fine, it's the generic "netlink says no" error code. 
> > The string error should be clear enough.  
> 
> IMHO, EINVAL is abused (which is probably why we introduced extack). I
> prefer to find a better errno than EINVAL whenever possible.
>  
> Extack is available but it is mostly for human to read, not technically
> an API for programs to interpret.

How is user space going to interpret the error code here?
Seems to me that'd mean the user space is both aware of the limit 
and yet trying to send more actions.

Re: [PATCH net-next] tc: Return an error if filters try to attach too many actions

[Paolo Abeni]

On 4/14/25 11:24 PM, Jakub Kicinski wrote:
> On Mon, 14 Apr 2025 14:03:31 -0700 Cong Wang wrote:
>>>> I wonder ENOSPC is a better errno than EINVAL here?  
>>>
>>> I think EINVAL is fine, it's the generic "netlink says no" error code. 
>>> The string error should be clear enough.  
>>
>> IMHO, EINVAL is abused (which is probably why we introduced extack). I
>> prefer to find a better errno than EINVAL whenever possible.
>>  
>> Extack is available but it is mostly for human to read, not technically
>> an API for programs to interpret.
> 
> How is user space going to interpret the error code here?
> Seems to me that'd mean the user space is both aware of the limit 
> and yet trying to send more actions.

FWIW I don't see much value in a more specific error code here. An
application will likely bail the current operation and the end-user will
have to look into the extack to pick enough details on what was possibly
wrong.

/P

Re: [PATCH net-next] tc: Return an error if filters try to attach too many actions

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed,  9 Apr 2025 16:55:23 +0200 you wrote:
> While developing the fix for the buffer sizing issue in [0], I noticed
> that the kernel will happily accept a long list of actions for a filter,
> and then just silently truncate that list down to a maximum of 32
> actions.
> 
> That seems less than ideal, so this patch changes the action parsing to
> return an error message and refuse to create the filter in this case.
> This results in an error like:
> 
> [...]

Here is the summary with links:
  - [net-next] tc: Return an error if filters try to attach too many actions
    https://git.kernel.org/netdev/net-next/c/5f5f92912b43

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html