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

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

On Thu, Jan 29, 2026 at 08:56:37AM +0100, Günther Noack wrote:
> 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?

Indeed.  We can use ssize_t as in get_hierarchy().

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

You're right, don't add this break.  BTW, would a test catch it?

> 
> > > +	}
> > > +	return !saw_unfulfilled_access;
> > >  }
> 
> –Günther
>