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 02:29:40AM +0100, Tingmao Wang wrote:
> On 5/23/26 21:48, Mickaël Salaün wrote:
> > This patch doesn't build.
> 
> Missed a hunk in this patch (ended up in the next one), will add.
> 
> >> @@ -797,20 +803,28 @@ is_access_to_paths_allowed(const struct landlock_ruleset *const domain,
> >>  	}
> >>  
> >>  	if (unlikely(dentry_child1)) {
> >> +		/*
> >> +		 * Get the layer masks for the child dentries for use by domain
> >> +		 * check later.  The rule_flags for child1 should have been
> >> +		 * included in rule_flags_parent1 already (cf.
> >> +		 * collect_domain_accesses), and is not relevant for domain check,
> >> +		 * so we don't have to pass it to landlock_unmask_layers.
> >> +		 */
> >>  		if (landlock_init_layer_masks(domain, LANDLOCK_MASK_ACCESS_FS,
> >>  					      &_layer_masks_child1,
> >>  					      LANDLOCK_KEY_INODE))
> >>  			landlock_unmask_layers(find_rule(domain, dentry_child1),
> >> -					       &_layer_masks_child1);
> >> +					       &_layer_masks_child1, NULL);
> >>  		layer_masks_child1 = &_layer_masks_child1;
> >>  		child1_is_directory = d_is_dir(dentry_child1);
> >>  	}
> >>  	if (unlikely(dentry_child2)) {
> >> +		/* See above comment for why NULL is passed as rule_flags_masks. */
> > 
> > rule_flags_masks doesn't exist.
> 
> I guess I was probably referring to the rule_flags argument - will fix.
> 
> >> [...]
> >> @@ -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.
> struct layer_masks {
> 	struct layer_mask layer[LANDLOCK_MAX_NUM_LAYERS];
> };
> 
>    (Maybe we can just make struct layer_masks a typedef to
>    layer_mask[LANDLOCK_MAX_NUM_LAYERS] instead?  But currently not sure if
>    there are any gotchas with a typedef like that)
> 
Typedefs are generally discouraged for structs and pointers in the
kernel coding style.

https://docs.kernel.org/process/coding-style.html
> 3. Mirror layer_access_masks::access[] - add a
>    rule_flags[LANDLOCK_MAX_NUM_LAYERS] too.  struct layer_masks becomes 80
>    bytes (+16).
> 
> struct rule_flags {
> 	bool quiet:1;
> };
> struct layer_masks {
> 	/**
> 	 * @access: The unfulfilled access rights for each layer.
> 	 */
> 	access_mask_t access[LANDLOCK_MAX_NUM_LAYERS];
> 	struct rule_flags rule_flags[LANDLOCK_MAX_NUM_LAYERS];
> };
> 
> (3 seems very wasteful to me)