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

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

On Sun, May 24, 2026 at 06:08:00PM -0400, Justin Suess wrote:
> 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.

LANDLOCK_NUM_ACCESS_MAX looks like a better name (even if it also
include scopes).

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

Agreed