Re: [PATCH v11.1] selftests/landlock: Add 11 new test suites dedicated to network

["Mickaël Salaün" <mic@digikod.net>]

On Sun, Aug 13, 2023 at 11:09:59PM +0300, Konstantin Meskhidze (A) wrote:
> 
> 
> 7/12/2023 10:02 AM, Mickaël Salaün пишет:
> > 
> > On 06/07/2023 16:55, Mickaël Salaün wrote:
> > > From: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
> > > 
> > > This patch is a revamp of the v11 tests [1] with new tests (see the
> > > "Changes since v11" description).  I (Mickaël) only added the following
> > > todo list and the "Changes since v11" sections in this commit message.
> > > I think this patch is good but it would appreciate reviews.
> > > You can find the diff of my changes here but it is not really readable:
> > > https://git.kernel.org/mic/c/78edf722fba5 (landlock-net-v11 branch)
> > > [1] https://lore.kernel.org/all/20230515161339.631577-11-konstantin.meskhidze@huawei.com/
> > > TODO:
> > > - Rename all "net_service" to "net_port".
> > > - Fix the two kernel bugs found with the new tests.
> > > - Update this commit message with a small description of all tests.
> > 
> > [...]

> > > +FIXTURE(inet)
> > > +{
> > > +	struct service_fixture srv0, srv1;
> > > +};
> > 
> > The "inet" variants are useless and should be removed. The "inet"
> > fixture can then be renamed to "ipv4_tcp".
> > 
>   Maybe its better to name it "tcp". So we dont need to copy TEST_F(tcp,
> port_endianness) for ipv6 and ipv4.
> What do you think?

I don't see any need to test with IPv4 and IPv6, hence the "inet" name
(and without variants). You can rename it to "inet_tcp" to highlight the
specificities of this fixture.

> 
> > 
> > > +
> > > +FIXTURE_VARIANT(inet)
> > > +{
> > > +	const bool is_sandboxed;
> > > +	const struct protocol_variant prot;
> > > +};
> > > +
> > > +/* clang-format off */
> > > +FIXTURE_VARIANT_ADD(inet, no_sandbox_with_ipv4) {
> > > +	/* clang-format on */
> > > +	.is_sandboxed = false,
> > > +	.prot = {
> > > +		.domain = AF_INET,
> > > +		.type = SOCK_STREAM,
> > > +	},
> > > +};
> > > +
> > > +/* clang-format off */
> > > +FIXTURE_VARIANT_ADD(inet, sandbox_with_ipv4) {
> > > +	/* clang-format on */
> > > +	.is_sandboxed = true,
> > > +	.prot = {
> > > +		.domain = AF_INET,
> > > +		.type = SOCK_STREAM,
> > > +	},
> > > +};
> > > +
> > > +/* clang-format off */
> > > +FIXTURE_VARIANT_ADD(inet, no_sandbox_with_ipv6) {
> > > +	/* clang-format on */
> > > +	.is_sandboxed = false,
> > > +	.prot = {
> > > +		.domain = AF_INET6,
> > > +		.type = SOCK_STREAM,
> > > +	},
> > > +};
> > > +
> > > +/* clang-format off */
> > > +FIXTURE_VARIANT_ADD(inet, sandbox_with_ipv6) {
> > > +	/* clang-format on */
> > > +	.is_sandboxed = true,
> > > +	.prot = {
> > > +		.domain = AF_INET6,
> > > +		.type = SOCK_STREAM,
> > > +	},
> > > +};
> > > +
> > > +FIXTURE_SETUP(inet)
> > > +{
> > > +	const struct protocol_variant ipv4_tcp = {
> > > +		.domain = AF_INET,
> > > +		.type = SOCK_STREAM,
> > > +	};
> > > +
> > > +	disable_caps(_metadata);
> > > +
> > > +	ASSERT_EQ(0, set_service(&self->srv0, ipv4_tcp, 0));
> > > +	ASSERT_EQ(0, set_service(&self->srv1, ipv4_tcp, 1));
> > > +
> > > +	setup_loopback(_metadata);
> > > +};
> > > +
> > > +FIXTURE_TEARDOWN(inet)
> > > +{
> > > +}
> > > +
> > > +TEST_F(inet, port_endianness)
> > > +{
> > > +	const struct landlock_ruleset_attr ruleset_attr = {
> > > +		.handled_access_net = LANDLOCK_ACCESS_NET_BIND_TCP |
> > > +				      LANDLOCK_ACCESS_NET_CONNECT_TCP,
> > > +	};
> > > +	const struct landlock_net_service_attr bind_host_endian_p0 = {
> > > +		.allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP,
> > > +		/* Host port format. */
> > > +		.port = self->srv0.port,
> > > +	};
> > > +	const struct landlock_net_service_attr connect_big_endian_p0 = {
> > > +		.allowed_access = LANDLOCK_ACCESS_NET_CONNECT_TCP,
> > > +		/* Big endian port format. */
> > > +		.port = htons(self->srv0.port),
> > > +	};
> > > +	const struct landlock_net_service_attr bind_connect_host_endian_p1 = {
> > > +		.allowed_access = LANDLOCK_ACCESS_NET_BIND_TCP |
> > > +				  LANDLOCK_ACCESS_NET_CONNECT_TCP,
> > > +		/* Host port format. */
> > > +		.port = self->srv1.port,
> > > +	};
> > > +	const unsigned int one = 1;
> > > +	const char little_endian = *(const char *)&one;
> > > +	int ruleset_fd;
> > > +
> > > +	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_NET_SERVICE,
> > > +				       &bind_host_endian_p0, 0));
> > > +	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE,
> > > +				       &connect_big_endian_p0, 0));
> > > +	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE,
> > > +				       &bind_connect_host_endian_p1, 0));
> > > +	enforce_ruleset(_metadata, ruleset_fd);
> > > +
> > > +	/* No restriction for big endinan CPU. */
> > > +	test_bind_and_connect(_metadata, &self->srv0, false, little_endian);
> > > +
> > > +	/* No restriction for any CPU. */
> > > +	test_bind_and_connect(_metadata, &self->srv1, false, false);
> > > +}
> > > +
> > > +TEST_HARNESS_MAIN
> > .