netdev - Re: [PATCH v2] net: sched: check tc_skip

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: <CA+FuTSftumQMYg8fcCW3C-A0CKZC=6GYDrR3UXjaS1gNJx=5TA@mail.gmail.com>
Date:   Wed, 3 Nov 2021 10:47:42 -0400
From:   Willem de Bruijn <willemdebruijn.kernel@...il.com>
To:     xiangxia.m.yue@...il.com
Cc:     netdev@...r.kernel.org, Cong Wang <xiyou.wangcong@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>
Subject: Re: [PATCH v2] net: sched: check tc_skip_classify as far as possible

On Wed, Nov 3, 2021 at 10:32 AM <xiangxia.m.yue@...il.com> wrote:
>
> From: Tonghao Zhang <xiangxia.m.yue@...il.com>
>
> We look up and then check tc_skip_classify flag in net
> sched layer, even though skb don't want to be classified.
> That case may consume a lot of cpu cycles.
>
> Install the rules as below:
> $ for id in $(seq 1 100); do
> $       tc filter add ... egress prio $id ... action mirred egress redirect dev ifb0
> $ done
>
> netperf:
> $ taskset -c 1 netperf -t TCP_RR -H ip -- -r 32,32
> $ taskset -c 1 netperf -t TCP_STREAM -H ip -- -m 32
>
> Before: 10662.33 tps, 108.95 Mbit/s
> After:  12434.48 tps, 145.89 Mbit/s
>
> For TCP_RR, there are 16.6% improvement, TCP_STREAM 33.9%.
>
> Cc: Willem de Bruijn <willemb@...gle.com>
> Cc: Cong Wang <xiyou.wangcong@...il.com>
> Cc: Jakub Kicinski <kuba@...nel.org>
> Cc: Daniel Borkmann <daniel@...earbox.net>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@...il.com>
> ---
> v2: don't delete skb_skip_tc_classify in act_api
> ---
>  net/core/dev.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index edeb811c454e..fc29a429e9ad 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -3940,6 +3940,9 @@ sch_handle_egress(struct sk_buff *skb, int *ret, struct net_device *dev)
>         if (!miniq)
>                 return skb;
>
> +       if (skb_skip_tc_classify(skb))
> +               return skb;
> +

This bit and test exist to make sure that packets redirected through
the ifb device are redirected only once.

I was afraid that on second loop, this will also short-circuit other
unrelated tc classifiers and actions that should take place.

The name and the variable comment make clear that the intention is
indeed to bypass all classification.

However, the current implementation acts at tcf_action_exec. So it
does not stop processing by fused classifier-action objects, notably tc_bpf.

So I agree both that this seems to follow the original intent, but also
has the chance to break existing production configurations.


>         /* qdisc_skb_cb(skb)->pkt_len was already set by the caller. */
>         qdisc_skb_cb(skb)->mru = 0;
>         qdisc_skb_cb(skb)->post_ct = false;
> --
> 2.27.0
>