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: <CAM0EoM=a=MuV5BOrPbFmkJa_5aYeDwk49mRXtVncwLwA_a8uwQ@mail.gmail.com>
Date: Mon, 7 Apr 2025 16:10:34 -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: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)

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