Re: [PATCH v4 03/10] landlock: Suppress logging when quiet flag is present

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

On Sun, Nov 23, 2025 at 09:01:47PM +0000, Tingmao Wang wrote:
> On 11/21/25 15:27, Mickaël Salaün wrote:
> > On Sun, Nov 16, 2025 at 09:59:33PM +0000, Tingmao Wang wrote:
> >> [...]

> >>  /**
> >>   * landlock_log_denial - Create audit records related to a denial
> >>   *
> >>   * @subject: The Landlock subject's credential denying an action.
> >>   * @request: Detail of the user space request.
> >> + * @rule_flags: The flags for the matched rule, or no_rule_flags (zero) if
> >> + * this is a scope request (no particular object involved).
> >>   */
> >>  void landlock_log_denial(const struct landlock_cred_security *const subject,
> >> -			 const struct landlock_request *const request)
> >> +			 const struct landlock_request *const request,
> >> +			 const struct collected_rule_flags rule_flags)
> >
> > It would be simpler and limit code change to move rule_flags/quiet_flags
> > into struct landlock_request, which means we can also remove
> > no_rule_flags.
> 
> That's true, I can do that.  In fact this way we also don't have to have 2
> extra parameters for is_access_to_paths_allowed - it can just operate on
> log_request_parent{1,2}->rule_flags.  However I do see that
> landlock_request is intended to only be used by audit/logging code (and
> there is a comment in audit.h about not using it outside CONFIG_AUDIT to
> enable it to be optimized away, although testing a fresh build on next it
> doesn't look like it is taken out of vmlinux if compiled without
> CONFIG_AUDIT).

The log_request_parent* are indeed not elided.  I sent a patch to fix
that: https://lore.kernel.org/r/20251219142302.744917-2-mic@digikod.net


> While this is fine for the purpose of this series as the
> quiet flag only affects audit logging, I wonder if this might cause a
> problem when we want to add more flags that might not be related to audit
> (e.g. Justin's LANDLOCK_ADD_RULE_NO_INHERIT).
> 
> Alternatively maybe is_access_to_paths_allowed can still take extra
> parameters for rule flags, and we can make it so that the new rule_flags
> field in landlock_request is only assigned to right before
> landlock_log_denial, not from is_access_to_paths_allowed?  (I won't do
> this for v5 which I will send in a minute)

Yes, we should store the rule flags in landlock_request struct when this
in only required for audit.  In any case, we should always be able to
build without CONFIG_AUDIT, and in this case the stack frame should not
include landlock_request.  So in your last patch series, because
landlock_request's rule flags are referenced by standalone pointers,
landlock_requset cannot be elided.

> 
> >
> >>  {
> >>  	struct audit_buffer *ab;
> >>  	struct landlock_hierarchy *youngest_denied;
> >>  	size_t youngest_layer;
> >> -	access_mask_t missing;
> >> +	access_mask_t missing, quiet_mask;
> >> +	bool object_quiet_flag = false, quiet_applicable_to_access = false;
> >>
> >>  	if (WARN_ON_ONCE(!subject || !subject->domain ||
> >>  			 !subject->domain->hierarchy || !request))
> >> @@ -409,10 +586,13 @@ void landlock_log_denial(const struct landlock_cred_security *const subject,
> >>  			youngest_layer = get_denied_layer(
> >>  				subject->domain, &missing, request->layer_masks,
> >>  				request->layer_masks_size);
> >> +			object_quiet_flag = !!(rule_flags.quiet_masks & BIT(youngest_layer));
> >>  		} else {
> >>  			youngest_layer = get_layer_from_deny_masks(
> >>  				&missing, request->all_existing_optional_access,
> >> -				request->deny_masks);
> >> +				request->deny_masks,
> >> +				request->quiet_optional_accesses,
> >> +				&object_quiet_flag);
> >>  		}
> >>  		youngest_denied =
> >>  			get_hierarchy(subject->domain, youngest_layer);