netdev - Re: [PATCH bpf-next v7 00/11] Extend SOCKMAP/SOCKHASH to store listening sockets

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]

Date:   Sun, 23 Feb 2020 13:43:31 -0800
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Jakub Sitnicki <jakub@...udflare.com>
Cc:     Daniel Borkmann <daniel@...earbox.net>, bpf <bpf@...r.kernel.org>,
        Network Development <netdev@...r.kernel.org>,
        kernel-team <kernel-team@...udflare.com>,
        John Fastabend <john.fastabend@...il.com>,
        Lorenz Bauer <lmb@...udflare.com>, Martin Lau <kafai@...com>
Subject: Re: [PATCH bpf-next v7 00/11] Extend SOCKMAP/SOCKHASH to store
 listening sockets

On Sat, Feb 22, 2020 at 01:49:52PM +0000, Jakub Sitnicki wrote:
> Hi Alexei,
> 
> On Sat, Feb 22, 2020 at 12:47 AM GMT, Alexei Starovoitov wrote:
> > On Fri, Feb 21, 2020 at 1:41 PM Daniel Borkmann <daniel@...earbox.net> wrote:
> >>
> >> On 2/18/20 6:10 PM, Jakub Sitnicki wrote:
> >> > This patch set turns SOCK{MAP,HASH} into generic collections for TCP
> >> > sockets, both listening and established. Adding support for listening
> >> > sockets enables us to use these BPF map types with reuseport BPF programs.
> >> >
> >> > Why? SOCKMAP and SOCKHASH, in comparison to REUSEPORT_SOCKARRAY, allow the
> >> > socket to be in more than one map at the same time.
> >> >
> >> > Having a BPF map type that can hold listening sockets, and gracefully
> >> > co-exist with reuseport BPF is important if, in the future, we want
> >> > BPF programs that run at socket lookup time [0]. Cover letter for v1 of
> >> > this series tells the full story of how we got here [1].
> >> >
> >> > Although SOCK{MAP,HASH} are not a drop-in replacement for SOCKARRAY just
> >> > yet, because UDP support is lacking, it's a step in this direction. We're
> >> > working with Lorenz on extending SOCK{MAP,HASH} to hold UDP sockets, and
> >> > expect to post RFC series for sockmap + UDP in the near future.
> >> >
> >> > I've dropped Acks from all patches that have been touched since v6.
> >> >
> >> > The audit for missing READ_ONCE annotations for access to sk_prot is
> >> > ongoing. Thus far I've found one location specific to TCP listening sockets
> >> > that needed annotating. This got fixed it in this iteration. I wonder if
> >> > sparse checker could be put to work to identify places where we have
> >> > sk_prot access while not holding sk_lock...
> >> >
> >> > The patch series depends on another one, posted earlier [2], that has been
> >> > split out of it.
> >> >
> >> > Thanks,
> >> > jkbs
> >> >
> >> > [0] https://lore.kernel.org/bpf/20190828072250.29828-1-jakub@cloudflare.com/
> >> > [1] https://lore.kernel.org/bpf/20191123110751.6729-1-jakub@cloudflare.com/
> >> > [2] https://lore.kernel.org/bpf/20200217121530.754315-1-jakub@cloudflare.com/
> >> >
> >> > v6 -> v7:
> >> >
> >> > - Extended the series to cover SOCKHASH. (patches 4-8, 10-11) (John)
> >> >
> >> > - Rebased onto recent bpf-next. Resolved conflicts in recent fixes to
> >> >    sk_state checks on sockmap/sockhash update path. (patch 4)
> >> >
> >> > - Added missing READ_ONCE annotation in sock_copy. (patch 1)
> >> >
> >> > - Split out patches that simplify sk_psock_restore_proto [2].
> >>
> >> Applied, thanks!
> >
> > Jakub,
> >
> > what is going on here?
> > # test_progs -n 40
> > #40 select_reuseport:OK
> > Summary: 1/126 PASSED, 30 SKIPPED, 0 FAILED
> >
> > Does it mean nothing was actually tested?
> > I really don't like to see 30 skipped tests.
> > Is it my environment?
> > If so please make them hard failures.
> > I will fix whatever I need to fix in my setup.
> 
> The UDP tests for sock{map,hash} are marked as skipped, because UDP
> support is not implemented yet. Sorry for the confusion.
> 
> Having read the recent thread about BPF selftests [0] I now realize that
> this is not the best idea. It sends the wrong signal to the developer.
> 
> I propose to exclude the UDP tests w/ sock{map,hash} by not registering
> them with test__start_subtest at all. Failing them would indicate a
> regression, which is not true. While skipping them points to a potential
> problem with the test environment, which isn't true, either.

So the tests are ready, but kernel support is missing?
Please don't run those tests then since they're guaranteed to fail atm.