| 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
| ||
|
Message-ID: <20221222235432.96615-1-kuniyu@amazon.com>
Date: Fri, 23 Dec 2022 08:54:32 +0900
From: Kuniyuki Iwashima <kuniyu@...zon.com>
To: <joannelkoong@...il.com>
CC: <davem@...emloft.net>, <edumazet@...gle.com>,
<jirislaby@...nel.org>, <kuba@...nel.org>, <kuni1840@...il.com>,
<kuniyu@...zon.com>, <netdev@...r.kernel.org>, <pabeni@...hat.com>
Subject: Re: [PATCH RFC net 2/2] tcp: Add selftest for bind() and TIME_WAIT.
From: Joanne Koong <joannelkoong@...il.com>
Date: Thu, 22 Dec 2022 13:41:11 -0800
> On Wed, Dec 21, 2022 at 7:14 AM Kuniyuki Iwashima <kuniyu@...zon.com> wrote:
> >
> > bhash2 split the bind() validation logic into wildcard and non-wildcard
> > cases. Let's add a test to catch the same regression.
> >
> > Before the previous patch:
> >
> > # ./bind_timewait
> > TAP version 13
> > 1..2
> > # Starting 2 tests from 3 test cases.
> > # RUN bind_timewait.localhost.1 ...
> > # bind_timewait.c:87:1:Expected ret (0) == -1 (-1)
> > # 1: Test terminated by assertion
> > # FAIL bind_timewait.localhost.1
> > not ok 1 bind_timewait.localhost.1
> > # RUN bind_timewait.addrany.1 ...
> > # OK bind_timewait.addrany.1
> > ok 2 bind_timewait.addrany.1
> > # FAILED: 1 / 2 tests passed.
> > # Totals: pass:1 fail:1 xfail:0 xpass:0 skip:0 error:0
> >
> > After:
> >
> > # ./bind_timewait
> > TAP version 13
> > 1..2
> > # Starting 2 tests from 3 test cases.
> > # RUN bind_timewait.localhost.1 ...
> > # OK bind_timewait.localhost.1
> > ok 1 bind_timewait.localhost.1
> > # RUN bind_timewait.addrany.1 ...
> > # OK bind_timewait.addrany.1
> > ok 2 bind_timewait.addrany.1
> > # PASSED: 2 / 2 tests passed.
> > # Totals: pass:2 fail:0 xfail:0 xpass:0 skip:0 error:0
> >
> > Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.com>
> > ---
> > tools/testing/selftests/net/.gitignore | 1 +
> > tools/testing/selftests/net/bind_timewait.c | 93 +++++++++++++++++++++
> > 2 files changed, 94 insertions(+)
> > create mode 100644 tools/testing/selftests/net/bind_timewait.c
> >
> > diff --git a/tools/testing/selftests/net/.gitignore b/tools/testing/selftests/net/.gitignore
> > index 9cc84114741d..a6911cae368c 100644
> > --- a/tools/testing/selftests/net/.gitignore
> > +++ b/tools/testing/selftests/net/.gitignore
> > @@ -1,5 +1,6 @@
> > # SPDX-License-Identifier: GPL-2.0-only
> > bind_bhash
> > +bind_timewait
> > csum
> > cmsg_sender
> > diag_uid
> > diff --git a/tools/testing/selftests/net/bind_timewait.c b/tools/testing/selftests/net/bind_timewait.c
> > new file mode 100644
> > index 000000000000..2d40403128ff
> > --- /dev/null
> > +++ b/tools/testing/selftests/net/bind_timewait.c
> > @@ -0,0 +1,93 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright Amazon.com Inc. or its affiliates. */
> > +
> > +#include "../kselftest_harness.h"
>
> nit: Not sure if this matters or not, but from looking at the other
> selftests/net it seems like the convention is to have relative path
> #include defined below absolute path #includes.
Will fix.
>
> > +
> > +#include <sys/socket.h>
> > +#include <netinet/in.h>
> > +#include <netinet/tcp.h>
>
> nit: i don't think we need this netinet/tcp.h include
Good catch, I was seeing an old man page.
>
> > +
> > +FIXTURE(bind_timewait)
> > +{
> > + struct sockaddr_in addr;
> > + socklen_t addrlen;
> > +};
> > +
> > +FIXTURE_VARIANT(bind_timewait)
> > +{
> > + __u32 addr_const;
> > +};
> > +
> > +FIXTURE_VARIANT_ADD(bind_timewait, localhost)
> > +{
> > + .addr_const = INADDR_LOOPBACK
> > +};
> > +
> > +FIXTURE_VARIANT_ADD(bind_timewait, addrany)
> > +{
> > + .addr_const = INADDR_ANY
> > +};
> > +
> > +FIXTURE_SETUP(bind_timewait)
> > +{
> > + self->addr.sin_family = AF_INET;
> > + self->addr.sin_port = 0;
> > + self->addr.sin_addr.s_addr = htonl(variant->addr_const);
> > + self->addrlen = sizeof(self->addr);
> > +}
> > +
> > +FIXTURE_TEARDOWN(bind_timewait)
> > +{
> > +}
> > +
> > +void create_timewait_socket(struct __test_metadata *_metadata,
> > + FIXTURE_DATA(bind_timewait) *self)
> > +{
> > + int server_fd, client_fd, child_fd, ret;
> > + struct sockaddr_in addr;
> > + socklen_t addrlen;
> > +
> > + server_fd = socket(AF_INET, SOCK_STREAM, 0);
> > + ASSERT_GT(server_fd, 0);
>
> If any of these assertions fail, do we leak fds because we don't get
> to calling the close()s at the end of this function? Do we need to
> have the fds cleaned up in the teardown fixture function?
I think exit() cleans up fds in the case. IIUC, the parent process
catches SIGABRT in __wait_for_test(), but the child does not call
FIXTURE_TEARDOWN().
Thank you!
>
> > +
> > + ret = bind(server_fd, (struct sockaddr *)&self->addr, self->addrlen);
> > + ASSERT_EQ(ret, 0);
> > +
> > + ret = listen(server_fd, 1);
> > + ASSERT_EQ(ret, 0);
> > +
> > + ret = getsockname(server_fd, (struct sockaddr *)&self->addr, &self->addrlen);
> > + ASSERT_EQ(ret, 0);
> > +
> > + client_fd = socket(AF_INET, SOCK_STREAM, 0);
> > + ASSERT_GT(client_fd, 0);
> > +
> > + ret = connect(client_fd, (struct sockaddr *)&self->addr, self->addrlen);
> > + ASSERT_EQ(ret, 0);
> > +
> > + addrlen = sizeof(addr);
> > + child_fd = accept(server_fd, (struct sockaddr *)&addr, &addrlen);
> > + ASSERT_GT(child_fd, 0);
> > +
> > + close(child_fd);
> > + close(client_fd);
> > + close(server_fd);
> > +}
> > +
> > +TEST_F(bind_timewait, 1)
> > +{
> > + int fd, ret;
> > +
> > + create_timewait_socket(_metadata, self);
> > +
> > + fd = socket(AF_INET, SOCK_STREAM, 0);
> > + ASSERT_GT(fd, 0);
> > +
> > + ret = bind(fd, (struct sockaddr *)&self->addr, self->addrlen);
> > + ASSERT_EQ(ret, -1);
> > + ASSERT_EQ(errno, EADDRINUSE);
> > +
> > + close(fd);
> > +}
> > +
> > +TEST_HARNESS_MAIN
> > --
> > 2.30.2
Powered by blists - more mailing lists