Re: [LTP] [PATCH 1/5] Fallback landlock network support

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> -#ifndef HAVE_ENUM_LANDLOCK_RULE_TYPE
> -enum landlock_rule_type
> -{
> -	LANDLOCK_RULE_PATH_BENEATH = 1,
> -	LANDLOCK_RULE_NET_PORT,
> -};
> +#ifndef LANDLOCK_RULE_PATH_BENEATH
> +# define LANDLOCK_RULE_PATH_BENEATH 1
> +#endif
> +
> +#ifndef LANDLOCK_RULE_NET_PORT
> +# define LANDLOCK_RULE_NET_PORT 2
>  #endif

Does this really work?

Because unlike the glibc that does additionally define enum mebers the
Linux kernel headers does not do that.

I.e. in glibc you have:

enum foo {
	FOO_AAA,
#define FOO_AAA FOO_AAA
	...
};

So that you can do #ifdef FOO_AAA, but there is no such thing in
linux/landlock.h so unfortunatelly we really have to check for each
individual enum member in configure.

> diff --git a/testcases/kernel/syscalls/landlock/landlock01.c b/testcases/kernel/syscalls/landlock/landlock01.c
> index 083685c64..7f767c007 100644
> --- a/testcases/kernel/syscalls/landlock/landlock01.c
> +++ b/testcases/kernel/syscalls/landlock/landlock01.c
> @@ -17,14 +17,14 @@
>  
>  #include "landlock_common.h"
>  
> -static struct landlock_ruleset_attr *ruleset_attr;
> -static struct landlock_ruleset_attr *null_attr;
> +static struct tst_landlock_ruleset_attr *ruleset_attr;
> +static struct tst_landlock_ruleset_attr *null_attr;
>  static size_t rule_size;
>  static size_t rule_small_size;
>  static size_t rule_big_size;
>  
>  static struct tcase {
> -	struct landlock_ruleset_attr **attr;
> +	struct tst_landlock_ruleset_attr **attr;
>  	uint64_t access_fs;
>  	size_t *size;
>  	uint32_t flags;
> @@ -44,10 +44,10 @@ static void run(unsigned int n)
>  	struct tcase *tc = &tcases[n];
>  
>  	if (*tc->attr)
> -		(*tc->attr)->handled_access_fs = tc->access_fs;
> +		(*tc->attr)->base.handled_access_fs = tc->access_fs;
>  
>  	TST_EXP_FAIL(tst_syscall(__NR_landlock_create_ruleset,
> -			*tc->attr, *tc->size, tc->flags),
> +			&(*tc->attr)->base, *tc->size, tc->flags),
>  		tc->exp_errno,
>  		"%s",
>  		tc->msg);
> @@ -60,12 +60,12 @@ static void setup(void)
>  {
>  	verify_landlock_is_enabled();
>  
> -	rule_size = sizeof(struct landlock_ruleset_attr);
> +	rule_size = sizeof(struct tst_landlock_ruleset_attr);
>  
>  #ifdef HAVE_STRUCT_LANDLOCK_RULESET_ATTR_HANDLED_ACCESS_NET
> -	rule_small_size = rule_size - sizeof(uint64_t) - 1;
> +	rule_small_size = rule_size - 2*sizeof(uint64_t) - 1;
>  #else
> -	rule_small_size = rule_size - 1;
> +	rule_small_size = rule_size - sizeof(uint64_t) - 1;
>  #endif

This is also completely wrong. You cannot check the system headers and
deduce anything about the currently running kernel from them. There is
no coupling between the system the tests are compiled on and the system
the test are executed later on.

Which ABI is supported by the kernel has to be deduced at runtime from
the kernel.

Also you have changed the rule_size = sizeof() to the fallback
structure which size is always constant. Which makes the rule_small_size
even more wrong.

What are you trying to do here? It looks to me like the rule_small_size
ends up being set to -1 anyways, because the size of the ruleset_attr
structure is equal to the sizeof() of the uint64_t or two uint64_t.

>  	rule_big_size = SAFE_SYSCONF(_SC_PAGESIZE) + 1;
> @@ -77,7 +77,7 @@ static struct tst_test test = {
>  	.setup = setup,
>  	.needs_root = 1,
>  	.bufs = (struct tst_buffers []) {
> -		{&ruleset_attr, .size = sizeof(struct landlock_ruleset_attr)},
> +		{&ruleset_attr, .size = sizeof(struct tst_landlock_ruleset_attr)},
>  		{},
>  	},
>  	.caps = (struct tst_cap []) {
> diff --git a/testcases/kernel/syscalls/landlock/landlock02.c b/testcases/kernel/syscalls/landlock/landlock02.c
> index 1a3df69c9..bdef57b55 100644
> --- a/testcases/kernel/syscalls/landlock/landlock02.c
> +++ b/testcases/kernel/syscalls/landlock/landlock02.c
> @@ -20,7 +20,7 @@
>  
>  #include "landlock_common.h"
>  
> -static struct landlock_ruleset_attr *ruleset_attr;
> +static struct tst_landlock_ruleset_attr *ruleset_attr;
>  static struct landlock_path_beneath_attr *path_beneath_attr;
>  static struct landlock_path_beneath_attr *rule_null;
>  static int ruleset_fd;
> @@ -28,7 +28,7 @@ static int invalid_fd = -1;
>  
>  static struct tcase {
>  	int *fd;
> -	enum landlock_rule_type rule_type;
> +	int rule_type;
>  	struct landlock_path_beneath_attr **attr;
>  	int access;
>  	int parent_fd;
> @@ -103,10 +103,10 @@ static void setup(void)
>  {
>  	verify_landlock_is_enabled();
>  
> -	ruleset_attr->handled_access_fs = LANDLOCK_ACCESS_FS_EXECUTE;
> +	ruleset_attr->base.handled_access_fs = LANDLOCK_ACCESS_FS_EXECUTE;
>  
>  	ruleset_fd = TST_EXP_FD_SILENT(tst_syscall(__NR_landlock_create_ruleset,
> -		ruleset_attr, sizeof(struct landlock_ruleset_attr), 0));
> +		&ruleset_attr->base, sizeof(struct tst_landlock_ruleset_attr), 0));

And this does not work either as this would disable these tests on older
kernels because it would pass size that is not supported.

So I supose that this is getting out of hand and we should ingore the
structure in the system headers and instead create two different
structures:

struct tst_landlock_ruleset_attr_1 {
	uint64_t handled_access_fs;
};

struct tst_landlock_ruleset_attr_2 {
	uint64_t handled_access_fs;
	uint64_t handled_access_net;
};

And use the tst_landlock_ruleset_attr_1 in the tests where the network
part is not needed.

-- 
Cyril Hrubis
chrubis@suse.cz

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