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

Andrea Cervesato via ltp
2024-11-05T13:15:38Z

Hi Cyril,

On 11/5/24 14:08, Cyril Hrubis wrote:
> 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.
>
We can't use #ifndef because HAVE_DECL_LANDLOCK_RULE_PATH_BENEATH is
always defined, but it can be 0 or 1 if it's present or not (this is
what I seen using autoconf). You can check in config.h as well.
Apparently this is how autoconf handles symbols.
>> +# 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
+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>
Andrea