Message-ID: <CAM0EoM=ntVF3JOdi9DgCrDsdoqW_dxF2bRaQPvTVcozNoobVfA@mail.gmail.com>
Date: Tue, 8 Apr 2025 09:42:02 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Ilya Maximets <i.maximets@....org>
Cc: Toke Høiland-Jørgensen <toke@...hat.com>, 
	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
Subject: Re: [RFC PATCH net] tc: Return an error if filters try to attach too
 many actions

On Tue, Apr 8, 2025 at 8:14 AM Ilya Maximets <i.maximets@....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@...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.
>

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

Powered by blists - more mailing lists