Re: [PATCH v8 1/9] landlock: Add a place for flags to layer rules

[Justin Suess <utilityemal77@gmail.com>]

On Sun, May 24, 2026 at 07:20:19PM +0100, Tingmao Wang wrote:
> On 5/24/26 15:46, Justin Suess wrote:
> > On Sun, May 24, 2026 at 02:29:40AM +0100, Tingmao Wang wrote:
> >> On 5/23/26 21:48, Mickaël Salaün wrote:
> >>> [...]
> >>>> @@ -647,9 +648,14 @@ bool landlock_unmask_layers(const struct landlock_rule *const rule,
> >>>>  	 */
> >>>>  	for (size_t i = 0; i < rule->num_layers; i++) {
> >>>>  		const struct landlock_layer *const layer = &rule->layers[i];
> >>>> +		const layer_mask_t layer_bit = BIT_ULL(layer->level - 1);
> >>>>  
> >>>
> >>>>  		/* Clear the bits where the layer in the rule grants access. */
> >>>>  		masks->access[layer->level - 1] &= ~layer->access;
> >>>> +
> >>>> +		/* Collect rule flags for each layer. */
> >>>> +		if (rule_flags && layer->flags.quiet)
> >>>> +			rule_flags->quiet_masks |= layer_bit;
> >>>
> >>> Why not store the quiet bit in masks?  That would not only be "access"
> >>> bits anymore but it makes sense to store all this bits it the same
> >>> place.
> >>>
> >>> We should then probably rename struct layer_access_masks to just struct
> >>> layer_masks.
> >>>
> >>> We need to be careful to not increase too much the size of this struct
> >>> though while keeping the [LANDLOCK_MAX_NUM_LAYERS] approach if possible
> >>> (see Günther's commit that added it).
> >>
> >> Most uses of struct layer_access_masks do not actually care about the rule
> >> flags tho, e.g. in unmask_scoped_access, scope_to_request, or may_refer.
> >> Such a rename would touch 31 places (and only a few of them would actually
> >> touch the quiet flag).
> >>
> >> If we want to refactor to make this be in the layer_access_masks (then
> >> rename it), I guess there are 3 options, which do you prefer?
> >>
> >> 1. Add a u16 bitfield for which layers are quieted.  Future rule flags
> >>    will be additional bitfields.  struct layer_masks becomes 68 bytes (+4).
> >>
> >> struct layer_masks {
> >> 	access_mask_t access[LANDLOCK_MAX_NUM_LAYERS];
> >> 	layer_mask_t quiet_layers;
> >> };
> >>
> >> 2. Make the [LANDLOCK_MAX_NUM_LAYERS] array store both the access mask and
> >>    the quiet bit (or more bits for future rule flags).  Size of struct stays
> >>    the same.
> >>
> > This approach seems best.
> >> static_assert(LANDLOCK_NUM_ACCESS_NET <= LANDLOCK_NUM_ACCESS_FS);
> >> static_assert(LANDLOCK_NUM_SCOPE <= LANDLOCK_NUM_ACCESS_FS);
> >> struct layer_mask {
> >> 	access_mask_t access:LANDLOCK_NUM_ACCESS_FS;
> >> 	bool quiet:1;
> >> };
> > 
> > Other way to do it could be an (anonymous?) union.
> > 
> > union {
> >   access_mask_t fs_access:LANDLOCK_NUM_ACCESS_FS;
> >   access_mask_t net_access:LANDLOCK_NUM_ACCESS_NET;
> >   access_mask_t scope_access:LANDLOCK_NUM_SCOPE;
> > }
> > 
> > The union should be sized to fit the largest field automatically.
> > 
> > That way you don't have to change this when adding new access rights
> > and avoid the brittle static_asserts.
> > 
> > Not sure about the alignment implications here though.
> 
> Unfortunately this forces struct layer_mask to be 2x as large:
> https://godbolt.org/z/5P9b4rrMW
> 
Yeah I guess the compiler can't pack the fields with differing types.

*In theory* you could make everything a _BitInt or something but it
seems better to do what you had below.
> But it turns out I could have just used MAX, seems to compile for me:
> 
> struct layer_mask {
> 	access_mask_t access
> 		: MAX(LANDLOCK_NUM_ACCESS_FS,
> 		      MAX(LANDLOCK_NUM_ACCESS_NET, LANDLOCK_NUM_SCOPE));
> 	bool quiet : 1;
> };
This works perfectly.

Mickaël's suggestion (except w/ all three access right classes like
you have here, think he missed LANDLOCK_NUM_SCOPE) is very close
to this.
> struct layer_masks {
> 	struct layer_mask layer[LANDLOCK_MAX_NUM_LAYERS];
> };
> 
> Maybe we could #define LANDLOCK_NUM_ACCESS_MAX to be MAX(...) then use it
> here.
> 
> I'm still not sure if putting the collected rule flags in struct
> layer_(access_)masks is a good idea tho.  Passing a separate struct
> collected_rule_flags to the functions that needs to deal with rule flags
> (quiet, and later, no inherit / has no inherit descendant) seems quite
> practical to me.

(Not sure how stingy we gotta be with stack space)

There's a *slight* stack space advantage to keeping them together.

If you pass by value, (separate layer_access_masks, collected_rule_flags),
those structs must be individually padded and aligned. Which may or may not
make a difference, it's dependent on alignment and architecture.

Whereas if we keep them all together, we only pad once.

If you pass by pointer, you have to allocate stack space for each
pointer, so passing it all at once saves sizeof(collected_rule_flags*)
bytes in the pass by pointer case.

Either way it's probably a couple bytes at worst, so probably nothing to
worry about.

The more compelling argument is that we don't know how future paths
will use rule flags, so keeping it all together reduces churn later
if a function ends up needing to access flags. Moreover, it makes those
messy function signatures in fs.h/c a little less hairy, and easy to
refactor later.