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

[RFC PATCH net] 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.

Sending as an RFC as this is obviously a change in UAPI. But seeing as
creating more than 32 filters has never actually *worked*, it could be
argued that the change is not likely to break any existing workflows.
But, well, OTOH: https://xkcd.com/1172/

So what do people think? Worth the risk for saner behaviour?

[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: [RFC PATCH net] tc: Return an error if filters try to attach too many actions

[Jiri Pirko]

Mon, Apr 07, 2025 at 01:29:23PM +0200, toke@redhat.com 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.
>
>Sending as an RFC as this is obviously a change in UAPI. But seeing as
>creating more than 32 filters has never actually *worked*, it could be
>argued that the change is not likely to break any existing workflows.
>But, well, OTOH: https://xkcd.com/1172/
>
>So what do people think? Worth the risk for saner behaviour?

I vote "yes".

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

[Jamal Hadi Salim]

On Mon, Apr 7, 2025 at 7:29 AM Toke Høiland-Jørgensen <toke@redhat.com> 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.
>
> Sending as an RFC as this is obviously a change in UAPI. But seeing as
> creating more than 32 filters has never actually *worked*, it could be
> argued that the change is not likely to break any existing workflows.
> But, well, OTOH: https://xkcd.com/1172/
>
> So what do people think? Worth the risk for saner behaviour?
>

I dont know anyone using that many actions per filter, but given it's
a uapi i am more inclined to keep it.
How about just removing the "return -EINVAL" then it becomes a
warning? It would need a 2-3 line change to iproute2 to recognize the
extack with positive ACK from the kernel.

cheers,
jamal


> [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: [RFC PATCH net] tc: Return an error if filters try to attach too many actions

[Jamal Hadi Salim]

On Mon, Apr 7, 2025 at 3:56 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Mon, Apr 7, 2025 at 7:29 AM Toke Høiland-Jørgensen <toke@redhat.com> 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.
> >
> > Sending as an RFC as this is obviously a change in UAPI. But seeing as
> > creating more than 32 filters has never actually *worked*, it could be
> > argued that the change is not likely to break any existing workflows.
> > But, well, OTOH: https://xkcd.com/1172/
> >
> > So what do people think? Worth the risk for saner behaviour?
> >
>
> I dont know anyone using that many actions per filter, but given it's
> a uapi i am more inclined to keep it.
> How about just removing the "return -EINVAL" then it becomes a
> warning? It would need a 2-3 line change to iproute2 to recognize the
> extack with positive ACK from the kernel.
>

Removing the return -EINVAL:

$tc actions add `for i in $(seq 33); do echo action gact ok; done`
Warning: Only 32 actions supported per filter.

We do have a tdc testcase which adds 32 actions and verifies, we can
add another one which will be something like above....

cheers,
jamal

> cheers,
> jamal
>
>
> > [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: [RFC PATCH net] tc: Return an error if filters try to attach too many actions

[Jamal Hadi Salim]

On Mon, Apr 7, 2025 at 4:08 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Mon, Apr 7, 2025 at 3:56 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >
> > On Mon, Apr 7, 2025 at 7:29 AM Toke Høiland-Jørgensen <toke@redhat.com> 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.
> > >
> > > Sending as an RFC as this is obviously a change in UAPI. But seeing as
> > > creating more than 32 filters has never actually *worked*, it could be
> > > argued that the change is not likely to break any existing workflows.
> > > But, well, OTOH: https://xkcd.com/1172/
> > >
> > > So what do people think? Worth the risk for saner behaviour?
> > >
> >
> > I dont know anyone using that many actions per filter, but given it's
> > a uapi i am more inclined to keep it.
> > How about just removing the "return -EINVAL" then it becomes a
> > warning? It would need a 2-3 line change to iproute2 to recognize the
> > extack with positive ACK from the kernel.
> >
>
> Removing the return -EINVAL:
>
> $tc actions add `for i in $(seq 33); do echo action gact ok; done`
> Warning: Only 32 actions supported per filter.
>
> We do have a tdc testcase which adds 32 actions and verifies, we can
> add another one which will be something like above....
>

And using your example:

$TC -echo filter add dev veth0 parent 1: u32 match u32 0 0 $(for i in
$(seq 33); do echo action gact ok; done)
Warning: Only 32 actions supported.
Not a filter(cmd 2)

cheers,
jamal

>
> > cheers,
> > jamal
> >
> >
> > > [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: [RFC PATCH net] tc: Return an error if filters try to attach too many actions

[Jamal Hadi Salim]

On Mon, Apr 7, 2025 at 4:10 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> On Mon, Apr 7, 2025 at 4:08 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> >
> > On Mon, Apr 7, 2025 at 3:56 PM Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> > >
> > > On Mon, Apr 7, 2025 at 7:29 AM Toke Høiland-Jørgensen <toke@redhat.com> 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.
> > > >
> > > > Sending as an RFC as this is obviously a change in UAPI. But seeing as
> > > > creating more than 32 filters has never actually *worked*, it could be
> > > > argued that the change is not likely to break any existing workflows.
> > > > But, well, OTOH: https://xkcd.com/1172/
> > > >
> > > > So what do people think? Worth the risk for saner behaviour?
> > > >
> > >
> > > I dont know anyone using that many actions per filter, but given it's
> > > a uapi i am more inclined to keep it.
> > > How about just removing the "return -EINVAL" then it becomes a
> > > warning? It would need a 2-3 line change to iproute2 to recognize the
> > > extack with positive ACK from the kernel.
> > >
> >
> > Removing the return -EINVAL:
> >
> > $tc actions add `for i in $(seq 33); do echo action gact ok; done`
> > Warning: Only 32 actions supported per filter.
> >
> > We do have a tdc testcase which adds 32 actions and verifies, we can
> > add another one which will be something like above....
> >
>
> And using your example:
>
> $TC -echo filter add dev veth0 parent 1: u32 match u32 0 0 $(for i in
> $(seq 33); do echo action gact ok; done)
> Warning: Only 32 actions supported.
> Not a filter(cmd 2)

Sorry the "Not a filter(cmd 2)" should not be showing up.
The "Only 32 actions supported" you see was a quick hack to your
extack "Only 32 actions supported per filter"  because the issue
occurs whether you have a filter with > 32 actions or instantiating a
batch of > 32 actions.

cheers,
jamal

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

[Ilya Maximets]

On 4/7/25 9:56 PM, Jamal Hadi Salim wrote:
> On Mon, Apr 7, 2025 at 7:29 AM Toke Høiland-Jørgensen <toke@redhat.com> 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.
>>
>> Sending as an RFC as this is obviously a change in UAPI. But seeing as
>> creating more than 32 filters has never actually *worked*, it could be
>> argued that the change is not likely to break any existing workflows.
>> But, well, OTOH: https://xkcd.com/1172/
>>
>> So what do people think? Worth the risk for saner behaviour?
>>
> 
> I dont know anyone using that many actions per filter, but given it's
> a uapi i am more inclined to keep it.
> How about just removing the "return -EINVAL" then it becomes a
> warning? It would need a 2-3 line change to iproute2 to recognize the
> extack with positive ACK from the kernel.

The warning is hard to act upon programmatically.  If some software is
trying to install those rules, it would expect a failure code if the
actions cannot be actually installed.  It's also not common to handle
extack in a success scenario.  Besides, TCA_ACT_MAX_PRIO itself is part
of uAPI, it makes sense to be that violation of this limit should cause
a failure.  Truncating the chain may cause unexpected consequences for
the user, i.e. traffic handled incorrectly, unless the user happened
to parse extack, which is not really machine-readable.

Throughout the years we've been adding extra validation to various parts
of tc, and these would also technically be uAPI changes.  I'm not sure
why this change would be any different.  In OVS we've been struggling for
a long time with various kernel inconsistencies in tc netlink API and
are forced to request ECHO for each request and compare rules we request
with what kernel actually installed [1].  This is a significant performance
and complexity burden that I hope can go away eventually.

To my knowledge, OVS doesn't hit this particular issue with the number of
actions, at least I've never seen it, but it's still a problem that the
kernel behavior is inconsistent.

So, I'd vote for adding the proper validation and allow users to detect
those failures when they happen.

Best regards, Ilya Maximets.

[1] https://github.com/openvswitch/ovs/commit/464b5b13e6d251c65b3158af5df16057243f1619

> 
> cheers,
> jamal
> 
> 
>> [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: [RFC PATCH net] tc: Return an error if filters try to attach too many actions

[Jamal Hadi Salim]

On Tue, Apr 8, 2025 at 8:14 AM Ilya Maximets <i.maximets@ovn.org> wrote:
>
> On 4/7/25 9:56 PM, Jamal Hadi Salim wrote:
> > On Mon, Apr 7, 2025 at 7:29 AM Toke Høiland-Jørgensen <toke@redhat.com> 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.
> >>
> >> Sending as an RFC as this is obviously a change in UAPI. But seeing as
> >> creating more than 32 filters has never actually *worked*, it could be
> >> argued that the change is not likely to break any existing workflows.
> >> But, well, OTOH: https://xkcd.com/1172/
> >>
> >> So what do people think? Worth the risk for saner behaviour?
> >>
> >
> > I dont know anyone using that many actions per filter, but given it's
> > a uapi i am more inclined to keep it.
> > How about just removing the "return -EINVAL" then it becomes a
> > warning? It would need a 2-3 line change to iproute2 to recognize the
> > extack with positive ACK from the kernel.
>
> The warning is hard to act upon programmatically.  If some software is
> trying to install those rules, it would expect a failure code if the
> actions cannot be actually installed.  It's also not common to handle
> extack in a success scenario.  Besides, TCA_ACT_MAX_PRIO itself is part
> of uAPI, it makes sense to be that violation of this limit should cause
> a failure.  Truncating the chain may cause unexpected consequences for
> the user, i.e. traffic handled incorrectly, unless the user happened
> to parse extack, which is not really machine-readable.
>
> Throughout the years we've been adding extra validation to various parts
> of tc, and these would also technically be uAPI changes.  I'm not sure
> why this change would be any different.  In OVS we've been struggling for
> a long time with various kernel inconsistencies in tc netlink API and
> are forced to request ECHO for each request and compare rules we request
> with what kernel actually installed [1].  This is a significant performance
> and complexity burden that I hope can go away eventually.
>

This issue aside:
My experience on building a control system as such is that it is safer
to base on the philosophy of  "trust but verify". There will always be
bugs and quarks and it is advisable to be defensive. IOW, to not
assume reliability/correctness as guaranteed. So nothing wrong with
doing the ECHO if you take an approach of  "I dont trust the system at
all and performance is secondary to correctness"

The alternative is to semi trust the system and verify when
programmatically triggered.
In this specific case I would argue that a warning is a trigger to verify.
Another good example of triggers is on events - if events get lost
netlink will set an error on the socket which should trigger a
verification.

On this specific issue: the culprit is batching and expected behavior
on failure to meet the batch "transaction" - to be specific, one of
dealing with two independent systems (filters and actions).
Unfortunately, there is no consistency in behavior across all users of
netlink because there is no way to signal transactional behavior - i.e
what should happen if some of my batch entries succeed.
Example, it is totally reasonable to say "dont worry about failures,
just go over my whole list and skip things that fail".
Decisions so far have been left to the judgement of the programmer. If
we could codify expected behavior then it would be easier to follow
some rules - but that would mean big changes on netlink (which could
be done incrementally if there is desire). See for example[1].

> To my knowledge, OVS doesn't hit this particular issue with the number of
> actions, at least I've never seen it, but it's still a problem that the
> kernel behavior is inconsistent.
>

My 2 .ca cents above.

> So, I'd vote for adding the proper validation and allow users to detect
> those failures when they happen.
>
> Best regards, Ilya Maximets.
>
> [1] https://github.com/openvswitch/ovs/commit/464b5b13e6d251c65b3158af5df16057243f1619

These seem like serious bugs on flower, no?

cheers,
jamal

[1]https://www.rfc-editor.org/rfc/rfc5810.html#section-4.3.1

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

[Jakub Kicinski]

On Mon,  7 Apr 2025 13:29:23 +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.

FWIW I also vote "yes" for hard error, rather than warning.
But net-next, not net.