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

[Tingmao Wang]

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

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