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

[Günther Noack]

On Fri, Jan 09, 2026 at 05:18:43PM +0100, Mickaël Salaün wrote:
> 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?

Those are counted as well. The 160 lines statistic is directly from
the diffstat in the cover letter.

(I removed the test_get_layer_deny_mask KUnit test, because the
function under test was also not needed any more. Other than that, the
KUnit tests are just adapted to test the equivalent logic with the new
data structure.)


> > 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

Acknowledged.  I did not get around to it yet, but put it on my todo
list.


> > Signed-off-by: Günther Noack <gnoack3000 at 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(-)

460 - 290 is 170.  Well, almost 160. :)


> > 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?

I did not understand why the comment was discussing this higher level
picture when the surrounding function landlock_unmask_layers() is only
concerned with a single rule.  Should this comment be better moved
elsewhere?

I don't feel strongly about it and re-reading it, the comment is still
true.  In doubt, I can also just put it back into the same function
again.  Let me know what you prefer.


> > -	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

–Günther