Re: [RFC PATCH 2/2] landlock: transpose the layer masks data structure

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

This looks good overall but I need to spend more time reviewing it.

Because this changes may impact other ongoing patch series, I think I'll
take this patch first to ease potential future fix backports.

On Tue, Dec 30, 2025 at 11:39:21AM +0100, Günther Noack wrote:
> The layer masks data structure tracks the requested but unfulfilled
> access rights during an operations 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)
> 
> * Code is ~160 lines smaller.

What about the KUnit test lines?

> 
> Other noteworthy changes:
> 
> * Clarify deny_mask_t and the code assembling it.
>   * Document what that value looks like
>   * Make writing and reading functions specific to file system rules.
>     (It only worked for FS rules before as well, but going all the way
>     simplifies the code logic more.)
> * 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.

It would be interesting to check the stackframe diff.  You can use
scripts/stackdelta for that, see
https://git.kernel.org/mic/c/602acfb541195eb35584d7a3fc7d1db676f059bd

> 
> Signed-off-by: Günther Noack <gnoack3000@gmail.com>
> ---
>  security/landlock/access.h  |  10 +-
>  security/landlock/audit.c   | 155 ++++++----------
>  security/landlock/audit.h   |   3 +-
>  security/landlock/domain.c  | 120 +++----------
>  security/landlock/domain.h  |   6 +-
>  security/landlock/fs.c      | 350 ++++++++++++++++--------------------
>  security/landlock/net.c     |  10 +-
>  security/landlock/ruleset.c |  78 +++-----
>  security/landlock/ruleset.h |  18 +-
>  9 files changed, 290 insertions(+), 460 deletions(-)

> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
> index dfcdc19ea2683..d20e28d38e9c9 100644
> --- a/security/landlock/ruleset.c
> +++ b/security/landlock/ruleset.c
> @@ -622,49 +622,24 @@ landlock_find_rule(const struct landlock_ruleset *const ruleset,
>   * request are empty).
>   */
>  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)
>  {
> -	size_t layer_level;
> -
> -	if (!access_request || !layer_masks)
> +	if (!masks)
>  		return true;
>  	if (!rule)
>  		return false;
>  
> -	/*
> -	 * An access is granted if, for each policy layer, at least one rule
> -	 * encountered on the pathwalk grants the requested access,
> -	 * regardless of its position in the layer stack.  We must then check
> -	 * the remaining layers for each inode, from the first added layer to
> -	 * the last one.  When there is multiple requested accesses, for each
> -	 * policy layer, the full set of requested accesses may not be granted
> -	 * by only one rule, but by the union (binary OR) of multiple rules.
> -	 * E.g. /a/b <execute> + /a <read> => /a/b <execute + read>
> -	 */

Why removing this comment?

> -	for (layer_level = 0; layer_level < rule->num_layers; layer_level++) {
> -		const struct landlock_layer *const layer =
> -			&rule->layers[layer_level];
> -		const layer_mask_t layer_bit = BIT_ULL(layer->level - 1);
> -		const unsigned long access_req = access_request;
> -		unsigned long access_bit;
> -		bool is_empty;
> +	for (int i = 0; i < rule->num_layers; i++) {
> +		const struct landlock_layer *l = &rule->layers[i];
>  
> -		/*
> -		 * Records in @layer_masks which layer grants access to each requested
> -		 * access: bit cleared if the related layer grants access.
> -		 */
> -		is_empty = true;
> -		for_each_set_bit(access_bit, &access_req, masks_array_size) {
> -			if (layer->access & BIT_ULL(access_bit))
> -				(*layer_masks)[access_bit] &= ~layer_bit;
> -			is_empty = is_empty && !(*layer_masks)[access_bit];
> -		}
> -		if (is_empty)
> -			return true;
> +		masks->access[l->level - 1] &= ~l->access;
>  	}
> -	return false;
> +
> +	for (int i = 0; i < LANDLOCK_MAX_NUM_LAYERS; i++) {
> +		if (masks->access[i])
> +			return false;
> +	}
> +	return true;
>  }
>  
>  typedef access_mask_t