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

[Tingmao Wang]

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.

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;
};
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)

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)