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

[Cyril Hrubis <chrubis@suse.cz>]

Hi!
> Landlock network support has been added in the ABI v4, adding features
> for bind() and connect() syscalls. It also defined one more member in
> the landlock_ruleset_attr struct, breaking our LTP fallbacks, used to
> build landlock testing suite. For this reason, we introduce
> tst_landlock_ruleset_attr_abi[14] struct(s) which fallback ABI v1 and v4
> ruleset_attr definitions.
> 
> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
> ---
>  configure.ac                                       |  3 ++-
>  include/lapi/capability.h                          |  4 ++++
>  include/lapi/landlock.h                            | 28 ++++++++++++----------
>  testcases/kernel/syscalls/landlock/landlock01.c    | 15 ++++--------
>  testcases/kernel/syscalls/landlock/landlock02.c    |  8 +++----
>  testcases/kernel/syscalls/landlock/landlock03.c    |  6 ++---
>  testcases/kernel/syscalls/landlock/landlock04.c    |  6 ++---
>  testcases/kernel/syscalls/landlock/landlock05.c    | 10 ++++----
>  testcases/kernel/syscalls/landlock/landlock06.c    | 14 ++++-------
>  testcases/kernel/syscalls/landlock/landlock07.c    |  6 ++---
>  .../kernel/syscalls/landlock/landlock_common.h     | 12 ++++------
>  11 files changed, 53 insertions(+), 59 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index d327974efa71f263d7f7f5aec9d2c5831d53dd0e..e2e4fd18daa54dbf2034fa9bcc4f2383b53392f4 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -34,6 +34,8 @@ m4_ifndef([PKG_CHECK_EXISTS],
>  AC_PREFIX_DEFAULT(/opt/ltp)
>  
>  AC_CHECK_DECLS([IFLA_NET_NS_PID],,,[#include <linux/if_link.h>])
> +AC_CHECK_DECLS([LANDLOCK_RULE_PATH_BENEATH],,,[#include <linux/landlock.h>])
> +AC_CHECK_DECLS([LANDLOCK_RULE_NET_PORT],,,[#include <linux/landlock.h>])
>  AC_CHECK_DECLS([MADV_MERGEABLE],,,[#include <sys/mman.h>])
>  AC_CHECK_DECLS([NFTA_CHAIN_ID, NFTA_VERDICT_CHAIN_ID],,,[#include <linux/netfilter/nf_tables.h>])
>  AC_CHECK_DECLS([PR_CAPBSET_DROP, PR_CAPBSET_READ],,,[#include <sys/prctl.h>])
> @@ -172,7 +174,6 @@ AC_CHECK_MEMBERS([struct utsname.domainname],,,[
>  ])
>  
>  AC_CHECK_TYPES([enum kcmp_type],,,[#include <linux/kcmp.h>])
> -AC_CHECK_TYPES([enum landlock_rule_type],,,[#include <linux/landlock.h>])
>  AC_CHECK_TYPES([struct acct_v3],,,[#include <sys/acct.h>])
>  AC_CHECK_TYPES([struct af_alg_iv, struct sockaddr_alg],,,[# include <linux/if_alg.h>])
>  AC_CHECK_TYPES([struct fanotify_event_info_fid, struct fanotify_event_info_error,
> diff --git a/include/lapi/capability.h b/include/lapi/capability.h
> index 0f317d6d770e86b399f0fed2de04c1dce6723eae..14d2d3c12c051006875f1f864ec58a88a3870ec0 100644
> --- a/include/lapi/capability.h
> +++ b/include/lapi/capability.h
> @@ -20,6 +20,10 @@
>  # endif
>  #endif
>  
> +#ifndef CAP_NET_BIND_SERVICE
> +# define CAP_NET_BIND_SERVICE 10
> +#endif
> +
>  #ifndef CAP_NET_RAW
>  # define CAP_NET_RAW          13
>  #endif
> diff --git a/include/lapi/landlock.h b/include/lapi/landlock.h
> index 211d171ebecd92d75224369dc7f1d5c5903c9ce7..b3c8c548e661680541cdf6e4a8fb68a3f5029fec 100644
> --- a/include/lapi/landlock.h
> +++ b/include/lapi/landlock.h
> @@ -7,6 +7,7 @@
>  #define LAPI_LANDLOCK_H__
>  
>  #include "config.h"
> +#include <stdint.h>
>  
>  #ifdef HAVE_LINUX_LANDLOCK_H
>  # include <linux/landlock.h>
> @@ -14,13 +15,16 @@
>  
>  #include "lapi/syscalls.h"
>  
> -#ifndef HAVE_STRUCT_LANDLOCK_RULESET_ATTR
> -struct landlock_ruleset_attr
> +struct tst_landlock_ruleset_attr_abi1
> +{
> +	uint64_t handled_access_fs;
> +};
> +
> +struct tst_landlock_ruleset_attr_abi4
>  {
>  	uint64_t handled_access_fs;
>  	uint64_t handled_access_net;
>  };
> -#endif
>  
>  #ifndef HAVE_STRUCT_LANDLOCK_PATH_BENEATH_ATTR
>  struct landlock_path_beneath_attr
> @@ -30,12 +34,12 @@ struct landlock_path_beneath_attr
>  } __attribute__((packed));
>  #endif
>  
> -#ifndef HAVE_ENUM_LANDLOCK_RULE_TYPE
> -enum landlock_rule_type
> -{
> -	LANDLOCK_RULE_PATH_BENEATH = 1,
> -	LANDLOCK_RULE_NET_PORT,
> -};
> +#if !HAVE_DECL_LANDLOCK_RULE_PATH_BENEATH

These are more usually ifndef at least it's more readable.

> +# define LANDLOCK_RULE_PATH_BENEATH 1
> +#endif
> +
> +#if !HAVE_DECL_LANDLOCK_RULE_NET_PORT

Here as well.

> +# define LANDLOCK_RULE_NET_PORT 2
>  #endif
>  
>  #ifndef HAVE_STRUCT_LANDLOCK_NET_PORT_ATTR
> @@ -123,8 +127,7 @@ struct landlock_net_port_attr
>  #endif
>  
>  static inline int safe_landlock_create_ruleset(const char *file, const int lineno,
> -	const struct landlock_ruleset_attr *attr,
> -	size_t size , uint32_t flags)
> +	const void *attr, size_t size , uint32_t flags)
>  {
>  	int rval;
>  
> @@ -143,8 +146,7 @@ static inline int safe_landlock_create_ruleset(const char *file, const int linen
>  }
>  
>  static inline int safe_landlock_add_rule(const char *file, const int lineno,
> -	int ruleset_fd, enum landlock_rule_type rule_type,
> -	const void *rule_attr, uint32_t flags)
> +	int ruleset_fd, int rule_type, const void *rule_attr, uint32_t flags)
>  {
>  	int rval;
>  
> diff --git a/testcases/kernel/syscalls/landlock/landlock01.c b/testcases/kernel/syscalls/landlock/landlock01.c
> index 083685c64fa6d1c0caab887ee03594ea1426f62f..bd3a37153449b8d75b9671f5c3b3838c701b05ae 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_abi4 *ruleset_attr;
> +static struct tst_landlock_ruleset_attr_abi4 *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_abi4 **attr;
>  	uint64_t access_fs;
>  	size_t *size;
>  	uint32_t flags;
> @@ -60,13 +60,8 @@ static void setup(void)
>  {
>  	verify_landlock_is_enabled();
>  
> -	rule_size = sizeof(struct landlock_ruleset_attr);
> -
> -#ifdef HAVE_STRUCT_LANDLOCK_RULESET_ATTR_HANDLED_ACCESS_NET
> +	rule_size = sizeof(struct tst_landlock_ruleset_attr_abi4);
>  	rule_small_size = rule_size - sizeof(uint64_t) - 1;

I guess that the safest bet here would be:

sizeof(struct tst_landlock_ruleset_attr_abi1) - 1

That is by definition one byte less than the smallest size, this will
also in 99.99% cases evaluate to 7 since structure with single 64 bit
number will not need padding so hardcoding 7 should be safe as well.

Also I guess that we can use the v1 ABI for the whole invalid inputs
tests, all we need here is to pass a size that is valid in most cases,
which is v1 I suppose.


The rest looks fine to me:

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

-- 
Cyril Hrubis
chrubis@suse.cz

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