Re: [RFC PATCH v1 03/10] selftests/landlock: Create 'create' test

Hello!

I am very happy to see this patch set, this is a very valuable feature, IMHO! :)

Regarding the subject of this patch:

  [RFC PATCH v1 03/10] selftests/landlock: Create 'create' test
                                                   ^^^^^^

This was probably meant to say "socket"?

(In my mind, it is a good call to put the test in a separate file -
the existing test files have grown too large already.)


On Mon, Apr 08, 2024 at 05:39:20PM +0800, Ivanov Mikhail wrote:

This helper is mostly concerned with mapping the error code.

In the fs_test.c, we have dealt with such use cases with helpers like
test_open_rel() and test_open().  These helpers attempt to open the file, take
the same arguments as open(2), but instead of returning the opened fd, they only
return 0 or errno.  Do you think this would be an option here?

Then you could write your tests as

  ASSERT_EQ(EACCES, test_socket(p->domain, p->type, 0));

and the test would (a) more obviously map to socket(2), and (b) keep relevant
information like the expected error code at the top level of the test.

+
+FIXTURE(protocol)
+{
+	struct service_fixture srv0;
+};
+
+FIXTURE_VARIANT(protocol)
+{
+	const struct protocol_variant protocol;
+};
+
+FIXTURE_SETUP(protocol)
+{
+	disable_caps(_metadata);
+	self->srv0.protocol = variant->protocol;
+	setup_namespace(_metadata);
+};
+
+FIXTURE_TEARDOWN(protocol)
+{
+}
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(protocol, unspec) {
+	/* clang-format on */
+	.protocol = {
+		.domain = AF_UNSPEC,
+		.type = SOCK_STREAM,
+	},
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(protocol, unix_stream) {
+	/* clang-format on */
+	.protocol = {
+		.domain = AF_UNIX,
+		.type = SOCK_STREAM,
+	},
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(protocol, unix_dgram) {
+	/* clang-format on */
+	.protocol = {
+		.domain = AF_UNIX,
+		.type = SOCK_DGRAM,
+	},
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(protocol, ipv4_tcp) {
+	/* clang-format on */
+	.protocol = {
+		.domain = AF_INET,
+		.type = SOCK_STREAM,
+	},
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(protocol, ipv4_udp) {
+	/* clang-format on */
+	.protocol = {
+		.domain = AF_INET,
+		.type = SOCK_DGRAM,
+	},
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(protocol, ipv6_tcp) {
+	/* clang-format on */
+	.protocol = {
+		.domain = AF_INET6,
+		.type = SOCK_STREAM,
+	},
+};
+
+/* clang-format off */
+FIXTURE_VARIANT_ADD(protocol, ipv6_udp) {
+	/* clang-format on */
+	.protocol = {
+		.domain = AF_INET6,
+		.type = SOCK_DGRAM,
+	},
+};
+
+static void test_socket_create(struct __test_metadata *const _metadata,
+				  const struct service_fixture *const srv,
+				  const bool deny_create)
+{
+	int fd;
+
+	fd = socket_variant(srv);
+	if (srv->protocol.domain == AF_UNSPEC) {
+		EXPECT_EQ(fd, -EAFNOSUPPORT);
+	} else if (deny_create) {
+		EXPECT_EQ(fd, -EACCES);
+	} else {
+		EXPECT_LE(0, fd)
+		{
+			TH_LOG("Failed to create socket: %s", strerror(errno));
+		}
+		EXPECT_EQ(0, close(fd));
+	}
+}

This is slightly too much logic in a test helper, for my taste,
and the meaning of the true/false argument in the main test below
is not very obvious.

Extending the idea from above, if test_socket() was simpler, would it
be possible to turn these fixtures into something shorter like this:

  ASSERT_EQ(EAFNOSUPPORT, test_socket(AF_UNSPEC, SOCK_STREAM, 0));
  ASSERT_EQ(EACCES, test_socket(AF_UNIX, SOCK_STREAM, 0));
  ASSERT_EQ(EACCES, test_socket(AF_UNIX, SOCK_DGRAM, 0));
  ASSERT_EQ(EACCES, test_socket(AF_INET, SOCK_STREAM, 0));
  // etc.

Would that make the tests easier to write, to list out the table of
expected values aspect like that, rather than in a fixture?

+
+TEST_F(protocol, create)
+{
+	const struct landlock_ruleset_attr ruleset_attr = {
+		.handled_access_socket = LANDLOCK_ACCESS_SOCKET_CREATE,
+	};
+	const struct landlock_socket_attr create_socket_attr = {
+		.allowed_access = LANDLOCK_ACCESS_SOCKET_CREATE,
+		.domain = self->srv0.protocol.domain,
+		.type = self->srv0.protocol.type,
+	};
+
+	int ruleset_fd;
+
+	/* Allowed create */
+	ruleset_fd = landlock_create_ruleset(&ruleset_attr,
+							sizeof(ruleset_attr), 0);
+	ASSERT_LE(0, ruleset_fd);
+
+	ASSERT_EQ(0,
+			landlock_add_rule(ruleset_fd, LANDLOCK_RULE_SOCKET,
+					&create_socket_attr, 0));

The indentation looks wrong?  We run clang-format on Landlock files.

  clang-format -i security/landlock/*.[ch] \
  	include/uapi/linux/landlock.h \
  	tools/testing/selftests/landlock/*.[ch]

—Günther