| 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: <565914cb-1188-424c-b8ee-16739f350ddb@yahoo.com>
Date: Fri, 24 Oct 2025 10:18:13 +0100
From: Sunday Adelodun <adelodunolaoluwa@...oo.com>
To: Kuniyuki Iwashima <kuniyu@...gle.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, shuah@...nel.org, horms@...nel.org,
linux-kselftest@...r.kernel.org, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, skhan@...uxfoundation.org,
david.hunter.linux@...il.com
Subject: Re: [PATCH] selftests/unix: Add test for ECONNRESET and EOF behaviour
On 10/24/25 04:00, Kuniyuki Iwashima wrote:
> Thanks for adding tests.
>
>> [PATCH] selftests/unix: Add test for ECONNRESET and EOF behaviour
> nit: The common prefix is "selftest: af_unix:".
>
>
> On Thu, Oct 23, 2025 at 9:59 AM Sunday Adelodun
> <adelodunolaoluwa@...oo.com> wrote:
>> Add selftests verifying the EOF and ECONNRESET behaviour of
>> UNIX domain sockets (SOCK_STREAM and SOCK_DGRAM). The tests document
>> Linux's semantics and clarify the long-standing differences with BSD.
>>
>> Suggested-by: Kuniyuki Iwashima <kuniyu@...gle.com>
>> Signed-off-by: Sunday Adelodun <adelodunolaoluwa@...oo.com>
>> ---
>> tools/testing/selftests/net/unix/Makefile | 5 +
>> .../selftests/net/unix/test_unix_connreset.c | 147 ++++++++++++++++++
>> 2 files changed, 152 insertions(+)
>> create mode 100644 tools/testing/selftests/net/unix/Makefile
>> create mode 100644 tools/testing/selftests/net/unix/test_unix_connreset.c
> The test for af_unix is placed under tools/testing/selftests/net/af_unix.
>
>
>> diff --git a/tools/testing/selftests/net/unix/Makefile b/tools/testing/selftests/net/unix/Makefile
>> new file mode 100644
>> index 000000000000..a52992ba23d9
>> --- /dev/null
>> +++ b/tools/testing/selftests/net/unix/Makefile
>> @@ -0,0 +1,5 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +TEST_GEN_PROGS := test_unix_connreset
>> +
>> +include ../../lib.mk
>> +
>> diff --git a/tools/testing/selftests/net/unix/test_unix_connreset.c b/tools/testing/selftests/net/unix/test_unix_connreset.c
>> new file mode 100644
>> index 000000000000..a8720c7565cb
>> --- /dev/null
>> +++ b/tools/testing/selftests/net/unix/test_unix_connreset.c
>> @@ -0,0 +1,147 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Selftest for UNIX socket close and ECONNRESET behaviour.
> nit: s/UNIX/AF_UNIX/
>
>> + *
>> + * This test verifies that:
>> + * 1. SOCK_STREAM sockets return EOF when peer closes normally.
>> + * 2. SOCK_STREAM sockets return ECONNRESET if peer closes with unread data.
>> + * 3. SOCK_DGRAM sockets do not return ECONNRESET when peer closes,
>> + * unlike BSD where this error is observed.
>> + *
>> + * These tests document the intended Linux behaviour, distinguishing it from BSD.
> I'd not mention BSD as it could be outdated again.
>
>
>> + *
>> + */
>> +
>> +#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/test_unix_connreset.sock"
>> +
>> +static void remove_socket_file(void)
>> +{
>> + unlink(SOCK_PATH);
>> +}
>> +
>> +/* Test 1: peer closes normally */
>> +TEST(stream_eof)
> I think most of the code can be shared by defining
> FIXTURE_VARIANT().
>
> e.g. variant->unread_data can consolidate Test 1&2.
>
>
>> +{
>> + int server, client, child;
>> + struct sockaddr_un addr = {0};
>> + char buf[16] = {0};
> IIRC, {0} only initialises the first entry and we had a problem
> in kernel code, so simply use "= {};" everywhere.
>
>
>> + ssize_t n;
>> +
>> + server = socket(AF_UNIX, SOCK_STREAM, 0);
> Try using variant->type for SOCK_STREAM,
> SOCK_DGRAM, and SOCK_SEQPACKET.
>
> See unix_connect.c, or you could reuse the fixtures
> of err == 0 there.
>
>> + ASSERT_GE(server, 0);
> nit: The 1st arg is "expected", and the 2nd is "seen",
> so ASSERT_NE(-1, server) (or ASSERT_LT(-1, server)).
>
> Same for other places.
>
>
>> +
>> + addr.sun_family = AF_UNIX;
>> + strcpy(addr.sun_path, SOCK_PATH);
>> + remove_socket_file();
>> +
>> + ASSERT_EQ(bind(server, (struct sockaddr *)&addr, sizeof(addr)), 0);
> I personally feel easy to read this style:
>
> err = bind();
> ASSERT_EQ(0, err);
>
>> + ASSERT_EQ(listen(server, 1), 0);
>> +
>> + client = socket(AF_UNIX, SOCK_STREAM, 0);
>> + ASSERT_GE(client, 0);
>> + ASSERT_EQ(connect(client, (struct sockaddr *)&addr, sizeof(addr)), 0);
>> +
>> + child = accept(server, NULL, NULL);
>> + ASSERT_GE(child, 0);
>> +
>> + /* Peer closes normally */
>> + close(child);
>> +
>> + n = recv(client, buf, sizeof(buf), 0);
>> + EXPECT_EQ(n, 0);
>> + TH_LOG("recv=%zd errno=%d (%s)", n, errno, strerror(errno));
> I printed errno just for visibility, and you can simply use
> ASSERT here too like
>
> if (n == -1)
> ASSERT_EQ(ECONNRESET, errno)
>
> (I'm assuming Test 1 & 2 will share most code)
>
>> +
>> + close(client);
>> + close(server);
>> + remove_socket_file();
> This will not be executed if the program fails at ASSERT_XX(),
> so move it to FIXTURE_TEARDOWN().
>
>
>
>> +}
>> +
>> +/* Test 2: peer closes with unread data */
>> +TEST(stream_reset_unread)
>> +{
>> + int server, client, child;
>> + struct sockaddr_un addr = {0};
>> + char buf[16] = {0};
>> + ssize_t n;
>> +
>> + server = socket(AF_UNIX, SOCK_STREAM, 0);
>> + ASSERT_GE(server, 0);
>> +
>> + addr.sun_family = AF_UNIX;
>> + strcpy(addr.sun_path, SOCK_PATH);
>> + remove_socket_file();
>> +
>> + ASSERT_EQ(bind(server, (struct sockaddr *)&addr, sizeof(addr)), 0);
>> + ASSERT_EQ(listen(server, 1), 0);
>> +
>> + client = socket(AF_UNIX, SOCK_STREAM, 0);
>> + ASSERT_GE(client, 0);
>> + ASSERT_EQ(connect(client, (struct sockaddr *)&addr, sizeof(addr)), 0);
>> +
>> + child = accept(server, NULL, NULL);
>> + ASSERT_GE(child, 0);
>> +
>> + /* Send data that will remain unread by client */
>> + send(client, "hello", 5, 0);
>> + close(child);
>> +
>> + n = recv(client, buf, sizeof(buf), 0);
>> + EXPECT_LT(n, 0);
>> + EXPECT_EQ(errno, ECONNRESET);
>> + TH_LOG("recv=%zd errno=%d (%s)", n, errno, strerror(errno));
>> +
>> + close(client);
>> + close(server);
>> + remove_socket_file();
>> +}
>> +
>> +/* Test 3: SOCK_DGRAM peer close */
>> +TEST(dgram_reset)
>> +{
>> + int server, client;
>> + int flags;
>> + struct sockaddr_un addr = {0};
>> + char buf[16] = {0};
>> + ssize_t n;
>> +
>> + server = socket(AF_UNIX, SOCK_DGRAM, 0);
>> + ASSERT_GE(server, 0);
>> +
>> + addr.sun_family = AF_UNIX;
>> + strcpy(addr.sun_path, SOCK_PATH);
>> + remove_socket_file();
>> +
>> + ASSERT_EQ(bind(server, (struct sockaddr *)&addr, sizeof(addr)), 0);
>> +
>> + client = socket(AF_UNIX, SOCK_DGRAM, 0);
>> + ASSERT_GE(client, 0);
>> + ASSERT_EQ(connect(client, (struct sockaddr *)&addr, sizeof(addr)), 0);
>> +
>> + send(client, "hello", 5, 0);
>> + close(server);
>> +
>> + flags = fcntl(client, F_GETFL, 0);
>> + fcntl(client, F_SETFL, flags | O_NONBLOCK);
> You can save fcntl() with socket(..., ... | SOCK_NONBLOCK, ...).
>
>
>> +
>> + n = recv(client, buf, sizeof(buf), 0);
>> + TH_LOG("recv=%zd errno=%d (%s)", n, errno, strerror(errno));
>> + /* Expect EAGAIN or EWOULDBLOCK because there is no datagram and peer is closed. */
>> + EXPECT_LT(n, 0);
>> + EXPECT_TRUE(errno == EAGAIN);
>> +
>> + close(client);
>> + remove_socket_file();
>> +}
>> +
>> +TEST_HARNESS_MAIN
>> +
>> --
>> 2.43.0
>>
Hi Kuniyuki,
Thank you very much for the detailed review and helpful suggestions.
I will send a v2 patch incorporating these changes.
Thanks again for taking the time to review and explain the details, I
really appreciate it.
Best regards,
Sunday Adelodun
Powered by blists - more mailing lists