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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAM0EoMmswUHjcewAESuz0jsB+qXJ+QPB6kVzjS8bzqm4niFF-Q@mail.gmail.com>
Date: Mon, 7 Apr 2025 17:40:31 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Toke Høiland-Jørgensen <toke@...hat.com>
Cc: Cong Wang <xiyou.wangcong@...il.com>, Jiri Pirko <jiri@...nulli.us>, 
	Ilya Maximets <i.maximets@...hat.com>, "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 Mon, Apr 7, 2025 at 4:10 PM Jamal Hadi Salim <jhs@...atatu.com> wrote:
>
> On Mon, Apr 7, 2025 at 4:08 PM Jamal Hadi Salim <jhs@...atatu.com> wrote:
> >
> > On Mon, Apr 7, 2025 at 3:56 PM Jamal Hadi Salim <jhs@...atatu.com> 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.
> > >
> >
> > 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