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

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

On Thu, Jan 29, 2026 at 05:54:01PM +0100, Mickaël Salaün wrote:
> 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:
> > > > +	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().

Good point, done.


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

Yes, the existing tests already catch that; this happens when we break early:

[08:53:12] ================= landlock_fs (7 subtests) =================
[08:53:12] [PASSED] test_no_more_access
[08:53:12] [PASSED] test_scope_to_request_with_exec_none
[08:53:12] # test_scope_to_request_with_exec_some: EXPECTATION FAILED at security/landlock/fs.c:616
[08:53:12] Expected 0 == masks.access[1], but
[08:53:12]     masks.access[1] == 2 (0x2)
[08:53:12] [FAILED] test_scope_to_request_with_exec_some
[08:53:12] [PASSED] test_scope_to_request_without_access
[08:53:12] [PASSED] test_is_eacces_with_none
[08:53:12] [PASSED] test_is_eacces_with_refer
[08:53:12] [PASSED] test_is_eacces_with_write
[08:53:12]     # module: landlock
[08:53:12] # landlock_fs: pass:6 fail:1 skip:0 total:7
[08:53:12] # Totals: pass:6 fail:1 skip:0 total:7

Good coverage!

–Günther