On Thu, Mar 17, 2022 at 4:35 AM Mickaël Salaün <mic@xxxxxxxxxxx> wrote: > On 17/03/2022 02:26, Paul Moore wrote: > > On Mon, Feb 21, 2022 at 4:15 PM Mickaël Salaün <mic@xxxxxxxxxxx> wrote: > >> > >> From: Mickaël Salaün <mic@xxxxxxxxxxxxxxxxxxx> > >> > >> Create and use the access_mask_t typedef to enforce a consistent access > >> mask size and uniformly use a 16-bits type. This will helps transition > >> to a 32-bits value one day. > >> > >> Add a build check to make sure all (filesystem) access rights fit in. > >> This will be extended with a following commit. > >> > >> Signed-off-by: Mickaël Salaün <mic@xxxxxxxxxxxxxxxxxxx> > >> Link: https://lore.kernel.org/r/20220221212522.320243-2-mic@xxxxxxxxxxx > >> --- > >> security/landlock/fs.c | 19 ++++++++++--------- > >> security/landlock/fs.h | 2 +- > >> security/landlock/limits.h | 2 ++ > >> security/landlock/ruleset.c | 6 ++++-- > >> security/landlock/ruleset.h | 17 +++++++++++++---- > >> 5 files changed, 30 insertions(+), 16 deletions(-) ... > >> diff --git a/security/landlock/limits.h b/security/landlock/limits.h > >> index 2a0a1095ee27..458d1de32ed5 100644 > >> --- a/security/landlock/limits.h > >> +++ b/security/landlock/limits.h > >> @@ -9,6 +9,7 @@ > >> #ifndef _SECURITY_LANDLOCK_LIMITS_H > >> #define _SECURITY_LANDLOCK_LIMITS_H > >> > >> +#include <linux/bitops.h> > >> #include <linux/limits.h> > >> #include <uapi/linux/landlock.h> > >> > >> @@ -17,5 +18,6 @@ > >> > >> #define LANDLOCK_LAST_ACCESS_FS LANDLOCK_ACCESS_FS_MAKE_SYM > >> #define LANDLOCK_MASK_ACCESS_FS ((LANDLOCK_LAST_ACCESS_FS << 1) - 1) > >> +#define LANDLOCK_NUM_ACCESS_FS __const_hweight64(LANDLOCK_MASK_ACCESS_FS) > > > > The line above, and the static_assert() in ruleset.h are clever. I'll > > admit I didn't even know the hweightX() macros existed until looking > > at this code :) > > > > However, the LANDLOCK_NUM_ACCESS_FS is never really going to be used > > outside the static_assert() in ruleset.h is it? I wonder if it would > > be better to skip the extra macro and rewrite the static_assert like > > this: > > > > static_assert(BITS_PER_TYPE(access_mask_t) >= > > __const_hweight64(LANDLOCK_MASK_ACCESS_FS)); > > > > If not, I might suggest changing LANDLOCK_NUM_ACCESS_FS to > > LANDLOCK_BITS_ACCESS_FS or something similar. > > I declared LANDLOCK_NUM_ACCESS_FS in this patch to be able to have the > static_assert() here and ease the review, but LANDLOCK_NUM_ACCESS_FS is > really used in patch 6/11 to define an array size: > get_handled_acceses(), init_layer_masks(), is_superset(), > check_access_path_dual()… I wrote my comments as I was working my way through the patchset and didn't think to go back and check this when I hit patch 6/11 :) Looks good to me, sorry for the noise. Reviewed-by: Paul Moore <paul@xxxxxxxxxxxxxx> -- paul-moore.com
