| 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: <CAAVpQUADwghMj=SgdiZEErC5oy7RVpam4i9S2RwP19bA=Rbynw@mail.gmail.com>
Date: Mon, 3 Nov 2025 16:30:18 -0800
From: Kuniyuki Iwashima <kuniyu@...gle.com>
To: Sunday Adelodun <adelodunolaoluwa@...oo.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, horms@...nel.org, shuah@...nel.org,
linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
linux-kselftest@...r.kernel.org, skhan@...uxfoundation.org,
david.hunter.linux@...il.com, linux-kernel-mentees@...ts.linuxfoundation.org
Subject: Re: [PATCH v3] selftests: af_unix: Add tests for ECONNRESET and EOF semantics
On Mon, Nov 3, 2025 at 4:08 PM Sunday Adelodun
<adelodunolaoluwa@...oo.com> wrote:
>
> On 11/2/25 08:32, Kuniyuki Iwashima wrote:
> > On Sat, Nov 1, 2025 at 10:23 AM Sunday Adelodun
> > <adelodunolaoluwa@...oo.com> wrote:
> >> Add selftests to verify and document Linux’s intended behaviour for
> >> UNIX domain sockets (SOCK_STREAM and SOCK_DGRAM) when a peer closes.
> >> The tests verify that:
> >>
> >> 1. SOCK_STREAM returns EOF when the peer closes normally.
> >> 2. SOCK_STREAM returns ECONNRESET if the peer closes with unread data.
> >> 3. SOCK_SEQPACKET returns EOF when the peer closes normally.
> >> 4. SOCK_SEQPACKET returns ECONNRESET if the peer closes with unread data.
> >> 5. SOCK_DGRAM does not return ECONNRESET when the peer closes.
> >>
> >> This follows up on review feedback suggesting a selftest to clarify
> >> Linux’s semantics.
> >>
> >> Suggested-by: Kuniyuki Iwashima <kuniyu@...gle.com>
> >> Signed-off-by: Sunday Adelodun <adelodunolaoluwa@...oo.com>
> >> ---
> >> tools/testing/selftests/net/af_unix/Makefile | 1 +
> >> .../selftests/net/af_unix/unix_connreset.c | 179 ++++++++++++++++++
> >> 2 files changed, 180 insertions(+)
> >> create mode 100644 tools/testing/selftests/net/af_unix/unix_connreset.c
> >>
> >> diff --git a/tools/testing/selftests/net/af_unix/Makefile b/tools/testing/selftests/net/af_unix/Makefile
> >> index de805cbbdf69..5826a8372451 100644
> >> --- a/tools/testing/selftests/net/af_unix/Makefile
> >> +++ b/tools/testing/selftests/net/af_unix/Makefile
> >> @@ -7,6 +7,7 @@ TEST_GEN_PROGS := \
> >> scm_pidfd \
> >> scm_rights \
> >> unix_connect \
> >> + unix_connreset \
> > patchwork caught this test is not added to .gitignore.
> > https://patchwork.kernel.org/project/netdevbpf/patch/20251101172230.10179-1-adelodunolaoluwa@yahoo.com/
> >
> > Could you add it to this file ?
> >
> > tools/testing/selftests/net/.gitignore
> Oh, thank you for this. will add it
> >
> >
> >> # end of TEST_GEN_PROGS
> >>
> >> include ../../lib.mk
> >> diff --git a/tools/testing/selftests/net/af_unix/unix_connreset.c b/tools/testing/selftests/net/af_unix/unix_connreset.c
> >> new file mode 100644
> >> index 000000000000..6f43435d96e2
> >> --- /dev/null
> >> +++ b/tools/testing/selftests/net/af_unix/unix_connreset.c
> >> @@ -0,0 +1,179 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * Selftest for AF_UNIX socket close and ECONNRESET behaviour.
> >> + *
> >> + * This test verifies:
> >> + * 1. SOCK_STREAM returns EOF when the peer closes normally.
> >> + * 2. SOCK_STREAM returns ECONNRESET if peer closes with unread data.
> >> + * 3. SOCK_SEQPACKET returns EOF when the peer closes normally.
> >> + * 4. SOCK_SEQPACKET returns ECONNRESET if the peer closes with unread data.
> >> + * 5. SOCK_DGRAM does not return ECONNRESET when the peer closes.
> >> + *
> >> + * These tests document the intended Linux behaviour.
> >> + *
> >> + */
> >> +
> >> +#define _GNU_SOURCE
> >> +#include <stdlib.h>
> >> +#include <string.h>
> >> +#include <fcntl.h>
> >> +#include <unistd.h>
> >> +#include <errno.h>
> >> +#include <sys/socket.h>
> >> +#include <sys/un.h>
> >> +#include "../../kselftest_harness.h"
> >> +
> >> +#define SOCK_PATH "/tmp/af_unix_connreset.sock"
> >> +
> >> +static void remove_socket_file(void)
> >> +{
> >> + unlink(SOCK_PATH);
> >> +}
> >> +
> >> +FIXTURE(unix_sock)
> >> +{
> >> + int server;
> >> + int client;
> >> + int child;
> >> +};
> >> +
> >> +FIXTURE_VARIANT(unix_sock)
> >> +{
> >> + int socket_type;
> >> + const char *name;
> >> +};
> >> +
> >> +/* Define variants: stream and datagram */
> > nit: outdated, maybe simply remove ?
> oh..skipped me.
> will do so.
> >
> >> +FIXTURE_VARIANT_ADD(unix_sock, stream) {
> >> + .socket_type = SOCK_STREAM,
> >> + .name = "SOCK_STREAM",
> >> +};
> >> +
> >> +FIXTURE_VARIANT_ADD(unix_sock, dgram) {
> >> + .socket_type = SOCK_DGRAM,
> >> + .name = "SOCK_DGRAM",
> >> +};
> >> +
> >> +FIXTURE_VARIANT_ADD(unix_sock, seqpacket) {
> >> + .socket_type = SOCK_SEQPACKET,
> >> + .name = "SOCK_SEQPACKET",
> >> +};
> >> +
> >> +FIXTURE_SETUP(unix_sock)
> >> +{
> >> + struct sockaddr_un addr = {};
> >> + int err;
> >> +
> >> + addr.sun_family = AF_UNIX;
> >> + strcpy(addr.sun_path, SOCK_PATH);
> >> + remove_socket_file();
> >> +
> >> + self->server = socket(AF_UNIX, variant->socket_type, 0);
> >> + ASSERT_LT(-1, self->server);
> >> +
> >> + err = bind(self->server, (struct sockaddr *)&addr, sizeof(addr));
> >> + ASSERT_EQ(0, err);
> >> +
> >> + if (variant->socket_type == SOCK_STREAM ||
> >> + variant->socket_type == SOCK_SEQPACKET) {
> > patchwork caught mis-alignment here and other places.
> >
> > I'm using this for emacs, and other editors will have a similar config.
> >
> > (setq-default c-default-style "linux")
> >
> > You can check if lines are aligned properly by
> >
> > $ git show --format=email | ./scripts/checkpatch.pl
> >
> >
> >> + err = listen(self->server, 1);
> >> + ASSERT_EQ(0, err);
> >> +
> >> + self->client = socket(AF_UNIX, variant->socket_type, 0);
> > Could you add SOCK_NONBLOCK here too ?
> This is noted
> >
> >> + ASSERT_LT(-1, self->client);
> >> +
> >> + err = connect(self->client, (struct sockaddr *)&addr, sizeof(addr));
> >> + ASSERT_EQ(0, err);
> >> +
> >> + self->child = accept(self->server, NULL, NULL);
> >> + ASSERT_LT(-1, self->child);
> >> + } else {
> >> + /* Datagram: bind and connect only */
> >> + self->client = socket(AF_UNIX, SOCK_DGRAM | SOCK_NONBLOCK, 0);
> >> + ASSERT_LT(-1, self->client);
> >> +
> >> + err = connect(self->client, (struct sockaddr *)&addr, sizeof(addr));
> >> + ASSERT_EQ(0, err);
> >> + }
> >> +}
> >> +
> >> +FIXTURE_TEARDOWN(unix_sock)
> >> +{
> >> + if (variant->socket_type == SOCK_STREAM ||
> >> + variant->socket_type == SOCK_SEQPACKET)
> >> + close(self->child);
> >> +
> >> + close(self->client);
> >> + close(self->server);
> >> + remove_socket_file();
> >> +}
> >> +
> >> +/* Test 1: peer closes normally */
> >> +TEST_F(unix_sock, eof)
> >> +{
> >> + char buf[16] = {};
> >> + ssize_t n;
> >> +
> >> + /* Peer closes normally */
> >> + if (variant->socket_type == SOCK_STREAM ||
> >> + variant->socket_type == SOCK_SEQPACKET)
> >> + close(self->child);
> >> + else
> >> + close(self->server);
> >> +
> >> + n = recv(self->client, buf, sizeof(buf), 0);
> >> + TH_LOG("%s: recv=%zd errno=%d (%s)", variant->name, n, errno, strerror(errno));
> > errno is undefined if not set, and same for strerror(errno).
> >
> > Also, if ASSERT_XXX() below fails, the same information
> > (test case, errno) is logged. So, TH_LOG() seems unnecessary.
> >
> > Maybe try modifying the condition below and see how the
> > assertion is logged.
> Oh..thank you. Didn't it through that way.
> I understand.
> I will remove the TH_LOG()'s
> >
> >> + if (variant->socket_type == SOCK_STREAM ||
> >> + variant->socket_type == SOCK_SEQPACKET) {
> >> + ASSERT_EQ(0, n);
> >> + } else {
> >> + ASSERT_EQ(-1, n);
> >> + ASSERT_EQ(EAGAIN, errno);
> >> + }
> >> +}
> >> +
> >> +/* Test 2: peer closes with unread data */
> >> +TEST_F(unix_sock, reset_unread)
> >> +{
> >> + char buf[16] = {};
> >> + ssize_t n;
> >> +
> >> + /* Send data that will remain unread by client */
> >> + send(self->client, "hello", 5, 0);
> >> + close(self->child);
> >> +
> >> + n = recv(self->client, buf, sizeof(buf), 0);
> >> + TH_LOG("%s: recv=%zd errno=%d (%s)", variant->name, n, errno, strerror(errno));
> >> + if (variant->socket_type == SOCK_STREAM ||
> >> + variant->socket_type == SOCK_SEQPACKET) {
> >> + ASSERT_EQ(-1, n);
> >> + ASSERT_EQ(ECONNRESET, errno);
> >> + } else {
> >> + ASSERT_EQ(-1, n);
> >> + ASSERT_EQ(EAGAIN, errno);
> >> + }
> >> +}
> >> +
> >> +/* Test 3: SOCK_DGRAM peer close */
> >> Now Test 2 and Test 3 look identical ;)
>
> seems so, but the only difference is:
>
> close(self->child); is used in Test 2, while
> close(self->server); is used in Test 3.
> Maybe I should find a way to collapse Tests 2 and 3 (if statement might
> work)
>
> I am just afraid the tests to run will reduce to 6 from 9 and we will have 6
> cases passed as against 7 as before.
>
> What do you think?
The name of Test 2 is a bit confusing, which is not true
for SOCK_DGRAM. So, I'd use "if" to change which fd
to close() depending on the socket type.
Also, close(self->server) does nothing for SOCK_STREAM
and SOCK_SEQPACKET after accept(). Rather, that close()
should have the same effect if self->child is not accept()ed.
(In this case, Skip() for SOCK_DGRAM makes sense)
I think covering that scenario would be nicer.
If interested, you can check the test coverage with this patch.
https://lore.kernel.org/linux-kselftest/20251028024339.2028774-1-kuniyu@google.com/
Thanks!
Powered by blists - more mailing lists