Re: [PATCH bpf-next v3 12/15] landlock: Free rulesets after an RCU grace period

[bot+bpf-ci]

> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
> index 34d88ce37dc7..a65e9bce94d4 100644
> --- a/security/landlock/ruleset.c
> +++ b/security/landlock/ruleset.c

[ ... ]

> @@ -346,9 +347,26 @@ static void free_ruleset(struct landlock_ruleset *const ruleset)
>  	kfree(ruleset);
>  }
>
> -void landlock_put_ruleset(struct landlock_ruleset *const ruleset)
> +static void free_ruleset_work(struct work_struct *const work)
>  {
> -	might_sleep();
> +	struct rcu_work *const rcu_work = to_rcu_work(work);
> +	struct landlock_ruleset *const ruleset =
> +		container_of(rcu_work, struct landlock_ruleset, work_free);
> +
> +	free_ruleset(ruleset);
> +}
> +
> +/*
> + * landlock_put_ruleset - Release ruleset reference
> + *
> + * @ruleset: Ruleset to release, or NULL.
> + *
> + * Release a reference. When the last reference is dropped, the ruleset is
> + * freed after an RCU grace period.
> + */
> +void landlock_put_ruleset(struct landlock_ruleset *const ruleset)
> +{
>  	if (ruleset && refcount_dec_and_test(&ruleset->usage)) {
> -		trace_landlock_free_ruleset(ruleset);
> -		free_ruleset(ruleset);
> +		INIT_RCU_WORK(&ruleset->work_free, free_ruleset_work);
> +		queue_rcu_work(system_dfl_wq, &ruleset->work_free);
>  	}
>  }

Deferring the free moves the landlock_free_ruleset tracepoint out of the
caller's context: trace_landlock_free_ruleset() is emitted from
free_ruleset(), which after this patch only runs from free_ruleset_work()
on a kworker, an RCU grace period after the last put.

That changes an observable, in-tree-tested property of the tracepoint,
and the patch does not update the test that asserts the old behaviour, so
TEST_F(trace, free_ruleset_on_close) in
tools/testing/selftests/landlock/trace_test.c now fails deterministically
for three independent reasons:

1. PID filtering drops the event entirely. The fixture calls
tracefs_set_pid_filter(getpid()), which writes set_event_pid. ftrace
records an event only when the *current* task's pid is in that list. The
freeing kworker is a kthread forked from kthreadd, so neither
set_event_pid nor options/event-fork ever adds it, and the
landlock_free_ruleset record is discarded. The existing free_domain tests
handle exactly this by clearing the PID filter before triggering the
deferred free.

2. No wait for the grace period + workqueue. The test reads the buffer
synchronously right after close(ruleset_fd):

    close(ruleset_fd);
    buf = tracefs_read_buf();

The free_domain tests instead poll (for (int retry = 0; retry < 10;
retry++) { usleep(100000); ... }) precisely because the event now arrives
later.

3. Wrong task regex. The assertion uses REGEX_FREE_RULESET(TRACE_TASK),
but the record, if it were captured at all, carries a kworker name.
trace.h already defines KWORKER_TASK for exactly this case.

Net effect: EXPECT_EQ(1, tracefs_count_matches(buf,
REGEX_FREE_RULESET(TRACE_TASK))) sees 0. The selftest needs the same
treatment the free_domain tests already got (clear the PID filter, poll,
match KWORKER_TASK), otherwise the landlock kselftest suite regresses
with this commit.

---

The tracepoint documentation for landlock_free_ruleset is now stale and
actively misleading for tracer authors. include/trace/events/landlock.h
still says the event is "Emitted when a ruleset's last reference is
dropped (typically when the creating process closes the ruleset file
descriptor)", which after this patch is no longer the emission point: the
last put only queues rcu_work, and the event fires from a kworker one RCU
grace period later. Documentation/trace/events-landlock.rst likewise
lists only "``landlock_free_ruleset``: a ruleset is freed".

The tree already establishes the convention of spelling this out for the
sibling event: the landlock_free_domain kernel-doc states "A committed
domain is freed from a kworker via landlock_put_domain_deferred() ... so
the current task is not the sandboxed task that triggered the free", and
Documentation/trace/events-landlock.rst repeats it with a kworker example
line. landlock_free_ruleset now has the same property and neither
document was updated, so consumers keep assuming the event is attributable
to the task that closed the fd (which is precisely the assumption the
in-tree selftest encodes).

> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
> index c12b6e46fa3f..3e730e90ce06 100644
> --- a/security/landlock/ruleset.h
> +++ b/security/landlock/ruleset.h

[ ... ]

