diff mbox series

[v1,net,02/11] selftest: af_unix: Add msg_oob.c.

Message ID 20240625013645.45034-3-kuniyu@amazon.com (mailing list archive)
State Accepted
Commit d098d77232c375cb2cded4a7099f0a763016ee0d
Delegated to: Netdev Maintainers
Headers show
Series af_unix: Fix bunch of MSG_OOB bugs and add new tests. | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for net, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 859 this patch: 859
netdev/build_tools success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 3 maintainers not CCed: shuah@kernel.org linux-kselftest@vger.kernel.org brauner@kernel.org
netdev/build_clang success Errors and warnings before: 863 this patch: 863
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 868 this patch: 868
netdev/checkpatch warning CHECK: spaces preferred around that '*' (ctx:WxV) WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? WARNING: line length of 104 exceeds 80 columns WARNING: line length of 87 exceeds 80 columns WARNING: line length of 90 exceeds 80 columns WARNING: line length of 91 exceeds 80 columns WARNING: line length of 92 exceeds 80 columns WARNING: line length of 96 exceeds 80 columns WARNING: line length of 98 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
netdev/contest success net-next-2024-06-27--03-00 (tests: 665)

Commit Message

Kuniyuki Iwashima June 25, 2024, 1:36 a.m. UTC
AF_UNIX's MSG_OOB functionality lacked thorough testing, and we found
some bizarre behaviour.

The new selftest validates every MSG_OOB operation against TCP as a
reference implementation.

This patch adds only a few tests with basic send() and recv() that
do not fail.

The following patches will add more test cases for SO_OOBINLINE, SIGURG,
EPOLLPRI, and SIOCATMARK.

Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
 tools/testing/selftests/net/af_unix/Makefile  |   2 +-
 tools/testing/selftests/net/af_unix/msg_oob.c | 220 ++++++++++++++++++
 2 files changed, 221 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/net/af_unix/msg_oob.c

Comments

Jakub Kicinski June 26, 2024, 12:44 a.m. UTC | #1
On Mon, 24 Jun 2024 18:36:36 -0700 Kuniyuki Iwashima wrote:
> +	if (ret[0] != expected_len || recv_errno[0] != expected_errno) {
> +		TH_LOG("AF_UNIX :%s", ret[0] < 0 ? strerror(recv_errno[0]) : recv_buf[0]);
> +		TH_LOG("Expected:%s", expected_errno ? strerror(expected_errno) : expected_buf);
> +
> +		ASSERT_EQ(ret[0], expected_len);
> +		ASSERT_EQ(recv_errno[0], expected_errno);
> +	}

