Re: [PATCH v8 03/12] landlock: Replace union access_masks_all with helper functions

["Mickaël Salaün" <mic@digikod.net>]

On Fri, Mar 27, 2026 at 05:48:28PM +0100, Günther Noack wrote:
> * Stop using a union for access_masks_all.
> * Expose helper functions for intersection checks and union operations.
> 
> The memory layout of bitfields is only loosely defined by the C
> standard, so our static assertion that expects a fixed size was
> brittle, and it broke on some compilers when we attempted to add a
> 17th file system access right.
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202603261438.jBx2DGNe-lkp@intel.com/
> Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> ---
>  security/landlock/access.h  | 21 ++++++++++++++-------
>  security/landlock/cred.h    | 10 ++--------
>  security/landlock/ruleset.h | 13 ++++---------
>  3 files changed, 20 insertions(+), 24 deletions(-)

I'd prefer this approach:

diff --git a/security/landlock/access.h b/security/landlock/access.h
index 89dc8e7b93da..bc9efbb5c900 100644
--- a/security/landlock/access.h
+++ b/security/landlock/access.h
@@ -50,7 +50,7 @@ struct access_masks {
        access_mask_t fs : LANDLOCK_NUM_ACCESS_FS;
        access_mask_t net : LANDLOCK_NUM_ACCESS_NET;
        access_mask_t scope : LANDLOCK_NUM_SCOPE;
-};
+} __packed;

 union access_masks_all {
        struct access_masks masks;
@@ -58,7 +58,7 @@ union access_masks_all {
 };

 /* Makes sure all fields are covered. */
-static_assert(sizeof(typeof_member(union access_masks_all, masks)) ==
+static_assert(sizeof(typeof_member(union access_masks_all, masks)) <=
              sizeof(typeof_member(union access_masks_all, all)));

 /**


This keep the check and make sure new field will not introduce issues, but more
importantly it save memory, which was one of the goal of struct access_masks.

If that's good with you I'll replace your patch and squash this packed
annotation with the following patch.


> 
> diff --git a/security/landlock/access.h b/security/landlock/access.h
> index 42c95747d7bd..277b6ed7f7bb 100644
> --- a/security/landlock/access.h
> +++ b/security/landlock/access.h
> @@ -52,14 +52,21 @@ struct access_masks {
>  	access_mask_t scope : LANDLOCK_NUM_SCOPE;
>  };
>  
> -union access_masks_all {
> -	struct access_masks masks;
> -	u32 all;
> -};
> +/* Checks whether two access masks have any common bit set. */
> +static inline bool access_masks_intersect(const struct access_masks a,
> +					  const struct access_masks b)
> +{
> +	return (a.fs & b.fs) || (a.net & b.net) || (a.scope & b.scope);
> +}
>  
> -/* Makes sure all fields are covered. */
> -static_assert(sizeof(typeof_member(union access_masks_all, masks)) ==
> -	      sizeof(typeof_member(union access_masks_all, all)));
> +/* ORs the bits of @src into @dst. */
> +static inline void access_masks_merge(struct access_masks *dst,
> +				      const struct access_masks src)
> +{
> +	dst->fs |= src.fs;
> +	dst->net |= src.net;
> +	dst->scope |= src.scope;
> +}
>  
>  /**
>   * struct layer_access_masks - A boolean matrix of layers and access rights
> diff --git a/security/landlock/cred.h b/security/landlock/cred.h
> index f287c56b5fd4..207a6db1c086 100644
> --- a/security/landlock/cred.h
> +++ b/security/landlock/cred.h
> @@ -123,9 +123,6 @@ landlock_get_applicable_subject(const struct cred *const cred,
>  				const struct access_masks masks,
>  				size_t *const handle_layer)
>  {
> -	const union access_masks_all masks_all = {
> -		.masks = masks,
> -	};
>  	const struct landlock_ruleset *domain;
>  	ssize_t layer_level;
>  
> @@ -138,11 +135,8 @@ landlock_get_applicable_subject(const struct cred *const cred,
>  
>  	for (layer_level = domain->num_layers - 1; layer_level >= 0;
>  	     layer_level--) {
> -		union access_masks_all layer = {
> -			.masks = domain->access_masks[layer_level],
> -		};
> -
> -		if (layer.all & masks_all.all) {
> +		if (access_masks_intersect(domain->access_masks[layer_level],
> +					   masks)) {
>  			if (handle_layer)
>  				*handle_layer = layer_level;
>  
> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
> index 889f4b30301a..9f8b33815c2c 100644
> --- a/security/landlock/ruleset.h
> +++ b/security/landlock/ruleset.h
> @@ -229,18 +229,13 @@ static inline void landlock_get_ruleset(struct landlock_ruleset *const ruleset)
>  static inline struct access_masks
>  landlock_union_access_masks(const struct landlock_ruleset *const domain)
>  {
> -	union access_masks_all matches = {};
> +	struct access_masks matches = {};
>  	size_t layer_level;
>  
> -	for (layer_level = 0; layer_level < domain->num_layers; layer_level++) {
> -		union access_masks_all layer = {
> -			.masks = domain->access_masks[layer_level],
> -		};
> +	for (layer_level = 0; layer_level < domain->num_layers; layer_level++)
> +		access_masks_merge(&matches, domain->access_masks[layer_level]);
>  
> -		matches.all |= layer.all;
> -	}
> -
> -	return matches.masks;
> +	return matches;
>  }
>  
>  static inline void
> -- 
> 2.53.0
> 
>