landlock: Refactor layer masks
Subsystems (3)
v2 cover landlock: Refactor layer masks
Hello!
This patch set "transposes" the layer masks matrix, which was previously modeled as a access-max-sized array of layer masks, and changes it to be a layer-max-sized array of access masks instead. (It is a pure refactoring, there are no user-visible changes.)
This unlocks a few code simplifications and in multiple places it removes the need for loops and branches that deal with individual bits. Instead, the changed data structure now lends itself for more bitwise operations. The underlying hypothesis for me was that by using more bitwise operations and fewer branches, we would get an overall speedup even when the data structure size increases slightly in some cases.
Tentative results with and without this patch set show that the hypothesis likely holds true. The benchmark I used exercises a "worst case" scenario that attempts to be bottlenecked on the affected code:
constructs a large number of nested directories, with one "path beneath" rule each and then tries to open the innermost directory many times. The benchmark is intentionally unrealistic to amplify the amount of time used for the path walk logic and forces Landlock to walk the full path (eventually failing the open syscall). (I'll send the benchmark program in a reply to this mail for full transparency.)
Measured with the benchmark program, the patch set results in a speedup of about -10%. The benchmark results are only tentative and have been produced in Qemu:
With the patch, the benchmark runs in 6046 clocks (measured with times(3)):
*** Benchmark ***
10000 dirs, 100000 iterations, with landlock
*** Benchmark concluded ***
System: 6046 clocks
User : 1 clocks
Clocks per second: 1000000
Without the patch, we get 6713 clocks, which is 11% more
*** Benchmark ***
10000 dirs, 100000 iterations, with landlock
*** Benchmark concluded ***
System: 6713 clocks
User : 0 clocks
Clocks per second: 1000000
The base revision used for benchmarking was commit 7a51784da76d ("tools/sched_ext: update scx_show_state.py for scx_aborting change")
In real-life scenarios, the speed improvement from this patch set will be less pronounced than in the artificial benchmark, as people do not usually stack directories that deeply and attach so many rules to them, and the EACCES error should also be the exception rather than the norm.
I am looking forward to your feedback.
P.S.: I am open to suggestions on what the "layer masks" variables should be called, because the name "layer masks" might be less appropriate after this change. I have not fixed up the name everywhere because fixing up the code took priority for now.
---
Changes since previous versions:
V2: (This patch set)
* Remove the refactoring around the deny_mask_t type,
it is better to send that as a separate patch (mic review)
* Added the benchmark program to the selftests
* Fix unused variable report for "access_dom":
https://lore.kernel.org/all/[email protected]/
* Use size_t and ARRAY_SIZE to loop over the layers (mic review)
* Documentation
* Fixing up and adding back documentaiton (mic review)
* Documented landlock_unmask_layers()
* Fixed up kernel docs in a place where it was improperly updated (Spotted by Randy Dunlap
https://lore.kernel.org/all/[email protected]/)
* Minor
* Const, some newlines (mic review)
V1: (Initial version)
https://lore.kernel.org/all/[email protected]/
Günther Noack (3):
selftests/landlock: Add filesystem access benchmark
landlock: access_mask_subset() helper
landlock: transpose the layer masks data structure
security/landlock/access.h | 16 +- security/landlock/audit.c | 84 ++--- security/landlock/audit.h | 3 +- security/landlock/domain.c | 45 +-- security/landlock/domain.h | 4 +- security/landlock/fs.c | 354 +++++++++----------- security/landlock/net.c | 11 +- security/landlock/ruleset.c | 88 ++--- security/landlock/ruleset.h | 21 +- tools/testing/selftests/landlock/.gitignore | 1 + tools/testing/selftests/landlock/Makefile | 1 + tools/testing/selftests/landlock/fs_bench.c | 161 +++++++++ 12 files changed, 444 insertions(+), 345 deletions(-) create mode 100644 tools/testing/selftests/landlock/fs_bench.c
Signature · 1 line
2.52.0
PATCH v2 1/3 selftests/landlock: Add filesystem access benchmark
fs_bench benchmarks the performance of Landlock's path walk by exercising it in a scenario that amplifies Landlock's overhead:
* Create a large number of nested directories
* Enforce a Landlock policy in which a rule is associated with each of these subdirectories
* Benchmark openat() applied to the deepest directory, forcing Landlock to walk the entire path.
--- tools/testing/selftests/landlock/.gitignore | 1 + tools/testing/selftests/landlock/Makefile | 1 + tools/testing/selftests/landlock/fs_bench.c | 161 ++++++++++++++++++++ 3 files changed, 163 insertions(+) create mode 100644 tools/testing/selftests/landlock/fs_bench.c
tools/testing/selftests/landlock/.gitignore +1 -0
@@ -1,4 +1,5 @@ /*_test +/fs_bench /sandbox-and-launch /true /wait-pipe
tools/testing/selftests/landlock/Makefile +1 -0
@@ -9,6 +9,7 @@ LOCAL_HDRS += $(wildcard *.h) src_test := $(wildcard *_test.c) TEST_GEN_PROGS := $(src_test:.c=) +TEST_GEN_PROGS += fs_bench TEST_GEN_PROGS_EXTENDED := \ true \
tools/testing/selftests/landlock/fs_bench.c new file +161 -0
161 lines not on this page · raw message
Signature · 1 line
2.52.0
Mickaël Salaün
maintainerRe: [PATCH v2 1/3] selftests/landlock: Add filesystem access benchmark
Quoted 45 lines · Günther Noack
fs_benchbenchmarks the performance of Landlock's path walk by exercising it in a scenario that amplifies Landlock's overhead:* Create a large number of nested directories
* Enforce a Landlock policy in which a rule is associated with each of these subdirectories
* Benchmarkopenat()applied to the deepest directory, forcing Landlock to walk the entire path.Signed-off-by Günther Noack <[email protected]>--- tools/testing/selftests/landlock/.gitignore | 1 + tools/testing/selftests/landlock/Makefile | 1 + tools/testing/selftests/landlock/fs_bench.c | 161 ++++++++++++++++++++ 3 files changed, 163 insertions(+) create mode 100644 tools/testing/selftests/landlock/fs_bench.ctools/testing/selftests/landlock/.gitignore +1
-0@@ -1,4 +1,5 @@ /*_test +/fs_bench /sandbox-and-launch /true /wait-pipetools/testing/selftests/landlock/Makefile +1
-0@@ -9,6 +9,7 @@ LOCAL_HDRS += $(wildcard *.h) src_test := $(wildcard *_test.c) TEST_GEN_PROGS := $(src_test:.c=) +TEST_GEN_PROGS += fs_bench TEST_GEN_PROGS_EXTENDED := \ true \tools/testing/selftests/landlock/fs_bench.c new file +0
-0
tools/testing/selftests/landlock/fs_bench.c line 3
Quoted 3 lines
tools/testing/selftests/landlock/fs_bench.c new file +3
-0@@ -0,0 +1,161 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Landlock filesystem benchmark
You might want to add some copyright.
Quoted 9 lines
tools/testing/selftests/landlock/fs_bench.c new file +9
-0+*/ + +#define _GNU_SOURCE +#include <err.h> +#include <fcntl.h> +#include <linux/landlock.h> +#include <stdbool.h> +#include <stdio.h> +#include <stdio.h>
tools/testing/selftests/landlock/fs_bench.c line 22
Quoted 10 lines
tools/testing/selftests/landlock/fs_bench.c new file +10
-0+#include <stdlib.h> +#include <string.h> +#include <sys/prctl.h> +#include <sys/stat.h> +#include <sys/syscall.h> +#include <sys/times.h> +#include <time.h> +#include <unistd.h> + +void usage (const char *argv0)
const
Quoted 8 lines
tools/testing/selftests/landlock/fs_bench.c new file +8
-0+{ +printf ("Usage:\n" ); +printf (" %s [OPTIONS]\n" , argv0); +printf ("\n" ); +printf (" Benchmark expensive Landlock checks for D nested dirs\n" ); +printf ("\n" ); +printf ("Options:\n" ); +printf (" -h help\n" );
tools/testing/selftests/landlock/fs_bench.c line 40
Quoted 10 lines
tools/testing/selftests/landlock/fs_bench.c new file +10
-0+printf (" -L disable Landlock (as a baseline)\n" ); +printf (" -d D set directory depth to D\n" ); +printf (" -n N set number of benchmark iterations to N\n" ); +} + +/* +* Build a deep directory, enforce Landlock and return the FD to the +* deepest dir. On any failure, exit the process with an error. +*/ +int build_directory (size_t depth,bool use_landlock)
const
tools/testing/selftests/landlock/fs_bench.c line 47
Quoted 7 lines
tools/testing/selftests/landlock/fs_bench.c new file +7
-0+{ +const char *path ="d" ;/* directory name */ +int abi, ruleset_fd, current, previous; + +if (use_landlock) { + abi =syscall (SYS_landlock_create_ruleset,NULL ,0 , +LANDLOCK_CREATE_RULESET_VERSION );
Please include wrappers.h and use the related syscall helpers. One of the benefit is to use __NR_* constants defined by the installed kernel headers.
Quoted 23 lines
tools/testing/selftests/landlock/fs_bench.c new file +23
-0+if (abi <7 ) +err (1 ,"Landlock ABI too low: got %d, wanted 7+" , abi); + } + + ruleset_fd =-1 ; +if (use_landlock) { +struct landlock_ruleset_attr attr = { + .handled_access_fs = +0xffff ,/* All FS access rights as of 2026-01 */ + }; + ruleset_fd =syscall (SYS_landlock_create_ruleset, &attr, +sizeof (attr),0U ); +if (ruleset_fd <0 ) +err (1 ,"landlock_create_ruleset" ); + } + + current =open ("." ,O_PATH ); +if (current <0 ) +err (1 ,"open(.)" ); + +while (depth--) { +if (use_landlock) { + struct landlock_path_beneath_attr attr = {
tools/testing/selftests/landlock/fs_bench.c line 80
Quoted 10 lines
tools/testing/selftests/landlock/fs_bench.c new file +10
-0+ .allowed_access = LANDLOCK_ACCESS_FS_IOCTL_DEV, + .parent_fd = current, + }; +if (syscall (SYS_landlock_add_rule, ruleset_fd, +LANDLOCK_RULE_PATH_BENEATH , &attr,0 ) <0 ) +err (1 ,"landlock_add_rule" ); + } + +if (mkdirat (current, path,0700 ) <0 ) +err (1 ,"mkdirat(%s)" , path);
We should have a loop to build the directories, then start the timer and have another loop to add Landlock rules.
Quoted 41 lines
tools/testing/selftests/landlock/fs_bench.c new file +41
-0+ + previous = current; + current =openat (current, path,O_PATH ); +if (current <0 ) +err (1 ,"open(%s)" , path); + +close (previous); + } + +if (use_landlock) { +if (prctl (PR_SET_NO_NEW_PRIVS ,1 ,0 ,0 ,0 ) <0 ) +err (1 ,"prctl" ); + +if (syscall (SYS_landlock_restrict_self, ruleset_fd,0 ) <0 ) +err (1 ,"landlock_restrict_self" ); + } + +close (ruleset_fd); +return current; +} + +int main (int argc,char *argv[]) +{ +bool use_landlock = true; +size_t num_iterations =100000 ; +size_t num_subdirs =10000 ; +int c, current, fd; +struct tms start_time, end_time; + +setbuf (stdout,NULL ); +while ((c =getopt (argc, argv,"hLd:n:" )) !=-1 ) { +switch (c) { +case 'h' : +usage (argv[0 ]); +return EXIT_SUCCESS ; +case 'L' : + use_landlock = false; +break ; +case 'd' : + num_subdirs =atoi (optarg); +break ;
tools/testing/selftests/landlock/fs_bench.c line 131
Quoted 10 lines
tools/testing/selftests/landlock/fs_bench.c new file +10
-0+case 'n' : + num_iterations =atoi (optarg); +break ; +default : +usage (argv[0 ]); +return EXIT_FAILURE ; + } + } + +printf ("*** Benchmark ***\n" );
We should probably use ksft_*() helpers in main (see seccomp_benchmark.c).
tools/testing/selftests/landlock/fs_bench.c line 141
Quoted 10 lines
tools/testing/selftests/landlock/fs_bench.c new file +10
-0+printf ("%zu dirs, %zu iterations, %s landlock\n" , num_subdirs, + num_iterations, use_landlock ?"with" :"without" ); + +if (times (&start_time) ==-1 ) +err (1 ,"times" ); + + current =build_directory (num_subdirs, use_landlock); + +for (int i =0 ; i < num_iterations; i++) { + fd =openat (current,"." ,O_DIRECTORY );
We can use AT_EMPTY_PATH (with an empty path) instead of "." I guess the benchmark should not change, but better to check again.
tools/testing/selftests/landlock/fs_bench.c line 151
Quoted 10 lines
tools/testing/selftests/landlock/fs_bench.c new file +10
-0+if (fd !=-1 ) { +if (use_landlock) +errx (1 ,"openat succeeded, expected error" ); + +close (fd); + } + } + +if (times (&end_time) ==-1 ) +err (1 ,"times" );
The created directories should be removed here (setup and teardown).
Quoted 14 lines
tools/testing/selftests/landlock/fs_bench.c new file +10
-0+ +printf ("*** Benchmark concluded ***\n" ); +printf ("System: %ld clocks\n" , + end_time.tms_stime - start_time.tms_stime); +printf ("User : %ld clocks\n" , + end_time.tms_utime - start_time.tms_utime); +printf ("Clocks per second: %ld\n" ,CLOCKS_PER_SEC ); + +close (current); +}Signature · 1 line
2.52.0
Re: [PATCH v2 1/3] selftests/landlock: Add filesystem access benchmark
Hello!
Quoted 9 lines · Mickaël Salaün
tools/testing/selftests/landlock/fs_bench.c line 3
Quoted 6 lines · Günther Noack
tools/testing/selftests/landlock/fs_bench.c new file +3
-0@@ -0,0 +1,161 @@ +// SPDX-License-Identifier: GPL-2.0 +/* +* Landlock filesystem benchmark You might want to add some copyright.
Done.
Quoted 3 lines
tools/testing/selftests/landlock/fs_bench.c line 22
Quoted 1 line
tools/testing/selftests/landlock/fs_bench.c new file +1
-0+void usage (const char *argv0)const
Done.
Quoted 3 lines
tools/testing/selftests/landlock/fs_bench.c line 40
Quoted 1 line
tools/testing/selftests/landlock/fs_bench.c new file +1
-0+int build_directory (size_t depth,bool use_landlock)const
Done.
Quoted 7 lines
tools/testing/selftests/landlock/fs_bench.c line 47
Quoted 3 lines
tools/testing/selftests/landlock/fs_bench.c new file +3
-0+if (use_landlock) { + abi =syscall (SYS_landlock_create_ruleset,NULL ,0 , +LANDLOCK_CREATE_RULESET_VERSION );Please include
wrappers.hand use the related syscall helpers. One of the benefit is to use __NR_* constants defined by the installed kernel headers.
Done.
Quoted 28 lines
Quoted 28 lines
tools/testing/selftests/landlock/fs_bench.c new file +28
-0+if (abi <7 ) +err (1 ,"Landlock ABI too low: got %d, wanted 7+" , abi); + } + + ruleset_fd =-1 ; +if (use_landlock) { +struct landlock_ruleset_attr attr = { + .handled_access_fs = +0xffff ,/* All FS access rights as of 2026-01 */ + }; + ruleset_fd =syscall (SYS_landlock_create_ruleset, &attr, +sizeof (attr),0U ); +if (ruleset_fd <0 ) +err (1 ,"landlock_create_ruleset" ); + } + + current =open ("." ,O_PATH ); +if (current <0 ) +err (1 ,"open(.)" ); + +while (depth--) { +if (use_landlock) { +struct landlock_path_beneath_attr attr = { + .allowed_access =LANDLOCK_ACCESS_FS_IOCTL_DEV , + .parent_fd = current, + }; +if (syscall (SYS_landlock_add_rule, ruleset_fd, +LANDLOCK_RULE_PATH_BENEATH , &attr,0 ) <0 )
Quoted 8 lines
tools/testing/selftests/landlock/fs_bench.c line 80
Quoted 5 lines
tools/testing/selftests/landlock/fs_bench.c new file +5
-0+err (1 ,"landlock_add_rule" ); + } + +if (mkdirat (current, path,0700 ) <0 ) +err (1 ,"mkdirat(%s)" , path);We should have a loop to build the directories, then start the timer and have another loop to add Landlock rules.
I have to politely push back on this; the granularity of time measurement is not high enough and the measurement below only works because we repeat it 100000 times. This is not the case when we construct a Landlock ruleset, and it would IMHO be weird to build the ruleset multiple times as well. It feels like this would better be measured in a separate benchmark.
Adding a rule is an operation whose runtime does not depend on the depth of the nested directories, so such a separate benchmark would then also be simpler and wouldn't need to construct such a deeply nested hierarchy.
Quoted 4 lines
tools/testing/selftests/landlock/fs_bench.c line 131
Quoted 1 line
tools/testing/selftests/landlock/fs_bench.c new file +1
-0+printf ("*** Benchmark ***\n" );We should probably use ksft_*() helpers in main (see
seccomp_benchmark.c).
Among the benchmarks, the seccomp benchmark is the one exception in that it uses these ksft_*() helpers, and it's not clear to me that it has any benefit. These helpers are for producing TAP-formatted output, and assume that there will be individual test cases with success/failure results, which is not the case here. The seccomp test uses approximate assertions about the expected timing of operations (+-10%), but I don't think we can easily do that in our case.
I would therefore prefer to use a normal textual output format, similar to the other benchmarks in tools/testing/kselftests.
Quoted 5 lines
Quoted 5 lines
tools/testing/selftests/landlock/fs_bench.c new file +5
-0+printf ("%zu dirs, %zu iterations, %s landlock\n" , num_subdirs, + num_iterations, use_landlock ?"with" :"without" ); + +if (times (&start_time) ==-1 ) +err (1 ,"times" );
Quoted 8 lines
tools/testing/selftests/landlock/fs_bench.c line 141
Quoted 5 lines
tools/testing/selftests/landlock/fs_bench.c new file +5
-0+ + current =build_directory (num_subdirs, use_landlock); + +for (int i =0 ; i < num_iterations; i++) { + fd =openat (current,"." ,O_DIRECTORY );We can use
AT_EMPTY_PATH(with an empty path) instead of "." I guess the benchmark should not change, but better to check again.
This had to change anyway; now that I added cleanup of the created directories, I had to use another operation here that would trigger the path walk (file open for creation). Opening directories and removing directories both need to continue working so that we can later remove the directories. (See discussion below.)
Quoted 4 lines
Quoted 4 lines
tools/testing/selftests/landlock/fs_bench.c new file +4
-0+if (fd !=-1 ) { +if (use_landlock) +errx (1 ,"openat succeeded, expected error" ); +
Quoted 8 lines
tools/testing/selftests/landlock/fs_bench.c line 151
Quoted 6 lines
tools/testing/selftests/landlock/fs_bench.c new file +6
-0+close (fd); + } + } + +if (times (&end_time) ==-1 ) +err (1 ,"times" );The created directories should be removed here (setup and teardown).
Done.
Minor implementation remark: This is also done with explicit loops that use openat() to walk the directory tree with file descriptors and then unlinkat(fd, "d", ...). At this nesting depth, the paths don't fit into PATH_MAX any more and relative dirfds are the only way to do that AFAIK. (The directory walk function nftw(3) also breaks down FWIW, because it uses long paths relative to cwd.)
–Günther
Mickaël Salaün
maintainerRe: [PATCH v2 1/3] selftests/landlock: Add filesystem access benchmark
Quoted 51 lines · Günther Noack
Hello!
Quoted 32 lines · Mickaël Salaün
Quoted 31 lines · Günther Noack
tools/testing/selftests/landlock/fs_bench.c new file +28
-0+if (abi <7 ) +err (1 ,"Landlock ABI too low: got %d, wanted 7+" , abi); + } + + ruleset_fd =-1 ; +if (use_landlock) { +struct landlock_ruleset_attr attr = { + .handled_access_fs = +0xffff ,/* All FS access rights as of 2026-01 */ + }; + ruleset_fd =syscall (SYS_landlock_create_ruleset, &attr, +sizeof (attr),0U ); +if (ruleset_fd <0 ) +err (1 ,"landlock_create_ruleset" ); + } + + current =open ("." ,O_PATH ); +if (current <0 ) +err (1 ,"open(.)" ); + +while (depth--) { +if (use_landlock) { +struct landlock_path_beneath_attr attr = { + .allowed_access =LANDLOCK_ACCESS_FS_IOCTL_DEV , + .parent_fd = current, + }; +if (syscall (SYS_landlock_add_rule, ruleset_fd, +LANDLOCK_RULE_PATH_BENEATH , &attr,0 ) <0 )Quoted 8 lines
tools/testing/selftests/landlock/fs_bench.c line 80
Quoted 5 lines
tools/testing/selftests/landlock/fs_bench.c new file +5
-0+err (1 ,"landlock_add_rule" ); + } + +if (mkdirat (current, path,0700 ) <0 ) +err (1 ,"mkdirat(%s)" , path);We should have a loop to build the directories, then start the timer and have another loop to add Landlock rules.
I have to politely push back on this; the granularity of time measurement is not high enough and the measurement below only works because we repeat it 100000 times. This is not the case when we construct a Landlock ruleset, and it would IMHO be weird to build the ruleset multiple times as well. It feels like this would better be measured in a separate benchmark.
Quoted 4 lines
Adding a rule is an operation whose runtime does not depend on the depth of the nested directories, so such a separate benchmark would then also be simpler and wouldn't need to construct such a deeply nested hierarchy.
OK. Please add this explanation in a comment.
Quoted 15 lines
Quoted 6 lines
tools/testing/selftests/landlock/fs_bench.c line 131
Quoted 3 lines
tools/testing/selftests/landlock/fs_bench.c new file +1
-0+printf ("*** Benchmark ***\n" );We should probably use ksft_*() helpers in main (see
seccomp_benchmark.c).Among the benchmarks, the seccomp benchmark is the one exception in that it uses these ksft_*() helpers, and it's not clear to me that it has any benefit. These helpers are for producing TAP-formatted output, and assume that there will be individual test cases with success/failure results, which is not the case here. The seccomp test uses approximate assertions about the expected timing of operations (+-10%), but I don't think we can easily do that in our case.
Quoted 2 lines
I would therefore prefer to use a normal textual output format, similar to the other benchmarks in
tools/testing/kselftests.
OK
Quoted 47 lines
Quoted 15 lines
tools/testing/selftests/landlock/fs_bench.c line 141
Quoted 12 lines
tools/testing/selftests/landlock/fs_bench.c new file +10
-0+printf ("%zu dirs, %zu iterations, %s landlock\n" , num_subdirs, + num_iterations, use_landlock ?"with" :"without" ); + +if (times (&start_time) ==-1 ) +err (1 ,"times" ); + + current =build_directory (num_subdirs, use_landlock); + +for (int i =0 ; i < num_iterations; i++) { + fd =openat (current,"." ,O_DIRECTORY );We can use
AT_EMPTY_PATH(with an empty path) instead of "." I guess the benchmark should not change, but better to check again.This had to change anyway; now that I added cleanup of the created directories, I had to use another operation here that would trigger the path walk (file open for creation). Opening directories and removing directories both need to continue working so that we can later remove the directories. (See discussion below.)
Quoted 4 lines
Quoted 4 lines
tools/testing/selftests/landlock/fs_bench.c new file +4
-0+if (fd !=-1 ) { +if (use_landlock) +errx (1 ,"openat succeeded, expected error" ); +Quoted 8 lines
tools/testing/selftests/landlock/fs_bench.c line 151
Quoted 6 lines
tools/testing/selftests/landlock/fs_bench.c new file +6
-0+close (fd); + } + } + +if (times (&end_time) ==-1 ) +err (1 ,"times" );The created directories should be removed here (setup and teardown).
Done.
Minor implementation remark: This is also done with explicit loops that use
openat()to walk the directory tree with file descriptors and thenunlinkat(fd, "d", ...). At this nesting depth, the paths don't fit intoPATH_MAXany more and relative dirfds are the only way to do that AFAIK. (The directory walk function nftw(3) also breaks down FWIW, because it uses long paths relative to cwd.)–Günther
Re: [PATCH v2 1/3] selftests/landlock: Add filesystem access benchmark
Quoted 12 lines · Mickaël Salaün
Quoted 11 lines · Günther Noack
Quoted 2 lines · Mickaël Salaün
We should have a loop to build the directories, then start the timer and have another loop to add Landlock rules.
I have to politely push back on this; the granularity of time measurement is not high enough and the measurement below only works because we repeat it 100000 times. This is not the case when we construct a Landlock ruleset, and it would IMHO be weird to build the ruleset multiple times as well. It feels like this would better be measured in a separate benchmark.
Quoted 6 lines
Quoted 4 lines
Adding a rule is an operation whose runtime does not depend on the depth of the nested directories, so such a separate benchmark would then also be simpler and wouldn't need to construct such a deeply nested hierarchy.
OK. Please add this explanation in a comment.
Done.
–Günther
PATCH v2 2/3 landlock: access_mask_subset() helper
In reply to Günther NoackThis helper function checks whether an access_mask_t has a subset of the bits enabled than another one. This expresses the intent a bit smoother in the code and does not cost us anything when it gets inlined.
--- security/landlock/access.h | 6 ++++++ security/landlock/fs.c | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-)
security/landlock/access.h +6 -0
@@ -97,4 +97,10 @@ landlock_upgrade_handled_access_masks(struct access_masks access_masks) return access_masks; } +/** access_mask_subset - true iff a has a subset of the bits of b. */ +static inline bool access_mask_subset (access_mask_t a,access_mask_t b) +{ +return (a | b) == b; +} +#endif /* _SECURITY_LANDLOCK_ACCESS_H */
security/landlock/fs.c +1 -1
@@ -1704,7 +1704,7 @@ static int hook_file_open(struct file *const file) ARRAY_SIZE(layer_masks));#endif /* CONFIG_AUDIT */ -if ((open_access_request & allowed_access) == open_access_request) +if (access_mask_subset (open_access_request, allowed_access))return 0 ;/* Sets access to reflect the actual request. */
Signature · 1 line
2.52.0
Re: [PATCH v2 2/3] landlock: access_mask_subset() helper
Hi,
security/landlock/access.h landlock_upgrade_handled_access_masks(struct access_masks access_masks) · line 104
Quoted 13 lines · Günther Noack
security/landlock/access.h +5
-0@@ -97,4 +97,10 @@ landlock_upgrade_handled_access_masks(struct access_masks access_masks)return access_masks; } +/** access_mask_subset - true iff a has a subset of the bits of b. */ +static inline bool access_mask_subset (access_mask_t a,access_mask_t b) +{ +return (a | b) == b; +}
Don't use "/**" for comments that are not in kernel-doc format. This function doesn't need kernel-doc comments, so just use "/*" here, please.
Signature · 1 line
~Randy
Re: [PATCH v2 2/3] landlock: access_mask_subset() helper
Quoted 11 lines · Randy Dunlap
Hi,
Quoted 8 lines · Günther Noack
security/landlock/access.h +0
-0@@ -97,4 +97,10 @@ landlock_upgrade_handled_access_masks(struct access_masks access_masks)return access_masks; }
Quoted 9 lines
security/landlock/access.h landlock_upgrade_handled_access_masks(struct access_masks access_masks) · line 104
Quoted 5 lines
security/landlock/access.h +5
-0+/** access_mask_subset - true iff a has a subset of the bits of b. */ +static inline bool access_mask_subset (access_mask_t a,access_mask_t b) +{ +return (a | b) == b; +}Don't use "/**" for comments that are not in kernel-doc format. This function doesn't need kernel-doc comments, so just use "/*" here, please.
Thanks for the correction, will be fixed for next revision. :)
–Günther
Mickaël Salaün
maintainerRe: [PATCH v2 2/3] landlock: access_mask_subset() helper
In reply to Günther NoackQuoted 15 lines · Günther Noack
This helper function checks whether an
access_mask_thas a subset of the bits enabled than another one. This expresses the intent a bit smoother in the code and does not cost us anything when it gets inlined.Signed-off-by Günther Noack <[email protected]>--- security/landlock/access.h | 6 ++++++ security/landlock/fs.c | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-)security/landlock/access.h +0
-0
security/landlock/access.h landlock_upgrade_handled_access_masks(struct access_masks access_masks) · line 101
Quoted 5 lines
security/landlock/access.h +2
-0@@ -97,4 +97,10 @@ landlock_upgrade_handled_access_masks(struct access_masks access_masks)return access_masks; } +/** access_mask_subset - true iff a has a subset of the bits of b. */ +static inline bool access_mask_subset (access_mask_t a,access_mask_t b)
What about renaming "a" to "subset" and "b" to "superset"?
Quoted 22 lines
security/landlock/access.h +4
-0+{ +return (a | b) == b; +} +#endif /* _SECURITY_LANDLOCK_ACCESS_H */ security/landlock/fs.c +1
-1@@ -1704,7 +1704,7 @@ static int hook_file_open(struct file *const file) ARRAY_SIZE(layer_masks));#endif /* CONFIG_AUDIT */ -if ((open_access_request & allowed_access) == open_access_request) +if (access_mask_subset (open_access_request, allowed_access))return 0 ;/* Sets access to reflect the actual request. */ Signature · 1 line
2.52.0
Re: [PATCH v2 2/3] landlock: access_mask_subset() helper
Quoted 11 lines · Mickaël Salaün
security/landlock/access.h landlock_upgrade_handled_access_masks(struct access_masks access_masks) · line 101
Quoted 8 lines · Günther Noack
security/landlock/access.h +2
-0@@ -97,4 +97,10 @@ landlock_upgrade_handled_access_masks(struct access_masks access_masks)return access_masks; } +/** access_mask_subset - true iff a has a subset of the bits of b. */ +static inline bool access_mask_subset (access_mask_t a,access_mask_t b)What about renaming "a" to "subset" and "b" to "superset"?
Sure, sounds reasonable. Will be done in V3.
Quoted 3 lines
security/landlock/access.h landlock_upgrade_handled_access_masks(struct access_masks access_masks) · line 104
Quoted 3 lines
security/landlock/access.h +3
-0+{ +return (a | b) == b; +}
–Günther
PATCH v2 3/3 landlock: transpose the layer masks data structure
In reply to Günther NoackThe layer masks data structure tracks the requested but unfulfilled access rights during an operation's security check. It stores one bit for each combination of access right and layer index. If the bit is set, that access right is not granted (yet) in the given layer and we have to traverse the path further upwards to grant it.
Previously, the layer masks were stored as arrays mapping from access right indices to layer_mask_t. The layer_mask_t value then indicates all layers in which the given access right is still (tentatively) denied.
This patch introduces struct layer_access_masks instead: This struct contains an array with the access_mask_t of each (tentatively) denied access right in that layer.
The hypothesis of this patch is that this simplifies the code enough so that the resulting code will run faster:
* We can use bitwise operations in multiple places where we previously looped over bits individually with macros. (Should require less branch speculation and lends itself to better loop unrolling.)
* Code is ~75 lines smaller.
Other noteworthy changes:
* In no_more_access(), call a new helper function may_refer(), which only solves the asymmetric case. Previously, the code interleaved the checks for the two symmetric cases in RENAME_EXCHANGE. It feels that the code is clearer when renames without RENAME_EXCHANGE are more obviously the normal case.
Tradeoffs:
This change improves performance, at a slight size increase to the layer masks data structure.
At the moment, for the filesystem access rights, the data structure has the same size as before, but once we introduce the 17th filesystem access right, it will double in size (from 32 to 64 bytes), as access_mask_t grows from 16 to 32 bit. See the link below for measurements.
--- security/landlock/access.h | 10 +- security/landlock/audit.c | 84 +++------ security/landlock/audit.h | 3 +- security/landlock/domain.c | 45 +++-- security/landlock/domain.h | 4 +- security/landlock/fs.c | 352 ++++++++++++++++-------------------- security/landlock/net.c | 11 +- security/landlock/ruleset.c | 88 ++++----- security/landlock/ruleset.h | 21 ++- 9 files changed, 274 insertions(+), 344 deletions(-)
security/landlock/access.h +5 -5
@@ -61,14 +61,14 @@ union access_masks_all {static_assert (sizeof (typeof_member (union access_masks_all ,masks )) ==sizeof (typeof_member (union access_masks_all ,all ))); -typedef u16 layer_mask_t ; - -/* Makes sure all layers can be checked. */ -static_assert (BITS_PER_TYPE (layer_mask_t) >=LANDLOCK_MAX_NUM_LAYERS ); -/* * Tracks domains responsible of a denied access. This is required to avoid * storing in each object the full layer_masks[] required by update_request(). +* +* Each nibble represents the layer index of the newest layer which denied a +* certain access right. For file system access rights, the upper four bits are +* the index of the layer which denies LANDLOCK_ACCESS_FS_IOCTL_DEV and the +* lower nibble represents LANDLOCK_ACCESS_FS_TRUNCATE. */ typedef u8 deny_masks_t ;
security/landlock/audit.c +26 -58
145 lines not on this page · raw message
security/landlock/audit.h +1 -2
@@ -43,8 +43,7 @@ struct landlock_request {access_mask_t access;/* Required fields for requests with layer masks. */ -const layer_mask_t (*layer_masks)[]; -size_t layer_masks_size; +const struct layer_access_masks *masks;/* Required fields for requests with deny masks. */ const access_mask_t all_existing_optional_access;
security/landlock/domain.c +25 -20
91 lines not on this page · raw message
security/landlock/domain.h +2 -2
@@ -10,6 +10,7 @@#ifndef _SECURITY_LANDLOCK_DOMAIN_H#define _SECURITY_LANDLOCK_DOMAIN_H +#include "ruleset.h" #include <linux/limits.h> #include <linux/mm.h> #include <linux/path.h>
@@ -122,8 +123,7 @@ struct landlock_hierarchy {deny_masks_t landlock_get_deny_masks (const access_mask_t all_existing_optional_access,const access_mask_t optional_access, -const layer_mask_t (*const layer_masks)[], -size_t layer_masks_size); +const struct layer_access_masks *const masks);int landlock_init_hierarchy_log (struct landlock_hierarchy *const hierarchy);
security/landlock/fs.c +159 -193
539 lines not on this page · raw message
security/landlock/net.c +6 -5
@@ -47,7 +47,7 @@ static int current_check_access_socket(struct socket *const sock, access_mask_t access_request) {__be16 port; -layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_NET ] = {}; +struct layer_access_masks layer_masks = {};const struct landlock_rule *rule;struct landlock_id id = { .type =LANDLOCK_KEY_NET_PORT ,
@@ -194,8 +194,10 @@ static int current_check_access_socket(struct socket *const sock, access_request =landlock_init_layer_masks (subject->domain, access_request, &layer_masks,LANDLOCK_KEY_NET_PORT ); -if (landlock_unmask_layers (rule, access_request, &layer_masks, -ARRAY_SIZE (layer_masks))) +if (!access_request) +return 0 ; + +if (landlock_unmask_layers (rule, &layer_masks))return 0 ; audit_net.family = address->sa_family;
@@ -205,8 +207,7 @@ static int current_check_access_socket(struct socket *const sock, .audit.type = LSM_AUDIT_DATA_NET, .audit.u.net = &audit_net, .access = access_request, - .layer_masks = &layer_masks, - .layer_masks_size = ARRAY_SIZE(layer_masks), + .masks = &layer_masks, });return -EACCES ; }
security/landlock/ruleset.c +33 -55
146 lines not on this page · raw message
security/landlock/ruleset.h +17 -4
@@ -301,15 +301,28 @@ landlock_get_scope_mask(const struct landlock_ruleset *const ruleset, return ruleset->access_masks[layer_level].scope; } +/** +* struct layer_access_masks - A boolean matrix of layers and access rights +* +* This has a bit for each combination of layer numbers and access rights. +* During access checks, it is used to represent the access rights for each +* layer which still need to be fulfilled. When all bits are 0, the access +* request is considered to be fulfilled. +*/ +struct layer_access_masks { +/** +* @access: The unfulfilled access rights for each layer. +*/ +access_mask_t access[LANDLOCK_MAX_NUM_LAYERS ]; +}; +bool landlock_unmask_layers (const struct landlock_rule *const rule, -const access_mask_t access_request, -layer_mask_t (*const layer_masks)[], -const size_t masks_array_size); +struct layer_access_masks *masks);access_mask_t landlock_init_layer_masks (const struct landlock_ruleset *const domain,const access_mask_t access_request, -layer_mask_t (*const layer_masks)[], +struct layer_access_masks *masks,const enum landlock_key_type key_type);#endif /* _SECURITY_LANDLOCK_RULESET_H */
Signature · 1 line
2.52.0
Re: [PATCH v2 3/3] landlock: transpose the layer masks data structure
The first line here is confusing: "in @rule in @masks" Maybe:
Quoted 2 lines · Günther Noack
+/**
+ *landlock_unmask_layers- Cross off access rights granted in @rule in @masks
- Update (or Remove) access rights in @masks that are granted in @rules
?
Quoted 13 lines
*
- * Returns true if the request is allowed (i.e. relevant layer masks for the
- * request are empty).
+ * Updates the set of (per-layer) unfulfilled access rights @masks
+ * so that all the access rights granted in @rule are removed from it
+ * (because they are now fulfilled).
+ *
+ * @rule: A rule that grants a set of access rights for each layer
+ * @masks: A matrix of unfulfilled access rights for each layer
+ *
+ * Returns true if the request is allowed (i.e. the access rights granted all
+ * remaining unfulfilled access rights and masks has no leftover set bits). */
Signature · 1 line
~Randy
Re: [PATCH v2 3/3] landlock: transpose the layer masks data structure
Quoted 11 lines · Randy Dunlap
The first line here is confusing: "in @rule in @masks" Maybe:
Quoted 2 lines · Günther Noack
+/**
+ *landlock_unmask_layers- Cross off access rights granted in @rule in @masks- Update (or Remove) access rights in @masks that are granted in @rules
?
Thanks, that is a better wording indeed.
Will be included in next patch set version,
I think this is less grammatically ambiguous:
- * landlock_unmask_layers - Cross off access rights granted in @rule in @masks
+ * landlock_unmask_layers - Remove the access rights in @masks
+ * which are granted in @rule
–Günther
Re: [PATCH v2 3/3] landlock: transpose the layer masks data structure
Quoted 13 lines · Günther Noack
Quoted 11 lines · Randy Dunlap
The first line here is confusing: "in @rule in @masks" Maybe:
Quoted 2 lines · Günther Noack
+/**
+ *landlock_unmask_layers- Cross off access rights granted in @rule in @masks- Update (or Remove) access rights in @masks that are granted in @rules
?
Quoted 9 lines
Thanks, that is a better wording indeed.
Will be included in next patch set version,
I think this is less grammatically ambiguous:- *
landlock_unmask_layers- Cross off access rights granted in @rule in @masks
+ *landlock_unmask_layers- Remove the access rights in @masks
+ * which are granted in @rule
Ack, that's good. Thanks.
Signature · 1 line
~Randy
Mickaël Salaün
maintainerRe: [PATCH v2 3/3] landlock: transpose the layer masks data structure
In reply to Günther NoackQuoted 32 lines · Günther Noack
The layer masks data structure tracks the requested but unfulfilled access rights during an operation's security check. It stores one bit for each combination of access right and layer index. If the bit is set, that access right is not granted (yet) in the given layer and we have to traverse the path further upwards to grant it.
Previously, the layer masks were stored as arrays mapping from access right indices to
layer_mask_t. Thelayer_mask_tvalue then indicates all layers in which the given access right is still (tentatively) denied.This patch introduces struct
layer_access_masksinstead: This struct contains an array with theaccess_mask_tof each (tentatively) denied access right in that layer.The hypothesis of this patch is that this simplifies the code enough so that the resulting code will run faster:
* We can use bitwise operations in multiple places where we previously looped over bits individually with macros. (Should require less branch speculation and lends itself to better loop unrolling.)
* Code is ~75 lines smaller.
Other noteworthy changes:
* In
no_more_access(), call a new helper functionmay_refer(), which only solves the asymmetric case. Previously, the code interleaved the checks for the two symmetric cases inRENAME_EXCHANGE. It feels that the code is clearer when renames withoutRENAME_EXCHANGEare more obviously the normal case.
Quoted 8 lines
Tradeoffs:
This change improves performance, at a slight size increase to the layer masks data structure.
At the moment, for the filesystem access rights, the data structure has the same size as before, but once we introduce the 17th filesystem access right, it will double in size (from 32 to 64 bytes), as
...for all access rights (e.g. even if there is no new network one)
Quoted 46 lines
access_mask_tgrows from 16 to 32 bit. See the link below for measurements.Signed-off-by Günther Noack <[email protected]>--- security/landlock/access.h | 10 +- security/landlock/audit.c | 84 +++------ security/landlock/audit.h | 3 +- security/landlock/domain.c | 45 +++-- security/landlock/domain.h | 4 +- security/landlock/fs.c | 352 ++++++++++++++++-------------------- security/landlock/net.c | 11 +- security/landlock/ruleset.c | 88 ++++----- security/landlock/ruleset.h | 21 ++- 9 files changed, 274 insertions(+), 344 deletions(-)security/landlock/access.h +5
-5@@ -61,14 +61,14 @@ union access_masks_all {static_assert (sizeof (typeof_member (union access_masks_all ,masks )) ==sizeof (typeof_member (union access_masks_all ,all ))); -typedef u16 layer_mask_t ; - -/* Makes sure all layers can be checked. */ -static_assert (BITS_PER_TYPE (layer_mask_t) >=LANDLOCK_MAX_NUM_LAYERS ); -/* * Tracks domains responsible of a denied access. This is required to avoid * storing in each object the full layer_masks[] required by update_request(). +* +* Each nibble represents the layer index of the newest layer which denied a +* certain access right. For file system access rights, the upper four bits are +* the index of the layer which denies LANDLOCK_ACCESS_FS_IOCTL_DEV and the +* lower nibble represents LANDLOCK_ACCESS_FS_TRUNCATE. */ typedef u8 deny_masks_t ;security/landlock/audit.c +0
-0
security/landlock/audit.c static void test_get_hierarchy(struct kunit *const test) · line 183
Quoted 4 lines
security/landlock/audit.c +1
-0@@ -180,38 +180,21 @@ static void test_get_hierarchy(struct kunit *const test)#endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */ +/* get_denied_layer - get the youngest layer that denied the access_request */
/* Get the youngest layer that denied the access_request. */
Quoted 16 lines
security/landlock/audit.c +1
-12static size_t get_denied_layer (const struct landlock_ruleset *const domain,access_mask_t *const access_request, -const layer_mask_t (*const layer_masks)[], -const size_t layer_masks_size) +const struct layer_access_masks *masks) { -const unsigned long access_req = *access_request; -unsigned long access_bit; -access_mask_t missing =0 ; -long youngest_layer =-1 ; - -for_each_set_bit (access_bit, &access_req, layer_masks_size) { -const layer_mask_t mask = (*layer_masks)[access_bit]; -long layer; - -if (!mask)
security/landlock/audit.c static void test_get_hierarchy(struct kunit *const test) · line 188
Quoted 10 lines
security/landlock/audit.c +1
-9-continue ; - -/* __fls(1) == 0 */ - layer =__fls (mask); -if (layer > youngest_layer) { - youngest_layer = layer; - missing =BIT (access_bit); - }else if (layer == youngest_layer) { - missing |=BIT (access_bit); +for (int i =ARRAY_SIZE (masks->access) -1 ; i >=0 ; i--) {
size_t i
Quoted 163 lines
security/landlock/audit.c +23
-37+if (masks->access[i] & *access_request) { + *access_request &= masks->access[i]; +return i; } } - *access_request = missing; -if (youngest_layer ==-1 ) -return domain->num_layers -1 ; - -return youngest_layer; +/* Not found - fall back to default values */ + *access_request =0 ; +return domain->num_layers -1 ; }#ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST @@ -221,50 +204,39 @@ static void test_get_denied_layer(struct kunit *const test)const struct landlock_ruleset dom = { .num_layers =5 , }; -const layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS ] = { - [BIT_INDEX (LANDLOCK_ACCESS_FS_EXECUTE )] =BIT (0 ), - [BIT_INDEX (LANDLOCK_ACCESS_FS_READ_FILE )] =BIT (1 ), - [BIT_INDEX (LANDLOCK_ACCESS_FS_READ_DIR )] =BIT (1 ) |BIT (0 ), - [BIT_INDEX (LANDLOCK_ACCESS_FS_REMOVE_DIR )] =BIT (2 ), +const struct layer_access_masks masks = { + .access[0 ] =LANDLOCK_ACCESS_FS_EXECUTE | +LANDLOCK_ACCESS_FS_READ_DIR , + .access[1 ] =LANDLOCK_ACCESS_FS_READ_FILE | +LANDLOCK_ACCESS_FS_READ_DIR , + .access[2 ] =LANDLOCK_ACCESS_FS_REMOVE_DIR , };access_mask_t access; access =LANDLOCK_ACCESS_FS_EXECUTE ; -KUNIT_EXPECT_EQ (test,0 , -get_denied_layer (&dom, &access, &layer_masks, -sizeof (layer_masks))); +KUNIT_EXPECT_EQ (test,0 ,get_denied_layer (&dom, &access, &masks));KUNIT_EXPECT_EQ (test, access,LANDLOCK_ACCESS_FS_EXECUTE ); access =LANDLOCK_ACCESS_FS_READ_FILE ; -KUNIT_EXPECT_EQ (test,1 , -get_denied_layer (&dom, &access, &layer_masks, -sizeof (layer_masks))); +KUNIT_EXPECT_EQ (test,1 ,get_denied_layer (&dom, &access, &masks));KUNIT_EXPECT_EQ (test, access,LANDLOCK_ACCESS_FS_READ_FILE ); access =LANDLOCK_ACCESS_FS_READ_DIR ; -KUNIT_EXPECT_EQ (test,1 , -get_denied_layer (&dom, &access, &layer_masks, -sizeof (layer_masks))); +KUNIT_EXPECT_EQ (test,1 ,get_denied_layer (&dom, &access, &masks));KUNIT_EXPECT_EQ (test, access,LANDLOCK_ACCESS_FS_READ_DIR ); access =LANDLOCK_ACCESS_FS_READ_FILE |LANDLOCK_ACCESS_FS_READ_DIR ; -KUNIT_EXPECT_EQ (test,1 , -get_denied_layer (&dom, &access, &layer_masks, -sizeof (layer_masks))); +KUNIT_EXPECT_EQ (test,1 ,get_denied_layer (&dom, &access, &masks));KUNIT_EXPECT_EQ (test, access,LANDLOCK_ACCESS_FS_READ_FILE |LANDLOCK_ACCESS_FS_READ_DIR ); access =LANDLOCK_ACCESS_FS_EXECUTE |LANDLOCK_ACCESS_FS_READ_DIR ; -KUNIT_EXPECT_EQ (test,1 , -get_denied_layer (&dom, &access, &layer_masks, -sizeof (layer_masks))); +KUNIT_EXPECT_EQ (test,1 ,get_denied_layer (&dom, &access, &masks));KUNIT_EXPECT_EQ (test, access,LANDLOCK_ACCESS_FS_READ_DIR ); access =LANDLOCK_ACCESS_FS_WRITE_FILE ; -KUNIT_EXPECT_EQ (test,4 , -get_denied_layer (&dom, &access, &layer_masks, -sizeof (layer_masks))); +KUNIT_EXPECT_EQ (test,4 ,get_denied_layer (&dom, &access, &masks));KUNIT_EXPECT_EQ (test, access,0 ); }@@ -361,18 +333,15 @@ static bool is_valid_request(const struct landlock_request *const request)return false;if (request->access) { -if (WARN_ON_ONCE (!(!!request->layer_masks ^ +if (WARN_ON_ONCE (!(!!request->masks ^ !!request->all_existing_optional_access)))return false; }else { -if (WARN_ON_ONCE (request->layer_masks || +if (WARN_ON_ONCE (request->masks || request->all_existing_optional_access))return false; } -if (WARN_ON_ONCE (!!request->layer_masks ^ !!request->layer_masks_size)) -return false; -if (request->deny_masks) {if (WARN_ON_ONCE (!request->all_existing_optional_access))return false;@@ -405,13 +374,12 @@ void landlock_log_denial(const struct landlock_cred_security *const subject, missing = request->access;if (missing) {/* Gets the nearest domain that denies the request. */ -if (request->layer_masks) { +if (request->masks) { youngest_layer =get_denied_layer ( - subject->domain, &missing, request->layer_masks, - request->layer_masks_size); + subject->domain, &missing, request->masks); }else { youngest_layer =get_layer_from_deny_masks ( - &missing, request->all_existing_optional_access, + &missing, _LANDLOCK_ACCESS_FS_OPTIONAL, request->deny_masks); } youngest_denied =security/landlock/audit.h +1
-2@@ -43,8 +43,7 @@ struct landlock_request {access_mask_t access;/* Required fields for requests with layer masks. */ -const layer_mask_t (*layer_masks)[]; -size_t layer_masks_size; +const struct layer_access_masks *masks;/* Required fields for requests with deny masks. */ const access_mask_t all_existing_optional_access;security/landlock/domain.c +5
-4@@ -7,6 +7,7 @@* Copyright © 2024-2025 Microsoft Corporation */ +#include "ruleset.h" #include <kunit/test.h> #include <linux/bitops.h> #include <linux/bits.h> @@ -182,32 +183,36 @@ static void test_get_layer_deny_mask(struct kunit *const test)deny_masks_t landlock_get_deny_masks (const access_mask_t all_existing_optional_access,const access_mask_t optional_access, -const layer_mask_t (*const layer_masks)[], -const size_t layer_masks_size) +const struct layer_access_masks *const masks) {const unsigned long access_opt = optional_access;unsigned long access_bit; +access_mask_t all_denied =0 ;deny_masks_t deny_masks =0 ;/* This may require change with new object types. */ -WARN_ON_ONCE (access_opt != - (optional_access & all_existing_optional_access)); +WARN_ON_ONCE (!access_mask_subset (optional_access, + all_existing_optional_access));
security/landlock/domain.c static void test_get_layer_deny_mask(struct kunit *const test) · line 203
Quoted 10 lines
security/landlock/domain.c +2
-3-if (WARN_ON_ONCE (!layer_masks)) +if (WARN_ON_ONCE (!masks))return 0 ;if (WARN_ON_ONCE (!access_opt))return 0 ; -for_each_set_bit (access_bit, &access_opt, layer_masks_size) { -const layer_mask_t mask = (*layer_masks)[access_bit]; +for (int i =ARRAY_SIZE (masks->access) -1 ; i >=0 ; i--) {
size_t i
Quoted 251 lines
security/landlock/domain.c +18
-13+const access_mask_t denied = masks->access[i] & optional_access; +const unsigned long newly_denied = denied & ~all_denied; -if (!mask) +if (!newly_denied)continue ; -/* __fls(1) == 0 */ - deny_masks |=get_layer_deny_mask (all_existing_optional_access, - access_bit,__fls (mask)); +for_each_set_bit (access_bit, &newly_denied, +8 *sizeof (access_mask_t)) { + deny_masks |=get_layer_deny_mask ( + all_existing_optional_access, access_bit, i); + } + all_denied |= denied; }return deny_masks; }@@ -216,28 +221,28 @@ landlock_get_deny_masks(const access_mask_t all_existing_optional_access,static void test_landlock_get_deny_masks (struct kunit *const test) { -const layer_mask_t layers1[BITS_PER_TYPE (access_mask_t)] = { - [BIT_INDEX (LANDLOCK_ACCESS_FS_EXECUTE )] =BIT_ULL (0 ) | -BIT_ULL (9 ), - [BIT_INDEX (LANDLOCK_ACCESS_FS_TRUNCATE )] =BIT_ULL (1 ), - [BIT_INDEX (LANDLOCK_ACCESS_FS_IOCTL_DEV )] =BIT_ULL (2 ) | -BIT_ULL (0 ), +const struct layer_access_masks layers1 = { + .access[0 ] =LANDLOCK_ACCESS_FS_EXECUTE | +LANDLOCK_ACCESS_FS_IOCTL_DEV , + .access[1 ] =LANDLOCK_ACCESS_FS_TRUNCATE , + .access[2 ] =LANDLOCK_ACCESS_FS_IOCTL_DEV , + .access[9 ] =LANDLOCK_ACCESS_FS_EXECUTE , };KUNIT_EXPECT_EQ (test,0x1 ,landlock_get_deny_masks (_LANDLOCK_ACCESS_FS_OPTIONAL,LANDLOCK_ACCESS_FS_TRUNCATE , - &layers1,ARRAY_SIZE (layers1))); + &layers1));KUNIT_EXPECT_EQ (test,0x20 ,landlock_get_deny_masks (_LANDLOCK_ACCESS_FS_OPTIONAL,LANDLOCK_ACCESS_FS_IOCTL_DEV , - &layers1,ARRAY_SIZE (layers1))); + &layers1));KUNIT_EXPECT_EQ ( test,0x21 ,landlock_get_deny_masks (_LANDLOCK_ACCESS_FS_OPTIONAL,LANDLOCK_ACCESS_FS_TRUNCATE |LANDLOCK_ACCESS_FS_IOCTL_DEV , - &layers1,ARRAY_SIZE (layers1))); + &layers1)); }#endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */ security/landlock/domain.h +2
-2@@ -10,6 +10,7 @@#ifndef _SECURITY_LANDLOCK_DOMAIN_H#define _SECURITY_LANDLOCK_DOMAIN_H +#include "ruleset.h" #include <linux/limits.h> #include <linux/mm.h> #include <linux/path.h> @@ -122,8 +123,7 @@ struct landlock_hierarchy {deny_masks_t landlock_get_deny_masks (const access_mask_t all_existing_optional_access,const access_mask_t optional_access, -const layer_mask_t (*const layer_masks)[], -size_t layer_masks_size); +const struct layer_access_masks *const masks);int landlock_init_hierarchy_log (struct landlock_hierarchy *const hierarchy);security/landlock/fs.c +61
-67@@ -398,57 +398,55 @@ static const struct access_masks any_fs = { .fs = ~0, }; +/* +* Returns true iff the child file with the given src_child access rights under +* src_parent would result in having the same or fewer access rights if it were +* moved under new_parent. +*/ +static bool may_refer (const struct layer_access_masks *const src_parent, +const struct layer_access_masks *const src_child, +const struct layer_access_masks *const new_parent, +const bool child_is_dir) +{ +for (size_t i =0 ; i <ARRAY_SIZE (new_parent->access); i++) { +access_mask_t child_access = src_parent->access[i] & + src_child->access[i]; +access_mask_t parent_access = new_parent->access[i]; + +if (!child_is_dir) { + child_access &=ACCESS_FILE ; + parent_access &=ACCESS_FILE ; + } + +if (!access_mask_subset (child_access, parent_access)) +return false; + } +return true; +} +/* * Check that a destination file hierarchy has more restrictions than a source * file hierarchy. This is only used for link and rename actions. * -* @layer_masks_child2: Optional child masks. +* Returns: true if child1 may be moved from parent1 to parent2 without +* increasing its access rights. If child2 is set, an additional condition is +* that child2 may be used from parent2 to parent1 without increasing its access +* rights. */ -static bool no_more_access ( -const layer_mask_t (*const layer_masks_parent1)[LANDLOCK_NUM_ACCESS_FS ], -const layer_mask_t (*const layer_masks_child1)[LANDLOCK_NUM_ACCESS_FS ], -const bool child1_is_directory, -const layer_mask_t (*const layer_masks_parent2)[LANDLOCK_NUM_ACCESS_FS ], -const layer_mask_t (*const layer_masks_child2)[LANDLOCK_NUM_ACCESS_FS ], -const bool child2_is_directory) +static bool no_more_access (const struct layer_access_masks *const parent1, +const struct layer_access_masks *const child1, +const bool child1_is_dir, +const struct layer_access_masks *const parent2, +const struct layer_access_masks *const child2, +const bool child2_is_dir) { -unsigned long access_bit; +if (!may_refer (parent1, child1, parent2, child1_is_dir)) +return false; -for (access_bit =0 ; access_bit <ARRAY_SIZE (*layer_masks_parent2); - access_bit++) { -/* Ignores accesses that only make sense for directories. */ -const bool is_file_access = - !!(BIT_ULL (access_bit) &ACCESS_FILE ); +if (!child2) +return true; -if (child1_is_directory || is_file_access) { -/* -* Checks if the destination restrictions are a -* superset of the source ones (i.e. inherited access -* rights without child exceptions): -* restrictions(parent2) >= restrictions(child1) -*/ -if ((((*layer_masks_parent1)[access_bit] & - (*layer_masks_child1)[access_bit]) | - (*layer_masks_parent2)[access_bit]) != - (*layer_masks_parent2)[access_bit]) -return false; - } - -if (!layer_masks_child2) -continue ; -if (child2_is_directory || is_file_access) { -/* -* Checks inverted restrictions for RENAME_EXCHANGE: -* restrictions(parent1) >= restrictions(child2) -*/ -if ((((*layer_masks_parent2)[access_bit] & - (*layer_masks_child2)[access_bit]) | - (*layer_masks_parent1)[access_bit]) != - (*layer_masks_parent1)[access_bit]) -return false; - } - } -return true; +return may_refer (parent2, child2, parent1, child2_is_dir); }#define NMA_TRUE (...) KUNIT_EXPECT_TRUE(test, no_more_access(__VA_ARGS__))@@ -458,25 +456,25 @@ static bool no_more_access(static void test_no_more_access (struct kunit *const test) { -const layer_mask_t rx0[LANDLOCK_NUM_ACCESS_FS ] = { - [BIT_INDEX (LANDLOCK_ACCESS_FS_EXECUTE )] =BIT_ULL (0 ), - [BIT_INDEX (LANDLOCK_ACCESS_FS_READ_FILE )] =BIT_ULL (0 ), +const struct layer_access_masks rx0 = { + .access[0 ] =LANDLOCK_ACCESS_FS_EXECUTE | +LANDLOCK_ACCESS_FS_READ_FILE , }; -const layer_mask_t mx0[LANDLOCK_NUM_ACCESS_FS ] = { - [BIT_INDEX (LANDLOCK_ACCESS_FS_EXECUTE )] =BIT_ULL (0 ), - [BIT_INDEX (LANDLOCK_ACCESS_FS_MAKE_REG )] =BIT_ULL (0 ), +const struct layer_access_masks mx0 = { + .access[0 ] =LANDLOCK_ACCESS_FS_EXECUTE | +LANDLOCK_ACCESS_FS_MAKE_REG , }; -const layer_mask_t x0[LANDLOCK_NUM_ACCESS_FS ] = { - [BIT_INDEX (LANDLOCK_ACCESS_FS_EXECUTE )] =BIT_ULL (0 ), +const struct layer_access_masks x0 = { + .access[0 ] =LANDLOCK_ACCESS_FS_EXECUTE , }; -const layer_mask_t x1[LANDLOCK_NUM_ACCESS_FS ] = { - [BIT_INDEX (LANDLOCK_ACCESS_FS_EXECUTE )] =BIT_ULL (1 ), +const struct layer_access_masks x1 = { + .access[1 ] =LANDLOCK_ACCESS_FS_EXECUTE , }; -const layer_mask_t x01[LANDLOCK_NUM_ACCESS_FS ] = { - [BIT_INDEX (LANDLOCK_ACCESS_FS_EXECUTE )] =BIT_ULL (0 ) | -BIT_ULL (1 ), +const struct layer_access_masks x01 = { + .access[0 ] =LANDLOCK_ACCESS_FS_EXECUTE , + .access[1 ] =LANDLOCK_ACCESS_FS_EXECUTE , }; -const layer_mask_t allows_all[LANDLOCK_NUM_ACCESS_FS ] = {}; +const struct layer_access_masks allows_all = {};/* Checks without restriction. */ NMA_TRUE (&x0, &allows_all, false, &allows_all,NULL , false);@@ -564,31 +562,30 @@ static void test_no_more_access(struct kunit *const test)#undef NMA_TRUE#undef NMA_FALSE -static bool is_layer_masks_allowed ( -layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS ]) +static bool is_layer_masks_allowed (const struct layer_access_masks *masks) { -return !memchr_inv (layer_masks,0 ,sizeof (*layer_masks)); +return !memchr_inv (&masks->access,0 ,sizeof (masks->access)); }/* -* Removes @layer_masks accesses that are not requested. +* Removes @masks accesses that are not requested. * * Returns true if the request is allowed, false otherwise. */ -static bool -scope_to_request (const access_mask_t access_request, -layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS ]) +static bool scope_to_request (const access_mask_t access_request, +struct layer_access_masks *masks) { -const unsigned long access_req = access_request; -unsigned long access_bit; +bool saw_unfulfilled_access = false; -if (WARN_ON_ONCE (!layer_masks))
security/landlock/fs.c static void test_no_more_access(struct kunit *const test) · line 585
Quoted 10 lines
security/landlock/fs.c +4
-4+if (WARN_ON_ONCE (!masks))return true; -for_each_clear_bit (access_bit, &access_req,ARRAY_SIZE (*layer_masks)) - (*layer_masks)[access_bit] =0 ; - -return is_layer_masks_allowed (layer_masks); +for (size_t i =0 ; i <ARRAY_SIZE (masks->access); i++) { + masks->access[i] &= access_request; +if (masks->access[i])
{
security/landlock/fs.c static void test_no_more_access(struct kunit *const test) · line 586
Quoted 1 line
security/landlock/fs.c +1
-0+ saw_unfulfilled_access = true;
break;
}
Quoted 5 lines
security/landlock/fs.c +2
-0+ } +return !saw_unfulfilled_access; }#ifdef CONFIG_SECURITY_LANDLOCK_KUNIT_TEST
Re: [PATCH v2 3/3] landlock: transpose the layer masks data structure
Quoted 11 lines · Mickaël Salaün
Quoted 8 lines · Günther Noack
Tradeoffs:
This change improves performance, at a slight size increase to the layer masks data structure.
At the moment, for the filesystem access rights, the data structure has the same size as before, but once we introduce the 17th filesystem access right, it will double in size (from 32 to 64 bytes), as
...for all access rights (e.g. even if there is no new network one)
Added.
Quoted 9 lines
security/landlock/audit.c static void test_get_hierarchy(struct kunit *const test) · line 183
Quoted 7 lines
security/landlock/audit.c +1
-0@@ -180,38 +180,21 @@ static void test_get_hierarchy(struct kunit *const test)#endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */ +/* get_denied_layer - get the youngest layer that denied the access_request */ /* Get the youngest layer that denied the
access_request. */
OK, done. I also changed to non-docstring style for the access_mask_subset() helper.
Quoted 21 lines
Quoted 21 lines
security/landlock/audit.c +1
-16static size_t get_denied_layer (const struct landlock_ruleset *const domain,access_mask_t *const access_request, -const layer_mask_t (*const layer_masks)[], -const size_t layer_masks_size) +const struct layer_access_masks *masks) { -const unsigned long access_req = *access_request; -unsigned long access_bit; -access_mask_t missing =0 ; -long youngest_layer =-1 ; - -for_each_set_bit (access_bit, &access_req, layer_masks_size) { -const layer_mask_t mask = (*layer_masks)[access_bit]; -long layer; - -if (!mask) -continue ; - -/* __fls(1) == 0 */ - layer =__fls (mask);
Quoted 8 lines
security/landlock/audit.c static void test_get_hierarchy(struct kunit *const test) · line 188
Quoted 6 lines
security/landlock/audit.c +1
-5-if (layer > youngest_layer) { - youngest_layer = layer; - missing =BIT (access_bit); - }else if (layer == youngest_layer) { - missing |=BIT (access_bit); +for (int i =ARRAY_SIZE (masks->access) -1 ; i >=0 ; i--) {
size_ti
This is one of the two places where this didn't work.
The loop goes from top to bottom here, and the "i >= 0" check would always be true for a size_t.
If there is a more idiomatic way to write that loop, I can switch to it, but would otherwise lean towards keeping it as it is?
Quoted 11 lines
security/landlock/audit.c static void test_get_hierarchy(struct kunit *const test)
Quoted 9 lines
security/landlock/audit.c +4
-2+if (masks->access[i] & *access_request) { + *access_request &= masks->access[i]; +return i; } } -for_each_set_bit (access_bit, &access_opt, layer_masks_size) { -const layer_mask_t mask = (*layer_masks)[access_bit]; +for (int i =ARRAY_SIZE (masks->access) -1 ; i >=0 ; i--) {
size_ti
Ditto, the loop goes from top to bottom here.
Quoted 29 lines
Quoted 16 lines
security/landlock/audit.c +5
-6+const access_mask_t denied = masks->access[i] & optional_access; +const unsigned long newly_denied = denied & ~all_denied; -static bool -scope_to_request (const access_mask_t access_request, -layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS ]) +static bool scope_to_request (const access_mask_t access_request, +struct layer_access_masks *masks) { -const unsigned long access_req = access_request; -unsigned long access_bit; +bool saw_unfulfilled_access = false; -if (WARN_ON_ONCE (!layer_masks))security/landlock/audit.c static void test_get_hierarchy(struct kunit *const test)
Quoted 10 lines
security/landlock/audit.c +4
-4+if (WARN_ON_ONCE (!masks))return true; -for_each_clear_bit (access_bit, &access_req,ARRAY_SIZE (*layer_masks)) - (*layer_masks)[access_bit] =0 ; - -return is_layer_masks_allowed (layer_masks); +for (size_t i =0 ; i <ARRAY_SIZE (masks->access); i++) { + masks->access[i] &= access_request; +if (masks->access[i]){
Quoted 4 lines
security/landlock/audit.c
Quoted 1 line
security/landlock/audit.c +1
-0+ saw_unfulfilled_access = true;break;
}
Two lines above, this loop mutates masks->access[...]:
masks->access[i] &= access_request
If we break the loop early, we would not actually scope it down to the request entirely? Is this safe?
Quoted 3 lines
security/landlock/audit.c
Quoted 3 lines
security/landlock/audit.c +2
-0+ } +return !saw_unfulfilled_access; }
–Günther
Mickaël Salaün
maintainerRe: [PATCH v2 3/3] landlock: transpose the layer masks data structure
Quoted 58 lines · Günther Noack
Quoted 11 lines · Mickaël Salaün
Quoted 8 lines · Günther Noack
Tradeoffs:
This change improves performance, at a slight size increase to the layer masks data structure.
At the moment, for the filesystem access rights, the data structure has the same size as before, but once we introduce the 17th filesystem access right, it will double in size (from 32 to 64 bytes), as
...for all access rights (e.g. even if there is no new network one)
Added.
Quoted 9 lines
security/landlock/audit.c static void test_get_hierarchy(struct kunit *const test) · line 183
Quoted 7 lines
security/landlock/audit.c +1
-0@@ -180,38 +180,21 @@ static void test_get_hierarchy(struct kunit *const test)#endif /* CONFIG_SECURITY_LANDLOCK_KUNIT_TEST */ +/* get_denied_layer - get the youngest layer that denied the access_request */ /* Get the youngest layer that denied the
access_request. */OK, done. I also changed to non-docstring style for the
access_mask_subset()helper.Quoted 21 lines
Quoted 21 lines
security/landlock/audit.c +1
-16static size_t get_denied_layer (const struct landlock_ruleset *const domain,access_mask_t *const access_request, -const layer_mask_t (*const layer_masks)[], -const size_t layer_masks_size) +const struct layer_access_masks *masks) { -const unsigned long access_req = *access_request; -unsigned long access_bit; -access_mask_t missing =0 ; -long youngest_layer =-1 ; - -for_each_set_bit (access_bit, &access_req, layer_masks_size) { -const layer_mask_t mask = (*layer_masks)[access_bit]; -long layer; - -if (!mask) -continue ; - -/* __fls(1) == 0 */ - layer =__fls (mask);Quoted 8 lines
security/landlock/audit.c static void test_get_hierarchy(struct kunit *const test) · line 188
Quoted 6 lines
security/landlock/audit.c +1
-5-if (layer > youngest_layer) { - youngest_layer = layer; - missing =BIT (access_bit); - }else if (layer == youngest_layer) { - missing |=BIT (access_bit); +for (int i =ARRAY_SIZE (masks->access) -1 ; i >=0 ; i--) {
size_ti
Quoted 7 lines
This is one of the two places where this didn't work.
The loop goes from top to bottom here, and the "i >= 0" check would always be true for a
size_t.If there is a more idiomatic way to write that loop, I can switch to it, but would otherwise lean towards keeping it as it is?
Indeed. We can use ssize_t as in get_hierarchy().
Quoted 51 lines
Quoted 8 lines
Quoted 8 lines
security/landlock/audit.c +3
-0+if (masks->access[i] & *access_request) { + *access_request &= masks->access[i]; +return i; } }Quoted 5 lines
security/landlock/audit.c static void test_get_hierarchy(struct kunit *const test) · line 195
Quoted 3 lines
security/landlock/audit.c +1
-2-for_each_set_bit (access_bit, &access_opt, layer_masks_size) { -const layer_mask_t mask = (*layer_masks)[access_bit]; +for (int i =ARRAY_SIZE (masks->access) -1 ; i >=0 ; i--) {
size_tiDitto, the loop goes from top to bottom here.
Quoted 29 lines
Quoted 16 lines
security/landlock/audit.c +5
-6+const access_mask_t denied = masks->access[i] & optional_access; +const unsigned long newly_denied = denied & ~all_denied; -static bool -scope_to_request (const access_mask_t access_request, -layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS ]) +static bool scope_to_request (const access_mask_t access_request, +struct layer_access_masks *masks) { -const unsigned long access_req = access_request; -unsigned long access_bit; +bool saw_unfulfilled_access = false; -if (WARN_ON_ONCE (!layer_masks))security/landlock/audit.c static void test_get_hierarchy(struct kunit *const test)
Quoted 10 lines
security/landlock/audit.c +4
-4+if (WARN_ON_ONCE (!masks))return true; -for_each_clear_bit (access_bit, &access_req,ARRAY_SIZE (*layer_masks)) - (*layer_masks)[access_bit] =0 ; - -return is_layer_masks_allowed (layer_masks); +for (size_t i =0 ; i <ARRAY_SIZE (masks->access); i++) { + masks->access[i] &= access_request; +if (masks->access[i]){
Quoted 4 lines
security/landlock/audit.c
Quoted 1 line
security/landlock/audit.c +1
-0+ saw_unfulfilled_access = true;break;
}
Quoted 6 lines
Two lines above, this loop mutates masks->access[...]:
masks->access[i] &=
access_requestIf we break the loop early, we would not actually scope it down to the request entirely? Is this safe?
You're right, don't add this break. BTW, would a test catch it?
Quoted 7 lines
Quoted 4 lines
security/landlock/audit.c
Quoted 4 lines
security/landlock/audit.c +2
-0+ } +return !saw_unfulfilled_access; }–Günther
Re: [PATCH v2 3/3] landlock: transpose the layer masks data structure
Quoted 8 lines · Mickaël Salaün
Quoted 7 lines · Günther Noack
Quoted 3 lines · Mickaël Salaün
Review
Quoted 1 line
(quoted diff) +1
-0+ for (int i = ARRAY_SIZE(masks->access) - 1; i >= 0; i--) {
size_tiThis is one of the two places where this didn't work.
Quoted 7 lines
Quoted 5 lines
The loop goes from top to bottom here, and the "i >= 0" check would always be true for a
size_t.If there is a more idiomatic way to write that loop, I can switch to it, but would otherwise lean towards keeping it as it is?
Indeed. We can use
ssize_tas inget_hierarchy().
Good point, done.
Quoted 29 lines
Quoted 29 lines
Quoted 28 lines
Quoted 11 lines
security/landlock/fs.c +3
-6-static bool -scope_to_request (const access_mask_t access_request, -layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS ]) +static bool scope_to_request (const access_mask_t access_request, +struct layer_access_masks *masks) { -const unsigned long access_req = access_request; -unsigned long access_bit; +bool saw_unfulfilled_access = false; -if (WARN_ON_ONCE (!layer_masks))security/landlock/fs.c static void test_no_more_access(struct kunit *const test) · line 585
Quoted 10 lines
security/landlock/fs.c +4
-4+if (WARN_ON_ONCE (!masks))return true; -for_each_clear_bit (access_bit, &access_req,ARRAY_SIZE (*layer_masks)) - (*layer_masks)[access_bit] =0 ; - -return is_layer_masks_allowed (layer_masks); +for (size_t i =0 ; i <ARRAY_SIZE (masks->access); i++) { + masks->access[i] &= access_request; +if (masks->access[i]){
security/landlock/fs.c static void test_no_more_access(struct kunit *const test) · line 586
Quoted 1 line
security/landlock/fs.c +1
-0+ saw_unfulfilled_access = true;break;
}
Quoted 8 lines
Quoted 6 lines
Two lines above, this loop mutates masks->access[...]:
masks->access[i] &=
access_requestIf we break the loop early, we would not actually scope it down to the request entirely? Is this safe?
You're right, don't add this break. BTW, would a test catch it?
Yes, the existing tests already catch that; this happens when we break early:
[08:53:12] ================= landlock_fs (7 subtests) =================
[08:53:12] [PASSED] test_no_more_access
[08:53:12] [PASSED] test_scope_to_request_with_exec_none
[08:53:12] # test_scope_to_request_with_exec_some: EXPECTATION FAILED at security/landlock/fs.c:616 [08:53:12] Expected 0 == masks.access[1], but
[08:53:12] masks.access[1] == 2 (0x2)
[08:53:12] [FAILED] test_scope_to_request_with_exec_some
[08:53:12] [PASSED] test_scope_to_request_without_access
[08:53:12] [PASSED] test_is_eacces_with_none
[08:53:12] [PASSED] test_is_eacces_with_refer
[08:53:12] [PASSED] test_is_eacces_with_write
[08:53:12] # module: landlock
[08:53:12] # landlock_fs: pass:6 fail:1 skip:0 total:7
[08:53:12] # Totals: pass:6 fail:1 skip:0 total:7
Good coverage!
–Günther
Mickaël Salaün
maintainerRe: [PATCH v2 3/3] landlock: transpose the layer masks data structure
In reply to Günther NoackQuoted 37 lines · Günther Noack
The layer masks data structure tracks the requested but unfulfilled access rights during an operation's security check. It stores one bit for each combination of access right and layer index. If the bit is set, that access right is not granted (yet) in the given layer and we have to traverse the path further upwards to grant it.
Previously, the layer masks were stored as arrays mapping from access right indices to
layer_mask_t. Thelayer_mask_tvalue then indicates all layers in which the given access right is still (tentatively) denied.This patch introduces struct
layer_access_masksinstead: This struct contains an array with theaccess_mask_tof each (tentatively) denied access right in that layer.The hypothesis of this patch is that this simplifies the code enough so that the resulting code will run faster:
* We can use bitwise operations in multiple places where we previously looped over bits individually with macros. (Should require less branch speculation and lends itself to better loop unrolling.)
* Code is ~75 lines smaller.
Other noteworthy changes:
* In
no_more_access(), call a new helper functionmay_refer(), which only solves the asymmetric case. Previously, the code interleaved the checks for the two symmetric cases inRENAME_EXCHANGE. It feels that the code is clearer when renames withoutRENAME_EXCHANGEare more obviously the normal case.Tradeoffs:
This change improves performance, at a slight size increase to the layer masks data structure.
Quoted 7 lines
At the moment, for the filesystem access rights, the data structure has the same size as before, but once we introduce the 17th filesystem access right, it will double in size (from 32 to 64 bytes), as
access_mask_tgrows from 16 to 32 bit. See the link below for measurements.
When adding extra links, please add a [1] reference at the end and use this reference in the commit message.
Quoted 12 lines
Signed-off-by Günther Noack <[email protected]>--- security/landlock/access.h | 10 +- security/landlock/audit.c | 84 +++------ security/landlock/audit.h | 3 +- security/landlock/domain.c | 45 +++-- security/landlock/domain.h | 4 +- security/landlock/fs.c | 352 ++++++++++++++++-------------------- security/landlock/net.c | 11 +- security/landlock/ruleset.c | 88 ++++----- security/landlock/ruleset.h | 21 ++- 9 files changed, 274 insertions(+), 344 deletions(-)
Re: [PATCH v2 3/3] landlock: transpose the layer masks data structure
Quoted 11 lines · Mickaël Salaün
Quoted 7 lines · Günther Noack
At the moment, for the filesystem access rights, the data structure has the same size as before, but once we introduce the 17th filesystem access right, it will double in size (from 32 to 64 bytes), as
access_mask_tgrows from 16 to 32 bit. See the link below for measurements.When adding extra links, please add a [1] reference at the end and use this reference in the commit message.
Ack, done.
–Günther
Mickaël Salaün
maintainerQuoted 57 lines · Günther Noack
Hello!
This patch set "transposes" the layer masks matrix, which was previously modeled as a access-max-sized array of layer masks, and changes it to be a layer-max-sized array of access masks instead. (It is a pure refactoring, there are no user-visible changes.)
This unlocks a few code simplifications and in multiple places it removes the need for loops and branches that deal with individual bits. Instead, the changed data structure now lends itself for more bitwise operations. The underlying hypothesis for me was that by using more bitwise operations and fewer branches, we would get an overall speedup even when the data structure size increases slightly in some cases.
Tentative results with and without this patch set show that the hypothesis likely holds true. The benchmark I used exercises a "worst case" scenario that attempts to be bottlenecked on the affected code:
constructs a large number of nested directories, with one "path beneath" rule each and then tries to open the innermost directory many times. The benchmark is intentionally unrealistic to amplify the amount of time used for the path walk logic and forces Landlock to walk the full path (eventually failing the open syscall). (I'll send the benchmark program in a reply to this mail for full transparency.)Measured with the benchmark program, the patch set results in a speedup of about -10%. The benchmark results are only tentative and have been produced in Qemu:
With the patch, the benchmark runs in 6046 clocks (measured with times(3)):
*** Benchmark ***
10000 dirs, 100000 iterations, with landlock
*** Benchmark concluded ***
System: 6046 clocks
User : 1 clocks
Clocks per second: 1000000Without the patch, we get 6713 clocks, which is 11% more
*** Benchmark ***
10000 dirs, 100000 iterations, with landlock
*** Benchmark concluded ***
System: 6713 clocks
User : 0 clocks
Clocks per second: 1000000The base revision used for benchmarking was commit
7a51784da76d("tools/sched_ext: updatescx_show_state.pyforscx_abortingchange")In real-life scenarios, the speed improvement from this patch set will be less pronounced than in the artificial benchmark, as people do not usually stack directories that deeply and attach so many rules to them, and the EACCES error should also be the exception rather than the norm.
Quoted 6 lines
I am looking forward to your feedback.
P.S.: I am open to suggestions on what the "layer masks" variables should be called, because the name "layer masks" might be less appropriate after this change. I have not fixed up the name everywhere because fixing up the code took priority for now.
Could you please clarify your thoughts and explain why this name might not be appropriate anymore? Any list of name proposals?
If we rename the variables, this should be done in the same refactoring patch.
Quoted 8 lines
---
Changes since previous versions:V2: (This patch set)
* Remove the refactoring around the
deny_mask_ttype, it is better to send that as a separate patch (mic review)
Feel free to include the new dedicated patch in this series.
Quoted 41 lines
* Added the benchmark program to the selftests
* Fix unused variable report for "access_dom":
https://lore.kernel.org/all/[email protected]/
* Usesize_tandARRAY_SIZEto loop over the layers (mic review)
* Documentation
* Fixing up and adding back documentaiton (mic review)
* Documentedlandlock_unmask_layers()
* Fixed up kernel docs in a place where it was improperly updated (Spotted by Randy Dunlap
https://lore.kernel.org/all/[email protected]/)
* Minor
* Const, some newlines (mic review)V1: (Initial version)
https://lore.kernel.org/all/[email protected]/
Günther Noack (3):
selftests/landlock: Add filesystem access benchmark
landlock:access_mask_subset()helper
landlock: transpose the layer masks data structuresecurity/landlock/access.h | 16 +- security/landlock/audit.c | 84 ++--- security/landlock/audit.h | 3 +- security/landlock/domain.c | 45 +-- security/landlock/domain.h | 4 +- security/landlock/fs.c | 354 +++++++++----------- security/landlock/net.c | 11 +- security/landlock/ruleset.c | 88 ++--- security/landlock/ruleset.h | 21 +- tools/testing/selftests/landlock/.gitignore | 1 + tools/testing/selftests/landlock/Makefile | 1 + tools/testing/selftests/landlock/fs_bench.c | 161 +++++++++ 12 files changed, 444 insertions(+), 345 deletions(-) create mode 100644 tools/testing/selftests/landlock/fs_bench.cSignature · 1 line
2.52.0
Hello!
Quoted 11 lines · Mickaël Salaün
Quoted 4 lines · Günther Noack
P.S.: I am open to suggestions on what the "layer masks" variables should be called, because the name "layer masks" might be less appropriate after this change. I have not fixed up the name everywhere because fixing up the code took priority for now.Could you please clarify your thoughts and explain why this name might not be appropriate anymore? Any list of name proposals?
If we rename the variables, this should be done in the same refactoring patch.
When this was an array of layer_mask_t, the name layer_masks was a description of that underlying data type. Now that we have removed the layer_mask_t datatype, it is not as obviously true any more.
When trying to name these variables after the "role" that they have in their declaration context, I think of them as "unfulfilled per-layer access requests", but that strikes me as a bit long.
For the upcoming patch set, I'm leaning towards naming these variables just "masks", to keep it short.
Quoted 8 lines
Quoted 6 lines
Changes since previous versions:
V2: (This patch set)
* Remove the refactoring around the
deny_mask_ttype, it is better to send that as a separate patch (mic review)Feel free to include the new dedicated patch in this series.
I'm afraid that this one did not get any further than what it already was, and I'll have to leave it out for now. But I have it on my TODO list.
–Günther
Quoted 17 lines · Günther Noack
Quoted 11 lines · Mickaël Salaün
Quoted 4 lines · Günther Noack
P.S.: I am open to suggestions on what the "layer masks" variables should be called, because the name "layer masks" might be less appropriate after this change. I have not fixed up the name everywhere because fixing up the code took priority for now.Could you please clarify your thoughts and explain why this name might not be appropriate anymore? Any list of name proposals?
If we rename the variables, this should be done in the same refactoring patch.
When this was an array of
layer_mask_t, the namelayer_maskswas a description of that underlying data type. Now that we have removed thelayer_mask_tdatatype, it is not as obviously true any more.
Quoted 6 lines
When trying to name these variables after the "role" that they have in their declaration context, I think of them as "unfulfilled per-layer access requests", but that strikes me as a bit long.
For the upcoming patch set, I'm leaning towards naming these variables just "masks", to keep it short.
OK, staring at the code a bit longer, I realize that since the type is now named "struct layer_access_masks", "layer_masks" is actually still a reasonable shorthand. I have abbreviated that to "masks" in some places where it is anyway clear from the context that those are the layer access masks, but left it as "layer_masks" in places where we also use other access masks, for disambiguation.
–Günther
Mickaël Salaün
maintainerQuoted 25 lines · Günther Noack
Quoted 17 lines · Günther Noack
Quoted 11 lines · Mickaël Salaün
Quoted 4 lines · Günther Noack
P.S.: I am open to suggestions on what the "layer masks" variables should be called, because the name "layer masks" might be less appropriate after this change. I have not fixed up the name everywhere because fixing up the code took priority for now.Could you please clarify your thoughts and explain why this name might not be appropriate anymore? Any list of name proposals?
If we rename the variables, this should be done in the same refactoring patch.
When this was an array of
layer_mask_t, the namelayer_maskswas a description of that underlying data type. Now that we have removed thelayer_mask_tdatatype, it is not as obviously true any more.Quoted 6 lines
When trying to name these variables after the "role" that they have in their declaration context, I think of them as "unfulfilled per-layer access requests", but that strikes me as a bit long.
For the upcoming patch set, I'm leaning towards naming these variables just "masks", to keep it short.
Quoted 6 lines
OK, staring at the code a bit longer, I realize that since the type is now named "struct
layer_access_masks", "layer_masks" is actually still a reasonable shorthand. I have abbreviated that to "masks" in some places where it is anyway clear from the context that those are the layer access masks, but left it as "layer_masks" in places where we also use other access masks, for disambiguation.
Looks good