Message-ID: <20a20b0b-237c-42d2-8d17-a07ec87347c1@ovn.org>
Date: Tue, 8 Apr 2025 14:14:26 +0200
From: Ilya Maximets <i.maximets@....org>
To: Jamal Hadi Salim <jhs@...atatu.com>,
 Toke Høiland-Jørgensen <toke@...hat.com>
Cc: Cong Wang <xiyou.wangcong@...il.com>, Jiri Pirko <jiri@...nulli.us>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Simon Horman <horms@...nel.org>, netdev@...r.kernel.org, i.maximets@....org
Subject: Re: [RFC PATCH net] tc: Return an error if filters try to attach too
 many actions

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@...hat.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@...hat.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
>>

Powered by blists - more mailing lists