| 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: <26835ada-4e3a-4f78-8705-4ed2e3d44bd8@yahoo.com>
Date: Tue, 4 Nov 2025 11:42:47 +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, 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 11/4/25 01:30, Kuniyuki Iwashima wrote:
> 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!
Thank you!
kindly check these if any conforms to what it should be:
TEST_F(unix_sock, reset_unread_behavior)
{
char buf[16] = {};
ssize_t n;
/* Send data that will remain unread by client */
send(self->client, "hello", 5, 0);
if (variant->socket_type == SOCK_DGRAM) {
close(self->server);
}
else {
if (!self->child)
SKIP(return);
close(self->child);
}
n = recv(self->client, buf, sizeof(buf), 0);
ASSERT_EQ(-1, n);
if (variant->socket_type == SOCK_STREAM ||
variant->socket_type == SOCK_SEQPACKET)
do { ASSERT_EQ(ECONNRESET, errno); } while (0);
else
ASSERT_EQ(EAGAIN, errno);
}
OR
TEST_F(unix_sock, reset_unread_behavior)
{
char buf[16] = {};
ssize_t n;
/* Send data that will remain unread by client */
send(self->client, "hello", 5, 0);
if (variant->socket_type == SOCK_DGRAM) {
close(self->server);
}
else {
if (self->child)
close(self->child);
else
close(self->server);
}
n = recv(self->client, buf, sizeof(buf), 0);
ASSERT_EQ(-1, n);
if (variant->socket_type == SOCK_STREAM ||
variant->socket_type == SOCK_SEQPACKET)
do { ASSERT_EQ(ECONNRESET, errno); } while (0);
else
ASSERT_EQ(EAGAIN, errno);
}
OR
is there a better way to handle this?
I ran the KCOV_OUTPUT command using the first *TEST_F above* as the Test
2 and got the output below:
*$ KCOV_OUTPUT=kcov KCOV_SLOTS=8192
./tools/testing/selftests/net/af_unix/unix_connreset*
TAP version 13
1..6
# Starting 6 tests from 3 test cases.
# RUN unix_sock.stream.eof ...
# OK unix_sock.stream.eof
ok 1 unix_sock.stream.eof
# RUN unix_sock.stream.reset_unread_behavior ...
# OK unix_sock.stream.reset_unread_behavior
ok 2 unix_sock.stream.reset_unread_behavior
# RUN unix_sock.dgram.eof ...
# OK unix_sock.dgram.eof
ok 3 unix_sock.dgram.eof
# RUN unix_sock.dgram.reset_unread_behavior ...
# OK unix_sock.dgram.reset_unread_behavior
ok 4 unix_sock.dgram.reset_unread_behavior
# RUN unix_sock.seqpacket.eof ...
# OK unix_sock.seqpacket.eof
ok 5 unix_sock.seqpacket.eof
# RUN unix_sock.seqpacket.reset_unread_behavior ...
# OK unix_sock.seqpacket.reset_unread_behavior
ok 6 unix_sock.seqpacket.reset_unread_behavior
# PASSED: 6 / 6 tests passed.
# Totals: pass:6 fail:0 xfail:0 xpass:0 skip:0 error:0
Thank you once again for your continuous guidance and patience.
It's a worthy and rewarding learning period for me.
Powered by blists - more mailing lists