Re: [PATCH v2 3/3] landlock: transpose the layer masks data structure

["Günther Noack" <gnoack3000@gmail.com>]

On Wed, Jan 28, 2026 at 10:34:02PM +0100, Mickaël Salaün wrote:
> On Sun, Jan 25, 2026 at 08:58:53PM +0100, Günther Noack wrote:
> > 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.

> > --- a/security/landlock/audit.c
> > +++ b/security/landlock/audit.c
> > @@ -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.

> 
> >  static 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);
> > -		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

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?


> > +		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_t i

Ditto, the loop goes from top to bottom here.


> > +		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))
> > +	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])
> 
> {
> 
> > +			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?

> > +	}
> > +	return !saw_unfulfilled_access;
> >  }

–Günther