Re: [LTP] [PATCH v2 4/4] Add error coverage for landlock network support

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> diff --git a/testcases/kernel/syscalls/landlock/landlock02.c b/testcases/kernel/syscalls/landlock/landlock02.c
> index 8566d407f6d17ab367695125f07d0a80cf4130e5..dbc405a8a01ac58e0d22f952f57bd603c62ab8be 100644
> --- a/testcases/kernel/syscalls/landlock/landlock02.c
> +++ b/testcases/kernel/syscalls/landlock/landlock02.c
> @@ -20,93 +20,146 @@
>  
>  #include "landlock_common.h"
>  
> -static struct tst_landlock_ruleset_attr_abi1 *ruleset_attr;
> +static struct tst_landlock_ruleset_attr_abi4 *ruleset_attr;
>  static struct landlock_path_beneath_attr *path_beneath_attr;
>  static struct landlock_path_beneath_attr *rule_null;
> +static struct landlock_net_port_attr *net_port_attr;
>  static int ruleset_fd;
>  static int invalid_fd = -1;
> +static int abi_current;
>  
>  static struct tcase {
>  	int *fd;
> -	enum landlock_rule_type rule_type;
> -	struct landlock_path_beneath_attr **attr;
> +	int rule_type;
> +	struct landlock_path_beneath_attr **path_attr;
> +	struct landlock_net_port_attr **net_attr;
>  	int access;
>  	int parent_fd;
> +	int net_port;
>  	uint32_t flags;
>  	int exp_errno;
> +	int abi_ver;
>  	char *msg;
>  } tcases[] = {
>  	{
>  		.fd = &ruleset_fd,
> -		.attr = &path_beneath_attr,
> +		.path_attr = &path_beneath_attr,
> +		.net_attr = NULL,

This is a static structure, so anything that is not initialized will be
zeroed anyways, so I would just omit the explicit NULL initializations.

>  		.access = LANDLOCK_ACCESS_FS_EXECUTE,
>  		.flags = 1,
>  		.exp_errno = EINVAL,
> +		.abi_ver = 1,
>  		.msg = "Invalid flags"
>  	},
>  	{
>  		.fd = &ruleset_fd,
> -		.attr = &path_beneath_attr,
> +		.path_attr = &path_beneath_attr,
> +		.net_attr = NULL,
>  		.access = LANDLOCK_ACCESS_FS_EXECUTE,
>  		.exp_errno = EINVAL,
> +		.abi_ver = 1,
>  		.msg = "Invalid rule type"
>  	},
>  	{
>  		.fd = &ruleset_fd,
>  		.rule_type = LANDLOCK_RULE_PATH_BENEATH,
> -		.attr = &path_beneath_attr,
> +		.path_attr = &path_beneath_attr,
> +		.net_attr = NULL,
>  		.exp_errno = ENOMSG,
> +		.abi_ver = 1,
>  		.msg = "Empty accesses"
>  	},
>  	{
>  		.fd = &invalid_fd,
> -		.attr = &path_beneath_attr,
> +		.path_attr = &path_beneath_attr,
> +		.net_attr = NULL,
>  		.access = LANDLOCK_ACCESS_FS_EXECUTE,
>  		.exp_errno = EBADF,
> +		.abi_ver = 1,
>  		.msg = "Invalid file descriptor"
>  	},
>  	{
>  		.fd = &ruleset_fd,
>  		.rule_type = LANDLOCK_RULE_PATH_BENEATH,
> -		.attr = &path_beneath_attr,
> +		.path_attr = &path_beneath_attr,
> +		.net_attr = NULL,
>  		.access = LANDLOCK_ACCESS_FS_EXECUTE,
>  		.parent_fd = -1,
>  		.exp_errno = EBADF,
> +		.abi_ver = 1,
>  		.msg = "Invalid parent fd"
>  	},
>  	{
>  		.fd = &ruleset_fd,
>  		.rule_type = LANDLOCK_RULE_PATH_BENEATH,
> -		.attr = &rule_null,
> +		.path_attr = &rule_null,
> +		.net_attr = NULL,
>  		.exp_errno = EFAULT,
> +		.abi_ver = 1,
>  		.msg = "Invalid rule attr"
>  	},
> +	{
> +		.fd = &ruleset_fd,
> +		.rule_type = LANDLOCK_RULE_NET_PORT,
> +		.path_attr = NULL,
> +		.net_attr = &net_port_attr,
> +		.access = LANDLOCK_ACCESS_FS_EXECUTE,
> +		.net_port = 448,
> +		.exp_errno = EINVAL,
> +		.abi_ver = 4,
> +		.msg = "Invalid access rule for network type"
> +	},
> +	{
> +		.fd = &ruleset_fd,
> +		.rule_type = LANDLOCK_RULE_NET_PORT,
> +		.path_attr = NULL,
> +		.net_attr = &net_port_attr,
> +		.access = LANDLOCK_ACCESS_NET_BIND_TCP,
> +		.net_port = INT16_MAX + 1,
> +		.exp_errno = EINVAL,
> +		.abi_ver = 4,
> +		.msg = "Socket port greater than 65535"
> +	},
>  };
>  
>  static void run(unsigned int n)
>  {
>  	struct tcase *tc = &tcases[n];
>  
> -	if (*tc->attr) {
> -		(*tc->attr)->allowed_access = tc->access;
> -		(*tc->attr)->parent_fd = tc->parent_fd;
> +	if (tc->abi_ver > abi_current) {
> +		tst_res(TCONF, "Minimum ABI required: %d", tc->abi_ver);
> +		return;
>  	}
>  
> -	TST_EXP_FAIL(tst_syscall(__NR_landlock_add_rule,
> -		*tc->fd, tc->rule_type, *tc->attr, tc->flags),
> -		tc->exp_errno,
> -		"%s",
> -		tc->msg);
> +	if (tc->path_attr) {
> +		if (*tc->path_attr) {
> +			(*tc->path_attr)->allowed_access = tc->access;
> +			(*tc->path_attr)->parent_fd = tc->parent_fd;
> +		}
> +
> +		TST_EXP_FAIL(tst_syscall(__NR_landlock_add_rule,
> +			*tc->fd, tc->rule_type, *tc->path_attr, tc->flags),
> +			tc->exp_errno, "%s", tc->msg);
> +	} else if (tc->net_attr) {
> +		if (*tc->net_attr) {
> +			(*tc->net_attr)->allowed_access = tc->access;
> +			(*tc->net_attr)->port = tc->net_port;
> +		}
> +
> +		TST_EXP_FAIL(tst_syscall(__NR_landlock_add_rule,
> +			*tc->fd, tc->rule_type, *tc->net_attr, tc->flags),
> +			tc->exp_errno, "%s", tc->msg);

if we assing the attr into a pointer this TST_EPX_FAIL() can be outside
of the if as:

	void *attr;

	if (path_attr) {
		...
		attr = *path_attr;
	} else {
		...
		attr = *net_attr;
	}

	TST_EXP_FAIL(..., attr, ...);

> +	}
>  }
>  
>  static void setup(void)
>  {
> -	verify_landlock_is_enabled();
> +	abi_current = verify_landlock_is_enabled();
>  
>  	ruleset_attr->handled_access_fs = LANDLOCK_ACCESS_FS_EXECUTE;
>  
>  	ruleset_fd = TST_EXP_FD_SILENT(tst_syscall(__NR_landlock_create_ruleset,
> -		ruleset_attr, sizeof(struct tst_landlock_ruleset_attr_abi1), 0));
> +		ruleset_attr, sizeof(struct tst_landlock_ruleset_attr_abi4), 0));
                               ^
			       This should be abi_current otherwise we
			       will fail on v1 only system.

>  }
>  
>  static void cleanup(void)
> @@ -122,8 +175,9 @@ static struct tst_test test = {
>  	.cleanup = cleanup,
>  	.needs_root = 1,
>  	.bufs = (struct tst_buffers []) {
> -		{&ruleset_attr, .size = sizeof(struct tst_landlock_ruleset_attr_abi1)},
> +		{&ruleset_attr, .size = sizeof(struct tst_landlock_ruleset_attr_abi4)},
>  		{&path_beneath_attr, .size = sizeof(struct landlock_path_beneath_attr)},
> +		{&net_port_attr, .size = sizeof(struct landlock_net_port_attr)},
>  		{},
>  	},
>  	.caps = (struct tst_cap []) {

The rest looks good to me, with the minor probles fixed:

Reviewed-by: Cyril Hrubis <chrubis@suse.cz>

-- 
Cyril Hrubis
chrubis@suse.cz

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp