netdev - RE: [PATCH bpf-next v3 4/5] bpf: Add helpers to issue and check SYN cookies in XDP

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] [day] [month] [year] [list]

Date:   Mon, 14 Mar 2022 17:49:27 +0000
From:   Maxim Mikityanskiy <maximmi@...dia.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
CC:     "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Tariq Toukan <tariqt@...dia.com>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Petar Penkov <ppenkov@...gle.com>,
        Lorenz Bauer <lmb@...udflare.com>,
        Eric Dumazet <edumazet@...gle.com>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        David Ahern <dsahern@...nel.org>,
        Shuah Khan <shuah@...nel.org>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        Nathan Chancellor <nathan@...nel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Joe Stringer <joe@...ium.io>,
        Florent Revest <revest@...omium.org>,
        "linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>,
        Toke Høiland-Jørgensen <toke@...e.dk>,
        Kumar Kartikeya Dwivedi <memxor@...il.com>,
        Florian Westphal <fw@...len.de>
Subject: RE: [PATCH bpf-next v3 4/5] bpf: Add helpers to issue and check SYN
 cookies in XDP

> -----Original Message-----
> From: Alexei Starovoitov <alexei.starovoitov@...il.com>
> 
> On Fri, Mar 11, 2022 at 8:36 AM Maxim Mikityanskiy <maximmi@...dia.com>
> wrote:
> >
> > > -----Original Message-----
> > > From: Alexei Starovoitov <alexei.starovoitov@...il.com>
> > > Sent: 27 February, 2022 05:25
> > >
> > > On Thu, Feb 24, 2022 at 05:11:44PM +0200, Maxim Mikityanskiy wrote:
> > > > @@ -7798,6 +7916,14 @@ xdp_func_proto(enum bpf_func_id func_id, const
> > > struct bpf_prog *prog)
> > > >             return &bpf_tcp_check_syncookie_proto;
> > > >     case BPF_FUNC_tcp_gen_syncookie:
> > > >             return &bpf_tcp_gen_syncookie_proto;
> > > > +   case BPF_FUNC_tcp_raw_gen_syncookie_ipv4:
> > > > +           return &bpf_tcp_raw_gen_syncookie_ipv4_proto;
> > > > +   case BPF_FUNC_tcp_raw_gen_syncookie_ipv6:
> > > > +           return &bpf_tcp_raw_gen_syncookie_ipv6_proto;
> > > > +   case BPF_FUNC_tcp_raw_check_syncookie_ipv4:
> > > > +           return &bpf_tcp_raw_check_syncookie_ipv4_proto;
> > > > +   case BPF_FUNC_tcp_raw_check_syncookie_ipv6:
> > > > +           return &bpf_tcp_raw_check_syncookie_ipv6_proto;
> > > >  #endif
> > >
> > > I understand that the main use case for new helpers is XDP specific,
> > > but why limit them to XDP?
> > > The feature looks generic and applicable to skb too.
> >
> > That sounds like an extra feature, rather than a limitation. That's out
> > of scope of what I planned to do.
> >
> > Besides, it sounds kind of useless to me, because the intention of the
> > new helpers is to accelerate synproxy, and I doubt BPF over SKBs will
> > accelerate anything. Maybe someone else has another use case for these
> > helpers and SKBs - in that case I leave the opportunity to add this
> > feature up to them.
> 
> This patchset will not be accepted until the feature is generalized
> to both xdp and skb and tested for both.
> "I dont have a use case for it" is not an excuse to narrow down the scope.

I'm not narrowing down the scope. It was defined from the very
beginning: accelerating synproxy with XDP. Asking me to add SKB support
is extending the scope and making me develop extra things.

It's a well-known fact that features aren't added to the kernel if there
is no user. Sometimes, features without users (or with few users) are
even removed. Could you elaborate why you require me to develop a
feature that no one is going to use?

In my opinion, the whole review process of this series has many
unhealthy points:

1. I'm constantly requested to increase the scope.

1.1. Split the helpers not related to the feature.

1.2. Add new functionality to the verifier to accept memory regions of
fixed size.

1.3. Add support for SKB.

2. I'm requested to rewrite the same code multiple times, every time in
a new way.

2.1. I had to rewrite the sample into a selftest, then into a different
kind of a selftest. It's not obvious that standalone selftests are
deprecated (am I blind, or is it not documented anywhere?), there are a
lot of such tests already existing. Instead of saying it from the
beginning that the test must use the test_progs runner, you waited until
I implement a standalone one.

3. I receive comments on old code that passed a few iterations of the
review.

3.1. SKB support was never part of the feature, and the series passed
two iterations, but not the third.

3.2. Standalone selftest was OK in v2, but suddenly got unacceptable in
v3.