> @@ -175,22 +174,41 @@ struct landlock_ruleset {
>  	/**
>  	 * @usage: Number of alive &struct landlock_cred pointing to this
>  	 * ruleset and also the reference counter for user space. This must
> -	 * remain valid while the object can be retrieved from an RCU
> -	 * protected pointer.
> +	 * remain readable while the object can be retrieved from an RCU
> +	 * protected pointer.
>  	 */
>  	refcount_t usage;
> -	/**
> -	 * @lock: Guards modifications to the ruleset. Must not be taken under
> -	 * rcu_read_lock, but after refcount_inc_not_zero has succeeded. An
> -	 * RCU reader that needs the lock should bail (e.g., return -EAGAIN to
> -	 * user space) instead of trying to take it.
> -	 */
> -	struct mutex lock;
> -	/**
> -	 * @quiet_masks: Access rights to skip logging for, per layer level.
> -	 */
> -	struct access_masks quiet_masks;
> -	/** @handled_masks: Access rights supported by the ruleset. */
> -	struct access_masks handled_masks;
> +	union {
> +		/**
> +		 * @work_free: Scheduled via queue_rcu_work when @usage
> +		 * drops to zero.  By the time this work item runs, every
> +		 * RCU reader that could still observe a pointer to the
> +		 * containing struct landlock_ruleset is guaranteed to be
> +		 * done. (INIT_RCU_WORK initializes both the inner
> +		 * work_struct and the surrounding rcu_work).
> +		 * The fields @lock, @quiet_masks and @handled_masks are
> +		 * then unused, so overlay this work item with them to
> +		 * keep sizeof(struct landlock_ruleset) down.
> +		 */
> +		struct rcu_work work_free;
> +		struct {
> +			/**
> +			 * @lock: Guards modifications to the ruleset. Must not
> +			 * be taken under rcu_read_lock, but after
> +			 * refcount_inc_not_zero has succeeded. An RCU reader
> +			 * that needs the lock should bail (e.g., return -EAGAIN
> +			 * to user space) instead of trying to take it.
> +			 */
> +			struct mutex lock;
> +			/**
> +			 * @quiet_masks: Access rights to skip logging for, per
> +			 * layer level.
> +			 */
> +			struct access_masks quiet_masks;
> +			/**
> +			 * @handled_masks: Access rights supported by the
> +			 * ruleset.
> +			 */
> +			struct access_masks handled_masks;
> +		};
> +	};

The union makes @lock, @quiet_masks and @handled_masks alias the queued
rcu_work, and trace_landlock_free_ruleset() is emitted from
free_ruleset() *while that work item is executing*, so at trace time
those three fields hold workqueue bookkeeping bytes, not ruleset state.

That matters because include/trace/events/landlock.h explicitly
advertises reading the traced pointer's fields beyond TP_STRUCT__entry:
"eBPF programs read these pointers via BTF for richer introspection than
the TP_STRUCT__entry fields". A tracer that pairs landlock_create_ruleset
(whose TP_STRUCT__entry reads handled_masks.fs/net/scope) with
landlock_free_ruleset and reads ruleset->handled_masks or
ruleset->quiet_masks via BTF gets rcu_work contents. The commit message
and the @work_free kernel-doc justify the overlay with "The fields @lock,
@quiet_masks and @handled_masks are then unused", which holds for
in-kernel code but not for the BTF field access the header sanctions.

There is no in-tree precedent that would make this acceptable by analogy:
struct landlock_domain uses the same overlay trick but free_domain()
emits no tracepoint on the unioned object -- trace_landlock_free_domain()
is passed a struct landlock_hierarchy, which has no union. Either the
overlay should exclude the fields the trace interface exposes, or the
"Pointer access" documentation should state that only @usage, @version
and @id are valid at landlock_free_ruleset time.

---

> landlock: Free rulesets after an RCU grace period
>
> Defer every ruleset free behind an RCU grace period, and keep the
> fields that stay readable while a free is pending out of the union
> that overlays the deferred-free work item.
> The policy_object_get LSM hook lets a caller holding only an
> RCU-protected pointer to a ruleset (e.g. loaded from a BPF map kptr
> field under rcu_read_lock()) race a refcount_inc_not_zero() against
> the drop of the last reference.  For that to be sound, the ruleset's
> memory, and its reference count in particular, must remain valid
> until every RCU reader that could still observe the pointer is done:
> free the ruleset through queue_rcu_work(), which waits for a grace
> period before running the free work.
> The work item is overlaid with the fields that no one may touch once
> @usage reaches zero: @lock, @quiet_masks and @handled_masks.  @usage
> itself stays outside the union so a racing reader observes zero
> instead of the work item's bytes, and the tracing fields @version and
> @id stay outside too because the landlock_free_ruleset trace event
> reads them when the queued work finally runs.
> Since queueing the work never sleeps, the might_sleep() annotation
> is dropped: a following commit releases ruleset references from
> BPF object destructors that cannot sleep.
> Cc: Mickaël Salaün <mic@xxxxxxxxxxx>
> Signed-off-by: Justin Suess <utilityemal77@xxxxxxxxx>

This looks like a bug fix for stack argument support. Should this
include:

  Fixes: 7a529e9b4e96 ("lsm: Add the LSM policy object lifetime hooks")


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/34398515657