Re: [PATCH v10 3/9] landlock: Suppress logging when quiet flag is present

[Tingmao Wang <m@maowtm.org>]

On 6/8/26 23:41, Mickaël Salaün wrote:
> On Mon, Jun 01, 2026 at 01:00:37AM +0100, Tingmao Wang wrote:
>> [...]
>> @@ -265,20 +268,33 @@ get_layer_from_deny_masks(access_mask_t *const access_request,
>>  			 BITS_PER_TYPE(access_mask_t)) {
>>  		if (access_req & BIT(access_bit)) {
>>  			const size_t layer =
>> -				(deny_masks >> (access_index * 4)) &
>> +				(deny_masks >>
>> +				 (access_index *
>> +				  HWEIGHT(LANDLOCK_MAX_NUM_LAYERS - 1))) &
>>  				(LANDLOCK_MAX_NUM_LAYERS - 1);
>> +			const bool layer_has_quiet =
>> +				!!(quiet_optional_accesses & BIT(access_index));
>>  
>>  			if (layer > youngest_layer) {
>>  				youngest_layer = layer;
>>  				missing = BIT(access_bit);
>> +				should_quiet = layer_has_quiet;
>>  			} else if (layer == youngest_layer) {
>>  				missing |= BIT(access_bit);
>> +				/*
>> +				 * Whether the layer has rules with quiet flag covering
>> +				 * the file accessed does not depend on the access, and so
>> +				 * the following WARN_ON_ONCE() should not fail.
>> +				 */
>> +				WARN_ON_ONCE(should_quiet && !layer_has_quiet);
> 
> WARN_ON_ONCE(should_quiet != layer_has_quiet);

That will fail when layer 0 has quiet flag on the object, at which point
since youngest_layer starts from 0, we will reach this branch with
should_quiet initialized earlier to false, but layer_has_quiet == true.

Also, if should_quiet != layer_has_quiet is always false here, then the
next line is not necessary.

> 
>> +				should_quiet = layer_has_quiet;

Would it be clearer to do should_quiet |= layer_has_quiet, mimicking the
"missing |= BIT(access_bit)"? (The result is the same)

>>  			}
>>  		}
>>  		access_index++;
>>  	}
>>  
>>  	*access_request = missing;
>> +	*quiet = should_quiet;
>>  	return youngest_layer;
>>  }
>>  
>> [...]
>> diff --git a/security/landlock/fs.h b/security/landlock/fs.h
>> index cb7e654933ac..d0fca7da2466 100644
>> --- a/security/landlock/fs.h
>> +++ b/security/landlock/fs.h
>> @@ -63,11 +63,20 @@ struct landlock_file_security {
>>  	 * _LANDLOCK_ACCESS_FS_OPTIONAL).
>>  	 */
>>  	deny_masks_t deny_masks;
>> +	/**
>> +	 * @quiet_optional_accesses: Stores which optional accesses are
>> +	 * covered by quiet rules within the layer referred to in deny_masks,
>> +	 * one access per bit.  Does not take into account whether the quiet
>> +	 * access bits are actually set in the layer's corresponding
>> +	 * landlock_hierarchy.
>> +	 */
>> +	optional_access_t quiet_optional_accesses
>> +		: HWEIGHT(_LANDLOCK_ACCESS_FS_OPTIONAL);
>>  	/**
>>  	 * @fown_layer: Layer level of @fown_subject->domain with
>>  	 * LANDLOCK_SCOPE_SIGNAL.
>>  	 */
>> -	u8 fown_layer;
>> +	u8 fown_layer : 4;
> 
> Please don't hardcode such size.
> 
> Anyway, fown_layer can be updated concurrently (holding a lock), so we
> should not convert it to a bitfield.
> 
>>  #endif /* CONFIG_AUDIT */
>>  
>>  	/**
> 
>> @@ -82,12 +91,6 @@ struct landlock_file_security {
>>  
>>  #ifdef CONFIG_AUDIT
>>  
>> -/* Makes sure all layers can be identified. */
>> -/* clang-format off */
>> -static_assert((typeof_member(struct landlock_file_security, fown_layer))~0 >=
>> -	      LANDLOCK_MAX_NUM_LAYERS);
>> -/* clang-format off */
>> -
>>  #endif /* CONFIG_AUDIT */
> 
> Remaining useless ifdef/endif.

Since we are not using bitfield for fown_layer anymore, I've also turned
quiet_optional_accesses (u8) into a non-bitfield.  Then I reverted this
deletion and added

    static_assert((typeof_member(struct landlock_file_security,
    			     quiet_optional_accesses)) ~0 >=
    	      HWEIGHT(_LANDLOCK_ACCESS_FS_OPTIONAL));

> 
>> [...]