netdev - Re: [PATCH bpf-next v3 1/5] selftests/bpf: generalize helpers to control background listener

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:   Thu, 7 May 2020 17:56:15 +0200
From:   Jakub Sitnicki <jakub@...udflare.com>
To:     Martin KaFai Lau <kafai@...com>,
        Stanislav Fomichev <sdf@...gle.com>
Cc:     <netdev@...r.kernel.org>, <bpf@...r.kernel.org>,
        <davem@...emloft.net>, <ast@...nel.org>, <daniel@...earbox.net>,
        Andrey Ignatov <rdna@...com>
Subject: Re: [PATCH bpf-next v3 1/5] selftests/bpf: generalize helpers to
 control background listener

On Wed, 6 May 2020 23:09:21 -0700
Martin KaFai Lau <kafai@...com> wrote:

> On Wed, May 06, 2020 at 03:32:06PM -0700, Stanislav Fomichev wrote:
> > Move the following routines that let us start a background listener
> > thread and connect to a server by fd to the test_prog:
> > * start_server_thread - start background INADDR_ANY thread
> > * stop_server_thread - stop the thread
> > * connect_to_fd - connect to the server identified by fd
> > 
> > These will be used in the next commit.
> > 
> > Also, extend these helpers to support AF_INET6 and accept the family
> > as an argument.
> > 
> > v3:
> > * export extra helper to start server without a thread (Martin KaFai Lau)
> > 
> > v2:
> > * put helpers into network_helpers.c (Andrii Nakryiko)
> > 
> > Cc: Andrey Ignatov <rdna@...com>
> > Cc: Martin KaFai Lau <kafai@...com>
> > Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> > ---

Thanks for extracting network_helpers. I've seen network syscall
wrappers repeat between the tests I've touched. Will be super useful to
have it in one place.

[...]

> I still don't see a thread is needed in this existing test_tcp_rtt also.
> 
> I was hoping the start/stop_server_thread() helpers can be removed.
> I am not positive the future tests will find start/stop_server_thread()
> useful as is because it only does accept() and there is no easy way to
> get the accept-ed() fd.
> 
> If this test needs to keep the thread, may be only keep the
> start/stop_server_thread() in this test for now until
> there is another use case that can benefit from them.
> 
> Keep the start_server() and connect_to_fd() in the new
> network_helpers.c.  I think at least sk_assign.c (and likely
> a few others) should be able to reuse them later.  They
> are doing something very close to start_server() and
> connect_to_fd().
> 
> Thoughts?

IMHO starting a thread to accept() a connection and close it is an
overkill. I wouldn't spawn a thread if I can get away with interleaving
client and server operations on sockets on the main thread. Makes the
test code easier to follow, and strace, for me.