repeating the conditions feels slightly imperfect.
Would it be possible to modify EXPECT_* to return the condition?
Then we could:

	if (EXPECT(...)) {
		TH_LOG(...
		TH_LOG(...
	}
Kuniyuki Iwashima June 26, 2024, 1:45 a.m. UTC | #2
From: Jakub Kicinski <kuba@kernel.org>
Date: Tue, 25 Jun 2024 17:44:49 -0700
> On Mon, 24 Jun 2024 18:36:36 -0700 Kuniyuki Iwashima wrote:
> > +	if (ret[0] != expected_len || recv_errno[0] != expected_errno) {
> > +		TH_LOG("AF_UNIX :%s", ret[0] < 0 ? strerror(recv_errno[0]) : recv_buf[0]);
> > +		TH_LOG("Expected:%s", expected_errno ? strerror(expected_errno) : expected_buf);
> > +
> > +		ASSERT_EQ(ret[0], expected_len);
> > +		ASSERT_EQ(recv_errno[0], expected_errno);
> > +	}
> 
> repeating the conditions feels slightly imperfect.

Yeah actually I don't like this...

> Would it be possible to modify EXPECT_* to return the condition?
> Then we could:
> 
> 	if (EXPECT(...)) {
> 		TH_LOG(...
> 		TH_LOG(...
> 	}

We can use EXPECT_EQ() {} here, but for some test cases where TCP is
buggy, I'd like to print the difference but let the test pass.

For example, see patch 6.

  #  RUN           msg_oob.no_peek.ex_oob_ahead_break ...
  # msg_oob.c:146:ex_oob_ahead_break:AF_UNIX :hellowol
  # msg_oob.c:147:ex_oob_ahead_break:TCP     :helloworl
                                                     ^
            TCP recv()s already recv()ed data, "r" --'

  #            OK  msg_oob.no_peek.ex_oob_ahead_break
  ok 11 msg_oob.no_peek.ex_oob_ahead_break

In this case, this does not print the recv()ed data,

  if (self->tcp_compliant) {
      EXPECT_EQ(...) {
          /* log retval, errno, buffer */
      }
  }

and this fails the test even though AF_UNIX is doing correct.

  EXPECT_EQ(...) {
      if (self->tcp_compliant) {
          /* log retval, errno, buffer */
      }
  }

I think we can convert it to EXPECT_EQ() {} in all places after
fixing TCP side and removing tcp_incompliant{} uses in the test.
Jakub Kicinski June 26, 2024, 2:01 a.m. UTC | #3
On Tue, 25 Jun 2024 18:45:55 -0700 Kuniyuki Iwashima wrote:
> We can use EXPECT_EQ() {} here, but for some test cases where TCP is
> buggy, I'd like to print the difference but let the test pass.
...
> I think we can convert it to EXPECT_EQ() {} in all places after
> fixing TCP side and removing tcp_incompliant{} uses in the test.

I see, makes sense
kernel test robot June 27, 2024, 2:05 p.m. UTC | #4
Hi Kuniyuki,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Kuniyuki-Iwashima/selftest-af_unix-Remove-test_unix_oob-c/20240626-033725
base:   net/main
patch link:    https://lore.kernel.org/r/20240625013645.45034-3-kuniyu%40amazon.com
patch subject: [PATCH v1 net 02/11] selftest: af_unix: Add msg_oob.c.
:::::: branch date: 22 hours ago
:::::: commit date: 22 hours ago
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240627/202406270118.ajdL2qcN-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/r/202406270118.ajdL2qcN-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from msg_oob.c:11:
   msg_oob.c: In function '__recvpair':
>> ../../kselftest_harness.h:119:40: warning: format '%s' expects argument of type 'char *', but argument 6 has type 'const void *' [-Wformat=]
     119 |                 fprintf(TH_LOG_STREAM, "# %s:%d:%s:" fmt "\n", \
         |                                        ^~~~~~~~~~~~~
   ../../kselftest_harness.h:114:17: note: in expansion of macro '__TH_LOG'
     114 |                 __TH_LOG(fmt, ##__VA_ARGS__); \
         |                 ^~~~~~~~
   msg_oob.c:120:17: note: in expansion of macro 'TH_LOG'
     120 |                 TH_LOG("Expected:%s", expected_errno ? strerror(expected_errno) : expected_buf);
         |                 ^~~~~~
>> ../../kselftest_harness.h:119:40: warning: format '%s' expects argument of type 'char *', but argument 6 has type 'const void *' [-Wformat=]
     119 |                 fprintf(TH_LOG_STREAM, "# %s:%d:%s:" fmt "\n", \
         |                                        ^~~~~~~~~~~~~
   ../../kselftest_harness.h:114:17: note: in expansion of macro '__TH_LOG'
     114 |                 __TH_LOG(fmt, ##__VA_ARGS__); \
         |                 ^~~~~~~~
   msg_oob.c:140:25: note: in expansion of macro 'TH_LOG'
     140 |                         TH_LOG("Expected:%s", expected_errno ? strerror(expected_errno) : expected_buf);
         |                         ^~~~~~
diff mbox series

Patch

diff --git a/tools/testing/selftests/net/af_unix/Makefile b/tools/testing/selftests/net/af_unix/Makefile
index a25845251eed..50584479540b 100644
--- a/tools/testing/selftests/net/af_unix/Makefile
+++ b/tools/testing/selftests/net/af_unix/Makefile
@@ -1,4 +1,4 @@ 
 CFLAGS += $(KHDR_INCLUDES)
-TEST_GEN_PROGS := diag_uid scm_pidfd scm_rights unix_connect
+TEST_GEN_PROGS := diag_uid msg_oob scm_pidfd scm_rights unix_connect
 
 include ../../lib.mk
diff --git a/tools/testing/selftests/net/af_unix/msg_oob.c b/tools/testing/selftests/net/af_unix/msg_oob.c
new file mode 100644
index 000000000000..d427d39d0806
--- /dev/null
+++ b/tools/testing/selftests/net/af_unix/msg_oob.c
@@ -0,0 +1,220 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright Amazon.com Inc. or its affiliates. */
+
+#include <fcntl.h>
+#include <string.h>
+#include <unistd.h>
+
+#include <netinet/in.h>
+#include <sys/socket.h>
+
+#include "../../kselftest_harness.h"
+
+#define BUF_SZ	32
+
+FIXTURE(msg_oob)
+{
+	int fd[4];		/* 0: AF_UNIX sender
+				 * 1: AF_UNIX receiver
+				 * 2: TCP sender
+				 * 3: TCP receiver
+				 */
+};
+
+static void create_unix_socketpair(struct __test_metadata *_metadata,
+				   FIXTURE_DATA(msg_oob) *self)
+{
+	int ret;
+
+	ret = socketpair(AF_UNIX, SOCK_STREAM | SOCK_NONBLOCK, 0, self->fd);
+	ASSERT_EQ(ret, 0);
+}
+
+static void create_tcp_socketpair(struct __test_metadata *_metadata,
+				  FIXTURE_DATA(msg_oob) *self)
+{
+	struct sockaddr_in addr;
+	socklen_t addrlen;
+	int listen_fd;
+	int ret;
+
+	listen_fd = socket(AF_INET, SOCK_STREAM, 0);
+	ASSERT_GE(listen_fd, 0);
+
+	ret = listen(listen_fd, -1);
+	ASSERT_EQ(ret, 0);
+
+	addrlen = sizeof(addr);
+	ret = getsockname(listen_fd, (struct sockaddr *)&addr, &addrlen);
+	ASSERT_EQ(ret, 0);
+
+	self->fd[2] = socket(AF_INET, SOCK_STREAM, 0);
+	ASSERT_GE(self->fd[2], 0);
+
+	ret = connect(self->fd[2], (struct sockaddr *)&addr, addrlen);
+	ASSERT_EQ(ret, 0);
+
+	self->fd[3] = accept(listen_fd, (struct sockaddr *)&addr, &addrlen);
+	ASSERT_GE(self->fd[3], 0);
+
+	ret = fcntl(self->fd[3], F_SETFL, O_NONBLOCK);
+	ASSERT_EQ(ret, 0);
+}
+
+static void close_sockets(FIXTURE_DATA(msg_oob) *self)
+{
+	int i;
+
+	for (i = 0; i < 4; i++)
+		close(self->fd[i]);
+}
+
+FIXTURE_SETUP(msg_oob)
+{
+	create_unix_socketpair(_metadata, self);
+	create_tcp_socketpair(_metadata, self);
+}
+
+FIXTURE_TEARDOWN(msg_oob)
+{
+	close_sockets(self);
+}
+
+static void __sendpair(struct __test_metadata *_metadata,
+		       FIXTURE_DATA(msg_oob) *self,
+		       const void *buf, size_t len, int flags)
+{
+	int i, ret[2];
+
+	for (i = 0; i < 2; i++)
+		ret[i] = send(self->fd[i * 2], buf, len, flags);
+
+	ASSERT_EQ(ret[0], len);
+	ASSERT_EQ(ret[0], ret[1]);
+}
+
+static void __recvpair(struct __test_metadata *_metadata,
+		       FIXTURE_DATA(msg_oob) *self,
+		       const void *expected_buf, int expected_len,
+		       int buf_len, int flags)
+{
+	int i, ret[2], recv_errno[2], expected_errno = 0;
+	char recv_buf[2][BUF_SZ] = {};
+
+	ASSERT_GE(BUF_SZ, buf_len);
+
+	errno = 0;
+
+	for (i = 0; i < 2; i++) {
+		ret[i] = recv(self->fd[i * 2 + 1], recv_buf[i], buf_len, flags);
+		recv_errno[i] = errno;
+	}
+
+	if (expected_len < 0) {
+		expected_errno = -expected_len;
+		expected_len = -1;
+	}
+
+	if (ret[0] != expected_len || recv_errno[0] != expected_errno) {
+		TH_LOG("AF_UNIX :%s", ret[0] < 0 ? strerror(recv_errno[0]) : recv_buf[0]);
+		TH_LOG("Expected:%s", expected_errno ? strerror(expected_errno) : expected_buf);
+
+		ASSERT_EQ(ret[0], expected_len);
+		ASSERT_EQ(recv_errno[0], expected_errno);
+	}
+
+	if (ret[0] != ret[1] || recv_errno[0] != recv_errno[1]) {
+		TH_LOG("AF_UNIX :%s", ret[0] < 0 ? strerror(recv_errno[0]) : recv_buf[0]);
+		TH_LOG("TCP     :%s", ret[1] < 0 ? strerror(recv_errno[1]) : recv_buf[1]);
+
+		ASSERT_EQ(ret[0], ret[1]);
+		ASSERT_EQ(recv_errno[0], recv_errno[1]);
+	}
+
+	if (expected_len >= 0) {
+		int cmp;
+
+		cmp = strncmp(expected_buf, recv_buf[0], expected_len);
+		if (cmp) {
+			TH_LOG("AF_UNIX :%s", ret[0] < 0 ? strerror(recv_errno[0]) : recv_buf[0]);
+			TH_LOG("Expected:%s", expected_errno ? strerror(expected_errno) : expected_buf);
+
+			ASSERT_EQ(cmp, 0);
+		}
+
+		cmp = strncmp(recv_buf[0], recv_buf[1], expected_len);
+		if (cmp) {
+			TH_LOG("AF_UNIX :%s", ret[0] < 0 ? strerror(recv_errno[0]) : recv_buf[0]);
+			TH_LOG("TCP     :%s", ret[1] < 0 ? strerror(recv_errno[1]) : recv_buf[1]);
+
+			ASSERT_EQ(cmp, 0);
+		}
+	}
+}
+
+#define sendpair(buf, len, flags)					\
+	__sendpair(_metadata, self, buf, len, flags)
+
+#define recvpair(expected_buf, expected_len, buf_len, flags)		\
+	__recvpair(_metadata, self,					\
+		   expected_buf, expected_len, buf_len, flags)
+
+TEST_F(msg_oob, non_oob)
+{
+	sendpair("x", 1, 0);
+
+	recvpair("", -EINVAL, 1, MSG_OOB);
+}
+
+TEST_F(msg_oob, oob)
+{
+	sendpair("x", 1, MSG_OOB);
+
+	recvpair("x", 1, 1, MSG_OOB);
+}
+
+TEST_F(msg_oob, oob_drop)
+{
+	sendpair("x", 1, MSG_OOB);
+
+	recvpair("", -EAGAIN, 1, 0);		/* Drop OOB. */
+	recvpair("", -EINVAL, 1, MSG_OOB);
+}
+
+TEST_F(msg_oob, oob_ahead)
+{
+	sendpair("hello", 5, MSG_OOB);
+
+	recvpair("o", 1, 1, MSG_OOB);
+	recvpair("hell", 4, 4, 0);
+}
+
+TEST_F(msg_oob, oob_break)
+{
+	sendpair("hello", 5, MSG_OOB);
+
+	recvpair("hell", 4, 5, 0);		/* Break at OOB even with enough buffer. */
+	recvpair("o", 1, 1, MSG_OOB);
+}
+
+TEST_F(msg_oob, oob_ahead_break)
+{
+	sendpair("hello", 5, MSG_OOB);
+	sendpair("world", 5, 0);
+
+	recvpair("o", 1, 1, MSG_OOB);
+	recvpair("hell", 4, 9, 0);		/* Break at OOB even after it's recv()ed. */
+	recvpair("world", 5, 5, 0);
+}
+
+TEST_F(msg_oob, oob_break_drop)
+{
+	sendpair("hello", 5, MSG_OOB);
+	sendpair("world", 5, 0);
+
+	recvpair("hell", 4, 10, 0);		/* Break at OOB even with enough buffer. */
+	recvpair("world", 5, 10, 0);		/* Drop OOB and recv() the next skb. */
+	recvpair("", -EINVAL, 1, MSG_OOB);
+}
+
+TEST_HARNESS_MAIN