Re: [RFC PATCH v4 10/15] seltest/landlock: add tests for bind() hooks

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

On 09/03/2022 14:44, Konstantin Meskhidze wrote:
> Adds two selftests for bind socket action.
> The one is with no landlock restrictions:
>      - bind_no_restrictions;
> The second one is with mixed landlock rules:
>      - bind_with_restrictions;
> 
> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
> ---
> 
> Changes since v3:
> * Split commit.
> * Add helper create_socket.
> * Add FIXTURE_SETUP.
> 
> ---
>   .../testing/selftests/landlock/network_test.c | 153 ++++++++++++++++++
>   1 file changed, 153 insertions(+)
>   create mode 100644 tools/testing/selftests/landlock/network_test.c
> 
> diff --git a/tools/testing/selftests/landlock/network_test.c b/tools/testing/selftests/landlock/network_test.c
> new file mode 100644
> index 000000000000..4c60f6d973a8
> --- /dev/null
> +++ b/tools/testing/selftests/landlock/network_test.c

[...]

> +
> +uint port[MAX_SOCKET_NUM];
> +struct sockaddr_in addr[MAX_SOCKET_NUM];

You should not change global variables, it is a source of issue. Instead 
use FIXTURE local variables accessible through self->X.

> +
> +const int one = 1;

This doesn't need to be global.

[...]

> +
> +static void enforce_ruleset(struct __test_metadata *const _metadata,
> +		const int ruleset_fd)
> +{
> +	ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0));
> +	ASSERT_EQ(0, landlock_restrict_self(ruleset_fd, 0)) {
> +		TH_LOG("Failed to enforce ruleset: %s", strerror(errno));
> +	}
> +}

You should move the same helper from fs_base.c to common.h (see caps 
helpers) and reuse it here.


> +
> +FIXTURE(socket) { };
> +
> +FIXTURE_SETUP(socket)
> +{
> +	int i;

Please add a new line between declaration and actual code (everywhere).

> +	/* Creates socket addresses */
> +	for (i = 0; i < MAX_SOCKET_NUM; i++) {

Use ARRAY_SIZE() instead of MAY_SOCKET_NUM.


> +		port[i] = SOCK_PORT_START + SOCK_PORT_ADD*i;

Use self->port[i] and self->addr[i] instead.

> +		addr[i].sin_family = AF_INET;
> +		addr[i].sin_port = htons(port[i]);
> +		addr[i].sin_addr.s_addr = inet_addr(IP_ADDRESS);
> +		memset(&(addr[i].sin_zero), '\0', 8);
> +	}
> +}

[...]

> +	/* Allows connect and deny bind operations to the port[1] socket. */
> +	ASSERT_EQ(0, landlock_add_rule(ruleset_fd, LANDLOCK_RULE_NET_SERVICE,
> +				&net_service_2, 0));
> +	/* Empty allowed_access (i.e. deny rules) are ignored in network actions

The kernel coding style says to start a multi-line comments with a "/*" 
and a new line.