* Re: [PATCH v2 12/15] ovl: change ovl_create_real() to get a new lock when re-opening created file.
From: Amir Goldstein @ 2026-02-23 9:39 UTC (permalink / raw)
To: NeilBrown
Cc: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Jeff Layton, Miklos Szeredi, John Johansen,
Paul Moore, James Morris, Serge E. Hallyn, Stephen Smalley,
Darrick J. Wong, linux-kernel, netfs, linux-fsdevel, linux-nfs,
linux-unionfs, apparmor, linux-security-module, selinux
In-Reply-To: <20260223011210.3853517-13-neilb@ownmail.net>
On Mon, Feb 23, 2026 at 2:14 AM NeilBrown <neilb@ownmail.net> wrote:
>
> From: NeilBrown <neil@brown.name>
>
> When ovl_create_real() is used to create a file on the upper filesystem
> it needs to return the resulting dentry - positive and hashed.
> It is usually the case the that dentry passed to the create function
> (e.g. vfs_create()) will be suitable but this is not guaranteed. The
> filesystem may unhash that dentry forcing a repeat lookup next time the
> name is wanted.
>
> So ovl_create_real() must be (and is) aware of this and prepared to
> perform that lookup to get a hash positive dentry.
>
> This is currently done under that same directory lock that provided
> exclusion for the create. Proposed changes to locking will make this
> not possible - as the name, rather than the directory, will be locked.
> The new APIs provided for lookup and locking do not and cannot support
> this pattern.
>
> The lock isn't needed. ovl_create_real() can drop the lock and then get
> a new lock for the lookup - then check that the lookup returned the
> correct inode. In a well-behaved configuration where the upper
> filesystem is not being modified by a third party, this will always work
> reliably, and if there are separate modification it will fail cleanly.
>
> So change ovl_create_real() to drop the lock and call
> ovl_start_creating_upper() to find the correct dentry. Note that
> start_creating doesn't fail if the name already exists.
>
> The lookup previously used the name from newdentry which was guaranteed
> to be stable because the parent directory was locked. As we now drop
> the lock we lose that guarantee. As newdentry is unhashed it is
> unlikely for the name to change, but safest not to depend on that. So
> the expected name is now passed in to ovl_create_real() and that is
> used.
>
> This removes the only remaining use of ovl_lookup_upper, so it is
> removed.
>
> Signed-off-by: NeilBrown <neil@brown.name>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> ---
> fs/overlayfs/dir.c | 36 ++++++++++++++++++++++++------------
> fs/overlayfs/overlayfs.h | 8 +-------
> fs/overlayfs/super.c | 1 +
> 3 files changed, 26 insertions(+), 19 deletions(-)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index c4feb89ad1e3..6285069ccc59 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -159,7 +159,8 @@ int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *dir,
> }
>
> struct dentry *ovl_create_real(struct ovl_fs *ofs, struct dentry *parent,
> - struct dentry *newdentry, struct ovl_cattr *attr)
> + struct dentry *newdentry, struct qstr *qname,
> + struct ovl_cattr *attr)
> {
> struct inode *dir = parent->d_inode;
> int err;
> @@ -221,19 +222,29 @@ struct dentry *ovl_create_real(struct ovl_fs *ofs, struct dentry *parent,
> struct dentry *d;
> /*
> * Some filesystems (i.e. casefolded) may return an unhashed
> - * negative dentry from the ovl_lookup_upper() call before
> + * negative dentry from the ovl_start_creating_upper() call before
> * ovl_create_real().
> * In that case, lookup again after making the newdentry
> * positive, so ovl_create_upper() always returns a hashed
> - * positive dentry.
> + * positive dentry. We lookup using qname which should be
> + * the same name as newentry, but is certain not to change.
> + * As we have to drop the lock before the lookup a race
> + * could result in a lookup failure. In that case we return
> + * an error.
> */
> - d = ovl_lookup_upper(ofs, newdentry->d_name.name, parent,
> - newdentry->d_name.len);
> - dput(newdentry);
> - if (IS_ERR_OR_NULL(d))
> + end_creating_keep(newdentry);
> + d = ovl_start_creating_upper(ofs, parent, qname);
> +
> + if (IS_ERR_OR_NULL(d)) {
> err = d ? PTR_ERR(d) : -ENOENT;
> - else
> + } else if (d->d_inode != newdentry->d_inode) {
> + err = -EIO;
> + dput(newdentry);
> + } else {
> + dput(newdentry);
> return d;
> + }
> + return ERR_PTR(err);
> }
> out:
> if (err) {
> @@ -252,7 +263,7 @@ struct dentry *ovl_create_temp(struct ovl_fs *ofs, struct dentry *workdir,
> ret = ovl_start_creating_temp(ofs, workdir, name);
> if (IS_ERR(ret))
> return ret;
> - ret = ovl_create_real(ofs, workdir, ret, attr);
> + ret = ovl_create_real(ofs, workdir, ret, &QSTR(name), attr);
> return end_creating_keep(ret);
> }
>
> @@ -352,14 +363,15 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
> struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
> struct dentry *upperdir = ovl_dentry_upper(dentry->d_parent);
> struct dentry *newdentry;
> + struct qstr qname = QSTR_LEN(dentry->d_name.name,
> + dentry->d_name.len);
> int err;
>
> newdentry = ovl_start_creating_upper(ofs, upperdir,
> - &QSTR_LEN(dentry->d_name.name,
> - dentry->d_name.len));
> + &qname);
> if (IS_ERR(newdentry))
> return PTR_ERR(newdentry);
> - newdentry = ovl_create_real(ofs, upperdir, newdentry, attr);
> + newdentry = ovl_create_real(ofs, upperdir, newdentry, &qname, attr);
> if (IS_ERR(newdentry))
> return PTR_ERR(newdentry);
>
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index cad2055ebf18..714a1cec3709 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -406,13 +406,6 @@ static inline struct file *ovl_do_tmpfile(struct ovl_fs *ofs,
> return file;
> }
>
> -static inline struct dentry *ovl_lookup_upper(struct ovl_fs *ofs,
> - const char *name,
> - struct dentry *base, int len)
> -{
> - return lookup_one(ovl_upper_mnt_idmap(ofs), &QSTR_LEN(name, len), base);
> -}
> -
> static inline struct dentry *ovl_lookup_upper_unlocked(struct ovl_fs *ofs,
> const char *name,
> struct dentry *base,
> @@ -888,6 +881,7 @@ struct ovl_cattr {
>
> struct dentry *ovl_create_real(struct ovl_fs *ofs,
> struct dentry *parent, struct dentry *newdentry,
> + struct qstr *qname,
> struct ovl_cattr *attr);
> int ovl_cleanup(struct ovl_fs *ofs, struct dentry *workdir, struct dentry *dentry);
> #define OVL_TEMPNAME_SIZE 20
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index d4c12feec039..109643930b9f 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -634,6 +634,7 @@ static struct dentry *ovl_lookup_or_create(struct ovl_fs *ofs,
> if (!IS_ERR(child)) {
> if (!child->d_inode)
> child = ovl_create_real(ofs, parent, child,
> + &QSTR(name),
> OVL_CATTR(mode));
> end_creating_keep(child);
> }
> --
> 2.50.0.107.gf914562f5916.dirty
>
^ permalink raw reply
* Re: [syzbot] [kernel?] INFO: task hung in restrict_one_thread_callback
From: Günther Noack @ 2026-02-23 9:42 UTC (permalink / raw)
To: Ding Yihan
Cc: syzbot, Mickaël Salaün, linux-security-module,
Jann Horn, Paul Moore
In-Reply-To: <20260221.3ff0e30e4010@gnoack.org>
On Sat, Feb 21, 2026 at 02:19:53PM +0100, Günther Noack wrote:
> OK, I think I understand now. Our existing recovery code for this
> conflict is this:
>
> /*
> * Decrement num_preparing for current, to undo that we initialized it
> * to 1 a few lines above.
> */
> if (atomic_dec_return(&shared_ctx.num_preparing) > 0) {
> if (wait_for_completion_interruptible(
> &shared_ctx.all_prepared)) {
> /* In case of interruption, we need to retry the system call. */
> atomic_set(&shared_ctx.preparation_error,
> -ERESTARTNOINTR);
>
> /*
> * Cancel task works for tasks that did not start running yet,
> * and decrement all_prepared and num_unfinished accordingly.
> */
> cancel_tsync_works(&works, &shared_ctx);
>
> /*
> * The remaining task works have started running, so waiting for
> * their completion will finish.
> */
> wait_for_completion(&shared_ctx.all_prepared);
> }
> }
>
> When I wrote this, I assumed, as the last comment states, that the
> task works which we could not cancel, are already running.
>
> I was wrong there, because I had misunderstood task_work_run(). When
> the task works get run there, it first *atomically dequeues the entire
> queue of scheduled task works*, and then runs them sequentially.
>
> That is why, if we have one task work that belongs to the first
> landlock_restrict_self() call and one which belongs to the other, the
> task work which is scheduled later can (a) not be dequeued with
> cancel_tsync_works() any more, and (b) also has not started running
> yet.
>
> Now the only thing that is necessary to produce the deadlock is that
> we have a pair of threads where the task works for the restriction
> calls have been scheduled in different order. When the two
> landlock_restrict_self() calls end up in the recovery path quoted
> above, they will wait for one of their task works to run which is
> blocked from running by another task work that is scheduled before and
> does not finish either.
>
> (Just pasting a brain dump here to save you some time hunting for the
> root cause. I don't know the best solution yet either.)
Let me propose the following fixes:
1. Immediate fix for that specific issue
----------------------------------------
Proposal:
* Remove the wait_for_completion(&shared_ctx.all_prepared)
call in the code snippet above.
* Rewrite surrounding comments: Be clear about the fact that
cancel_tsync_works() is an opportunistic improvement, but we don't
have a guarantee at all that it cancels any of the enqueued task
works (because task_work_run might already have popped them off).
This removes the hold-and-wait dependency circle between the threads,
which produces the observed deadlock. The way that we shut down now
is that we exit the main loop (happens already without it, but we
might also "break" to be explicit).
I think that this fix or an equivalent one is needed here, because in
either way, our assumptions in the quoted code above were wrong.
2. Can we reason constructively about correctness?
--------------------------------------------------
The remaining question: If on the shutdown path, we can not actually
remove all the enqueued task works, under what circumstances are we
even able to interrupt and return from the landlock_restrict_self()
system call?
2.1 For n competing restrict_self calls, n-1 of them need to get interrupted
----------------------------------------------------------------------------
To answer this, consider a multithreaded process with threads named
"red", "green" and "blue" and many additional threads: When "red",
"green" and "blue" enforce landlock_restrict_self() concurrently, due
to differing iteration order, we might end up enqueueing the task
works on other threads in all of the following combinations:
t0: R G B <- front of queue
t1: R B G
t2: G R B
t3: G B R
t4: B R G
t5: B G R
In this configuration, for any of the landlock_restrict_self() system
calls to even return (successfully or unsuccessfully), at least two
threads must receive an interrupt and therefore remove their enqueued
task works from the front of the queue. Assuming those are green and
blue, we get:
t0: R <- front of queue
t1: R
t2: G R
t3: G B R
t4: B R
t5: B G R
(This works because after the patch above, all of the enqueued G and B
works finish even if there are remaining G and B works that are still
blocked by an "R" entry.)
Now, "R" is in the front of the queue, and the
landlock_restrict_self() call for the red thread can finish normally,
even without it being interrupted.
Once the "R" task works are done as well, the remaining G and B works
can run and finish as well.
This scheme generalizes: If we have n competing
landlock_restrict_self() calls, then in worst case, at least n-1 of
these system calls need to be interrupted so that they can all
terminate.
2.2 Can we guarantee that two system calls get interrupted?
-----------------------------------------------------------
In case of competing landlock_restrict_self() calls, I think it is
possible that not all relevant system calls get seen. The scenario is
one where we have a "red" and "green" thread calling
landlock_restrict_self().
(a set of additional threads)
t0: task_works: R G
t1: task_works: G R
tR: red thread
tG: green thread
In the red thread, the following happens:
* Under RCU, count the number of total threads => get a low number
* Allocate space for that number of task_works
* Under RCU
* Enqueue "R" into t0 and t1
* Enqueue "R" for some of the "additional threads"
* But we do not have enough pre-allocated space to enqueue "R" for
the green thread tG.
The same thing happens in the green thread as well.
The result is that we still have a deadlock between t0 and t1, but
neither the red nor the green thread get interrupted so that they can
resolve it.
(FWIW, you could resolve it from the outside by sending a signal to
the red or green thread manually, but it is not guaranteed to happen
on its own.)
Caveat: I am making pessimistic assumptions about the iteration order
of the task list here, and I am assuming that the number of
"additional threads" is swinging up and down during the competing
enforcement, so that the enforcing threads are mis-approximating the
required space for memory pre-allocation.
2.3 Possible resolutions
------------------------
* We could try to interrupt all sibling threads during the teardown,
to fix the issue discussed in 2.2. (Downside: Complicated, more
expensive)
* The reason why landlock_restrict_self() can't return is because it
needs to wait until all task works are done before it can free the
memory. Alternatively, we could make the task works take ownership
of these memory structures (refcounting the shared_ctx). (Downside:
The used memory is not linear to the number of threads any more.)
Side remark: In testing, I had the impression that the
landlock_restrict_self() calls can go into a retry loop for a while
where all competing threads get interrupted all the time; in a debug
build, when the Syzkaller test prints out a line for each attempt,
sometimes it was hanging for seconds and *then* resolving itself
again.
3 Conclusion
---------------
I would prefer if the final solution would not require deadlock
reasoning at that level and we could do it in simpler way. I
therefore propose to do what Ding Yihan suggested, and what we had
also discussed previously in the code review:
* Let's serialize the landlock_restrict_self()-with-TSYNC operations
through the cred_guard_mutex.
This will resolve the issue where competing landlock_restrict_self()
calls with TSYNC can deadlock. It will also remove the jittery
behavior for that worst case where the conflict is resolved through
retry.
So in my mind, we need both patches:
* The fix to the cleanup path from 1. above, to make interruption
work more reliably and to correct the misunderstandings in the
comments.
* cred_guard_mutex to serialize the TSYNC invocations.
Please let me know what you think.
Thanks,
–Günther
^ permalink raw reply
* Re: [PATCH v2 09/15] ovl: Simplify ovl_lookup_real_one()
From: Amir Goldstein @ 2026-02-23 9:49 UTC (permalink / raw)
To: NeilBrown
Cc: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Jeff Layton, Miklos Szeredi, John Johansen,
Paul Moore, James Morris, Serge E. Hallyn, Stephen Smalley,
Darrick J. Wong, linux-kernel, netfs, linux-fsdevel, linux-nfs,
linux-unionfs, apparmor, linux-security-module, selinux
In-Reply-To: <20260223011210.3853517-10-neilb@ownmail.net>
On Mon, Feb 23, 2026 at 2:13 AM NeilBrown <neilb@ownmail.net> wrote:
>
> From: NeilBrown <neil@brown.name>
>
> The primary purpose of this patch is to remove the locking from
> ovl_lookup_real_one() as part of centralising all locking of directories
> for name operations.
>
> The locking here isn't needed. By performing consistency tests after
> the lookup we can be sure that the result of the lookup was valid at
> least for a moment, which is all the original code promised.
>
> lookup_noperm_unlocked() is used for the lookup and it will take the
> lock if needed only where it is needed.
>
> Also:
> - don't take a reference to real->d_parent. The parent is
> only use for a pointer comparison, and no reference is needed for
> that.
> - Several "if" statements have a "goto" followed by "else" - the
> else isn't needed: the following statement can directly follow
> the "if" as a new statement
> - Use a consistent pattern of setting "err" before performing a test
> and possibly going to "fail".
> - remove the "out" label (now that we don't need to dput(parent) or
> unlock) and simply return from fail:.
>
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> Signed-off-by: NeilBrown <neil@brown.name>
Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> ---
> fs/overlayfs/export.c | 71 ++++++++++++++++++++-----------------------
> 1 file changed, 33 insertions(+), 38 deletions(-)
>
> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> index 83f80fdb1567..b448fc9424b6 100644
> --- a/fs/overlayfs/export.c
> +++ b/fs/overlayfs/export.c
> @@ -349,69 +349,64 @@ static struct dentry *ovl_dentry_real_at(struct dentry *dentry, int idx)
> return NULL;
> }
>
> -/*
> - * Lookup a child overlay dentry to get a connected overlay dentry whose real
> - * dentry is @real. If @real is on upper layer, we lookup a child overlay
> - * dentry with the same name as the real dentry. Otherwise, we need to consult
> - * index for lookup.
> +/**
> + * ovl_lookup_real_one - Lookup a child overlay dentry to get an overlay dentry whose real dentry is given
> + * @connected: parent overlay dentry
> + * @real: given child real dentry
> + * @layer: layer in which @real exists
> + *
> + *
> + * Lookup a child overlay dentry in @connected with the same name as the @real
> + * dentry. Then check that the parent of the result is the real dentry for
> + * @connected, and @real is the real dentry for the result.
> + *
> + * Returns:
> + * %-ECHILD if the parent of @real is no longer the real dentry for @connected.
> + * %-ESTALE if @real is no the real dentry of the found dentry.
> + * Otherwise the found dentry is returned.
> */
> static struct dentry *ovl_lookup_real_one(struct dentry *connected,
> struct dentry *real,
> const struct ovl_layer *layer)
> {
> - struct inode *dir = d_inode(connected);
> - struct dentry *this, *parent = NULL;
> + struct dentry *this;
> struct name_snapshot name;
> int err;
>
> /*
> - * Lookup child overlay dentry by real name. The dir mutex protects us
> - * from racing with overlay rename. If the overlay dentry that is above
> - * real has already been moved to a parent that is not under the
> - * connected overlay dir, we return -ECHILD and restart the lookup of
> - * connected real path from the top.
> - */
> - inode_lock_nested(dir, I_MUTEX_PARENT);
> - err = -ECHILD;
> - parent = dget_parent(real);
> - if (ovl_dentry_real_at(connected, layer->idx) != parent)
> - goto fail;
> -
> - /*
> - * We also need to take a snapshot of real dentry name to protect us
> + * We need to take a snapshot of real dentry name to protect us
> * from racing with underlying layer rename. In this case, we don't
> * care about returning ESTALE, only from dereferencing a free name
> * pointer because we hold no lock on the real dentry.
> */
> take_dentry_name_snapshot(&name, real);
> - /*
> - * No idmap handling here: it's an internal lookup.
> - */
> - this = lookup_noperm(&name.name, connected);
> + this = lookup_noperm_unlocked(&name.name, connected);
> release_dentry_name_snapshot(&name);
> +
> + err = -ECHILD;
> + if (ovl_dentry_real_at(connected, layer->idx) != real->d_parent)
> + goto fail;
> +
> err = PTR_ERR(this);
> - if (IS_ERR(this)) {
> + if (IS_ERR(this))
> goto fail;
> - } else if (!this || !this->d_inode) {
> - dput(this);
> - err = -ENOENT;
> +
> + err = -ENOENT;
> + if (!this || !this->d_inode)
> goto fail;
> - } else if (ovl_dentry_real_at(this, layer->idx) != real) {
> - dput(this);
> - err = -ESTALE;
> +
> + err = -ESTALE;
> + if (ovl_dentry_real_at(this, layer->idx) != real)
> goto fail;
> - }
>
> -out:
> - dput(parent);
> - inode_unlock(dir);
> return this;
>
> fail:
> pr_warn_ratelimited("failed to lookup one by real (%pd2, layer=%d, connected=%pd2, err=%i)\n",
> real, layer->idx, connected, err);
> - this = ERR_PTR(err);
> - goto out;
> + if (!IS_ERR(this))
> + dput(this);
> + return ERR_PTR(err);
> }
>
> static struct dentry *ovl_lookup_real(struct super_block *sb,
> --
> 2.50.0.107.gf914562f5916.dirty
>
^ permalink raw reply
* Re: [syzbot] [kernel?] INFO: task hung in restrict_one_thread_callback
From: Ding Yihan @ 2026-02-23 11:29 UTC (permalink / raw)
To: Günther Noack
Cc: syzbot, Mickaël Salaün, linux-security-module,
Jann Horn, Paul Moore
In-Reply-To: <20260223.52c45aed20f8@gnoack.org>
Hi Günther,
Thank you for the detailed analysis and the clear breakdown.
Apologies for the delayed response. I spent the last couple of days
thoroughly reading through the previous mailing list discussions. I
was trying hard to see if there was any viable pure lockless design
that could solve this concurrency issue while preserving the original
architecture.
However, after looking at the complexities you outlined, I completely
agree with your conclusion: serializing the TSYNC operations is indeed
the most robust and reasonable path forward to prevent the deadlock.
Regarding the lock choice, since 'cred_guard_mutex' is explicitly
marked as deprecated for new code in the kernel,maybe we can use its
modern replacement: 'exec_update_lock' (using down_write_trylock /
up_write on current->signal). This aligns with the current subsystem
standards and was also briefly touched upon by Jann in the older
discussions.
I fully understand the requirement for the two-part patch series:
1. Cleaning up the cancellation logic and comments.
2. Introducing the serialization lock for TSYNC.
I will take some time to draft and test this patch series properly.
I also plan to discuss this with my kernel colleagues here at
UnionTech to see if they have any additional suggestions on the
implementation details before I submit it.
I will send out the v1 patch series to the list as soon as it is
ready. Thanks again for your guidance and the great discussion!
Best regards,
Yihan Ding
在 2026/2/23 17:42, Günther Noack 写道:
> On Sat, Feb 21, 2026 at 02:19:53PM +0100, Günther Noack wrote:
>> OK, I think I understand now. Our existing recovery code for this
>> conflict is this:
>>
>> /*
>> * Decrement num_preparing for current, to undo that we initialized it
>> * to 1 a few lines above.
>> */
>> if (atomic_dec_return(&shared_ctx.num_preparing) > 0) {
>> if (wait_for_completion_interruptible(
>> &shared_ctx.all_prepared)) {
>> /* In case of interruption, we need to retry the system call. */
>> atomic_set(&shared_ctx.preparation_error,
>> -ERESTARTNOINTR);
>>
>> /*
>> * Cancel task works for tasks that did not start running yet,
>> * and decrement all_prepared and num_unfinished accordingly.
>> */
>> cancel_tsync_works(&works, &shared_ctx);
>>
>> /*
>> * The remaining task works have started running, so waiting for
>> * their completion will finish.
>> */
>> wait_for_completion(&shared_ctx.all_prepared);
>> }
>> }
>>
>> When I wrote this, I assumed, as the last comment states, that the
>> task works which we could not cancel, are already running.
>>
>> I was wrong there, because I had misunderstood task_work_run(). When
>> the task works get run there, it first *atomically dequeues the entire
>> queue of scheduled task works*, and then runs them sequentially.
>>
>> That is why, if we have one task work that belongs to the first
>> landlock_restrict_self() call and one which belongs to the other, the
>> task work which is scheduled later can (a) not be dequeued with
>> cancel_tsync_works() any more, and (b) also has not started running
>> yet.
>>
>> Now the only thing that is necessary to produce the deadlock is that
>> we have a pair of threads where the task works for the restriction
>> calls have been scheduled in different order. When the two
>> landlock_restrict_self() calls end up in the recovery path quoted
>> above, they will wait for one of their task works to run which is
>> blocked from running by another task work that is scheduled before and
>> does not finish either.
>>
>> (Just pasting a brain dump here to save you some time hunting for the
>> root cause. I don't know the best solution yet either.)
>
> Let me propose the following fixes:
>
> 1. Immediate fix for that specific issue
> ----------------------------------------
>
> Proposal:
> * Remove the wait_for_completion(&shared_ctx.all_prepared)
> call in the code snippet above.
> * Rewrite surrounding comments: Be clear about the fact that
> cancel_tsync_works() is an opportunistic improvement, but we don't
> have a guarantee at all that it cancels any of the enqueued task
> works (because task_work_run might already have popped them off).
>
> This removes the hold-and-wait dependency circle between the threads,
> which produces the observed deadlock. The way that we shut down now
> is that we exit the main loop (happens already without it, but we
> might also "break" to be explicit).
>
> I think that this fix or an equivalent one is needed here, because in
> either way, our assumptions in the quoted code above were wrong.
>
>
> 2. Can we reason constructively about correctness?
> --------------------------------------------------
>
> The remaining question: If on the shutdown path, we can not actually
> remove all the enqueued task works, under what circumstances are we
> even able to interrupt and return from the landlock_restrict_self()
> system call?
>
> 2.1 For n competing restrict_self calls, n-1 of them need to get interrupted
> ----------------------------------------------------------------------------
>
> To answer this, consider a multithreaded process with threads named
> "red", "green" and "blue" and many additional threads: When "red",
> "green" and "blue" enforce landlock_restrict_self() concurrently, due
> to differing iteration order, we might end up enqueueing the task
> works on other threads in all of the following combinations:
>
> t0: R G B <- front of queue
> t1: R B G
> t2: G R B
> t3: G B R
> t4: B R G
> t5: B G R
>
> In this configuration, for any of the landlock_restrict_self() system
> calls to even return (successfully or unsuccessfully), at least two
> threads must receive an interrupt and therefore remove their enqueued
> task works from the front of the queue. Assuming those are green and
> blue, we get:
>
> t0: R <- front of queue
> t1: R
> t2: G R
> t3: G B R
> t4: B R
> t5: B G R
>
> (This works because after the patch above, all of the enqueued G and B
> works finish even if there are remaining G and B works that are still
> blocked by an "R" entry.)
>
> Now, "R" is in the front of the queue, and the
> landlock_restrict_self() call for the red thread can finish normally,
> even without it being interrupted.
>
> Once the "R" task works are done as well, the remaining G and B works
> can run and finish as well.
>
> This scheme generalizes: If we have n competing
> landlock_restrict_self() calls, then in worst case, at least n-1 of
> these system calls need to be interrupted so that they can all
> terminate.
>
> 2.2 Can we guarantee that two system calls get interrupted?
> -----------------------------------------------------------
>
> In case of competing landlock_restrict_self() calls, I think it is
> possible that not all relevant system calls get seen. The scenario is
> one where we have a "red" and "green" thread calling
> landlock_restrict_self().
>
> (a set of additional threads)
> t0: task_works: R G
> t1: task_works: G R
> tR: red thread
> tG: green thread
>
> In the red thread, the following happens:
> * Under RCU, count the number of total threads => get a low number
> * Allocate space for that number of task_works
> * Under RCU
> * Enqueue "R" into t0 and t1
> * Enqueue "R" for some of the "additional threads"
> * But we do not have enough pre-allocated space to enqueue "R" for
> the green thread tG.
>
> The same thing happens in the green thread as well.
>
> The result is that we still have a deadlock between t0 and t1, but
> neither the red nor the green thread get interrupted so that they can
> resolve it.
>
> (FWIW, you could resolve it from the outside by sending a signal to
> the red or green thread manually, but it is not guaranteed to happen
> on its own.)
>
> Caveat: I am making pessimistic assumptions about the iteration order
> of the task list here, and I am assuming that the number of
> "additional threads" is swinging up and down during the competing
> enforcement, so that the enforcing threads are mis-approximating the
> required space for memory pre-allocation.
>
> 2.3 Possible resolutions
> ------------------------
>
> * We could try to interrupt all sibling threads during the teardown,
> to fix the issue discussed in 2.2. (Downside: Complicated, more
> expensive)
> * The reason why landlock_restrict_self() can't return is because it
> needs to wait until all task works are done before it can free the
> memory. Alternatively, we could make the task works take ownership
> of these memory structures (refcounting the shared_ctx). (Downside:
> The used memory is not linear to the number of threads any more.)
>
> Side remark: In testing, I had the impression that the
> landlock_restrict_self() calls can go into a retry loop for a while
> where all competing threads get interrupted all the time; in a debug
> build, when the Syzkaller test prints out a line for each attempt,
> sometimes it was hanging for seconds and *then* resolving itself
> again.
>
> 3 Conclusion
> ---------------
>
> I would prefer if the final solution would not require deadlock
> reasoning at that level and we could do it in simpler way. I
> therefore propose to do what Ding Yihan suggested, and what we had
> also discussed previously in the code review:
>
> * Let's serialize the landlock_restrict_self()-with-TSYNC operations
> through the cred_guard_mutex.
>
> This will resolve the issue where competing landlock_restrict_self()
> calls with TSYNC can deadlock. It will also remove the jittery
> behavior for that worst case where the conflict is resolved through
> retry.
>
>
> So in my mind, we need both patches:
>
> * The fix to the cleanup path from 1. above, to make interruption
> work more reliably and to correct the misunderstandings in the
> comments.
> * cred_guard_mutex to serialize the TSYNC invocations.
>
> Please let me know what you think.
>
> Thanks,
> –Günther
>
^ permalink raw reply
* Re: [PATCH v2 09/15] ovl: Simplify ovl_lookup_real_one()
From: Chris Mason @ 2026-02-23 13:13 UTC (permalink / raw)
To: NeilBrown
Cc: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Jeff Layton, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley, Darrick J. Wong, linux-kernel, netfs,
linux-fsdevel, linux-nfs, linux-unionfs, apparmor,
linux-security-module, selinux
In-Reply-To: <20260223011210.3853517-10-neilb@ownmail.net>
NeilBrown <neilb@ownmail.net> wrote:
> From: NeilBrown <neil@brown.name>
>
> The primary purpose of this patch is to remove the locking from
> ovl_lookup_real_one() as part of centralising all locking of directories
> for name operations.
>
> The locking here isn't needed. By performing consistency tests after
> the lookup we can be sure that the result of the lookup was valid at
> least for a moment, which is all the original code promised.
>
> lookup_noperm_unlocked() is used for the lookup and it will take the
> lock if needed only where it is needed.
>
> Also:
> - don't take a reference to real->d_parent. The parent is
> only use for a pointer comparison, and no reference is needed for
> that.
> - Several "if" statements have a "goto" followed by "else" - the
> else isn't needed: the following statement can directly follow
> the "if" as a new statement
> - Use a consistent pattern of setting "err" before performing a test
> and possibly going to "fail".
> - remove the "out" label (now that we don't need to dput(parent) or
> unlock) and simply return from fail:.
Hi everyone,
Amir asked me to run these through, and this commit was flagged. I recently
changed the prompts to include some vfs knowledge:
https://github.com/masoncl/review-prompts/blob/main/kernel/subsystem/vfs.md
And I also put in some words to make it prefer reporting problems that might
not be bugs, but that do violate some rule in the .md files. This might
be a false positive from that new shift toward blindly trusting the guides,
but I'll let everyone here decide:
commit d1bfcef048896cdc9e4c57e3249b6ad344d64c02
Author: NeilBrown <neil@brown.name>
ovl: Simplify ovl_lookup_real_one()
This patch removes locking from ovl_lookup_real_one() and reorganizes the
control flow. The function now uses lookup_noperm_unlocked() instead of
lookup_noperm() and performs consistency validation after the lookup rather
than holding a lock during the operation.
> diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> index 83f80fdb15674..b448fc9424b64 100644
> --- a/fs/overlayfs/export.c
> +++ b/fs/overlayfs/export.c
> @@ -349,69 +349,64 @@ static struct dentry *ovl_dentry_real_at(struct dentry *dentry, int idx)
> return NULL;
> }
>
> -/*
> - * Lookup a child overlay dentry to get a connected overlay dentry whose real
> - * dentry is @real. If @real is on upper layer, we lookup a child overlay
> - * dentry with the same name as the real dentry. Otherwise, we need to consult
> - * index for lookup.
> +/**
> + * ovl_lookup_real_one - Lookup a child overlay dentry to get an overlay dentry whose real dentry is given
> + * @connected: parent overlay dentry
> + * @real: given child real dentry
> + * @layer: layer in which @real exists
> + *
> + *
> + * Lookup a child overlay dentry in @connected with the same name as the @real
> + * dentry. Then check that the parent of the result is the real dentry for
> + * @connected, and @real is the real dentry for the result.
> + *
> + * Returns:
> + * %-ECHILD if the parent of @real is no longer the real dentry for @connected.
> + * %-ESTALE if @real is no the real dentry of the found dentry.
^^
There's a typo here: "is no the" should be "is not the".
> + * Otherwise the found dentry is returned.
> */
> static struct dentry *ovl_lookup_real_one(struct dentry *connected,
> struct dentry *real,
> const struct ovl_layer *layer)
> {
> - struct inode *dir = d_inode(connected);
> - struct dentry *this, *parent = NULL;
> + struct dentry *this;
> struct name_snapshot name;
> int err;
>
> /*
> - * Lookup child overlay dentry by real name. The dir mutex protects us
> - * from racing with overlay rename. If the overlay dentry that is above
> - * real has already been moved to a parent that is not under the
> - * connected overlay dir, we return -ECHILD and restart the lookup of
> - * connected real path from the top.
> - */
> - inode_lock_nested(dir, I_MUTEX_PARENT);
> - err = -ECHILD;
> - parent = dget_parent(real);
> - if (ovl_dentry_real_at(connected, layer->idx) != parent)
> - goto fail;
> -
> - /*
> - * We also need to take a snapshot of real dentry name to protect us
> + * We need to take a snapshot of real dentry name to protect us
> * from racing with underlying layer rename. In this case, we don't
> * care about returning ESTALE, only from dereferencing a free name
> * pointer because we hold no lock on the real dentry.
> */
> take_dentry_name_snapshot(&name, real);
> - /*
> - * No idmap handling here: it's an internal lookup.
> - */
> - this = lookup_noperm(&name.name, connected);
> + this = lookup_noperm_unlocked(&name.name, connected);
> release_dentry_name_snapshot(&name);
> +
> + err = -ECHILD;
> + if (ovl_dentry_real_at(connected, layer->idx) != real->d_parent)
> + goto fail;
> +
A subsystem pattern flags this as potentially concerning: the VFS subsystem
guide indicates that TOCTOU validation checking d_parent should also check
d_unhashed() because a dentry can be unhashed (removed from dcache by
concurrent operations) while its d_parent pointer remains unchanged.
The old code held inode_lock_nested(dir, I_MUTEX_PARENT) during the entire
lookup and validation, which serialized with operations that could unhash the
dentry. The new lockless pattern creates a window where:
1. lookup_noperm_unlocked() completes and returns dentry `this`
2. A concurrent unlink/remove operation unhashes `this`
3. The d_parent validation passes (parent pointer unchanged)
4. The function returns an unhashed dentry
Other overlayfs code uses the complete pattern. For example, in
ovl_lock_rename_workdir() at fs/overlayfs/util.c:
if (work->d_parent != workdir || d_unhashed(work)) {
...
}
Could this validation miss a race where the dentry is unhashed but d_parent
still matches? The subsequent checks (!this->d_inode for -ENOENT,
wrong real dentry for -ESTALE) might catch some cases, but is that
sufficient for all the scenarios where an unhashed dentry could cause
problems?
> err = PTR_ERR(this);
> - if (IS_ERR(this)) {
> + if (IS_ERR(this))
> goto fail;
> - } else if (!this || !this->d_inode) {
> - dput(this);
> - err = -ENOENT;
> +
> + err = -ENOENT;
> + if (!this || !this->d_inode)
> goto fail;
> - } else if (ovl_dentry_real_at(this, layer->idx) != real) {
> - dput(this);
> - err = -ESTALE;
> +
> + err = -ESTALE;
> + if (ovl_dentry_real_at(this, layer->idx) != real)
> goto fail;
> - }
>
> -out:
> - dput(parent);
> - inode_unlock(dir);
> return this;
>
> fail:
> pr_warn_ratelimited("failed to lookup one by real (%pd2, layer=%d, connected=%pd2, err=%i)\n",
> real, layer->idx, connected, err);
> - this = ERR_PTR(err);
> - goto out;
> + if (!IS_ERR(this))
> + dput(this);
> + return ERR_PTR(err);
> }
>
> static struct dentry *ovl_lookup_real(struct super_block *sb,
^ permalink raw reply
* Re: [PATCH v2 12/15] ovl: change ovl_create_real() to get a new lock when re-opening created file.
From: Chris Mason @ 2026-02-23 13:23 UTC (permalink / raw)
To: NeilBrown
Cc: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Jeff Layton, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley, Darrick J. Wong, linux-kernel, netfs,
linux-fsdevel, linux-nfs, linux-unionfs, apparmor,
linux-security-module, selinux
In-Reply-To: <20260223011210.3853517-13-neilb@ownmail.net>
NeilBrown <neilb@ownmail.net> wrote:
> From: NeilBrown <neil@brown.name>
>
> When ovl_create_real() is used to create a file on the upper filesystem
> it needs to return the resulting dentry - positive and hashed.
> It is usually the case the that dentry passed to the create function
> (e.g. vfs_create()) will be suitable but this is not guaranteed. The
> filesystem may unhash that dentry forcing a repeat lookup next time the
> name is wanted.
>
Hi everyone,
Amir suggested I run these through, and this commit was flagged:
commit 62d49d1e44667e4f93bec415faabec5526992ac0
Author: NeilBrown <neil@brown.name>
ovl: change ovl_create_real() to get a new lock when re-opening created file.
This commit changes ovl_create_real() to drop the directory lock and
reacquire a new lock for lookup when the created dentry is unhashed. It
also removes ovl_lookup_upper() which is no longer used.
Signed-off-by: NeilBrown <neil@brown.name>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index c4feb89ad1e39..6285069ccc590 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -159,7 +159,8 @@ int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *dir,
> }
>
> struct dentry *ovl_create_real(struct ovl_fs *ofs, struct dentry *parent,
> - struct dentry *newdentry, struct ovl_cattr *attr)
> + struct dentry *newdentry, struct qstr *qname,
> + struct ovl_cattr *attr)
> {
> struct inode *dir = parent->d_inode;
> int err;
> @@ -221,19 +222,29 @@ struct dentry *ovl_create_real(struct ovl_fs *ofs, struct dentry *parent,
> struct dentry *d;
> /*
> * Some filesystems (i.e. casefolded) may return an unhashed
> - * negative dentry from the ovl_lookup_upper() call before
> + * negative dentry from the ovl_start_creating_upper() call before
> * ovl_create_real().
> * In that case, lookup again after making the newdentry
> * positive, so ovl_create_upper() always returns a hashed
> - * positive dentry.
> + * positive dentry. We lookup using qname which should be
> + * the same name as newentry, but is certain not to change.
> + * As we have to drop the lock before the lookup a race
> + * could result in a lookup failure. In that case we return
> + * an error.
> */
> - d = ovl_lookup_upper(ofs, newdentry->d_name.name, parent,
> - newdentry->d_name.len);
> - dput(newdentry);
> - if (IS_ERR_OR_NULL(d))
> + end_creating_keep(newdentry);
> + d = ovl_start_creating_upper(ofs, parent, qname);
> +
> + if (IS_ERR_OR_NULL(d)) {
> err = d ? PTR_ERR(d) : -ENOENT;
> + } else if (d->d_inode != newdentry->d_inode) {
> + err = -EIO;
> + dput(newdentry);
> + } else {
> + dput(newdentry);
> return d;
> + }
> + return ERR_PTR(err);
> }
Does this leak the newdentry reference when IS_ERR_OR_NULL(d) is true?
The code now calls end_creating_keep(newdentry) which keeps the reference,
then calls ovl_start_creating_upper(). If that returns an error or NULL,
the function returns ERR_PTR(err) without cleaning up newdentry:
end_creating_keep(newdentry);
d = ovl_start_creating_upper(ofs, parent, qname);
if (IS_ERR_OR_NULL(d)) {
err = d ? PTR_ERR(d) : -ENOENT;
} ...
return ERR_PTR(err);
The old code always went through the 'out:' label which called
end_creating(newdentry) on error. The new code bypasses that cleanup.
Does this leak both the parent lock and the dentry 'd' when the lookup
returns a different inode?
When d->d_inode != newdentry->d_inode, the code sets err = -EIO and calls
dput(newdentry), but doesn't clean up 'd':
} else if (d->d_inode != newdentry->d_inode) {
err = -EIO;
dput(newdentry);
} ...
return ERR_PTR(err);
The dentry 'd' was returned by ovl_start_creating_upper() with the parent
directory lock held. The code needs to call end_creating(d) to release the
lock via end_dirop()->inode_unlock() and release the dentry reference via
end_dirop()->dput().
^ permalink raw reply
* Re: [PATCH v2 06/15] selinux: Use simple_start_creating() / simple_done_creating()
From: Chris Mason @ 2026-02-23 13:24 UTC (permalink / raw)
To: NeilBrown
Cc: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Jeff Layton, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley, Darrick J. Wong, linux-kernel, netfs,
linux-fsdevel, linux-nfs, linux-unionfs, apparmor,
linux-security-module, selinux
In-Reply-To: <20260223011210.3853517-7-neilb@ownmail.net>
NeilBrown <neilb@ownmail.net> wrote:
> From: NeilBrown <neil@brown.name>
>
> Instead of explicitly locking the parent and performing a lookup in
> selinux, use simple_start_creating(), and then use
> simple_done_creating() to unlock.
>
Hi everyone,
Amir suggested I run these through, and this commit was flagged:
commit bc981629767980bbaee2ca3c7348b5a648665a9d
Author: NeilBrown <neil@brown.name>
selinux: Use simple_start_creating() / simple_done_creating()
Instead of explicitly locking the parent and performing a lookup in
selinux, use simple_start_creating(), and then use
simple_done_creating() to unlock.
This extends the region that the directory is locked for, and also
performs a lookup. The lock extension is of no real consequence.
The lookup uses simple_lookup() and so always succeeds. Thus when
d_make_persistent() is called the dentry will already be hashed.
d_make_persistent() handles this case.
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Acked-by: Paul Moore <paul@paul-moore.com>
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index 3245cc531555b..7d4f90e5b12a0 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -1931,15 +1931,16 @@ static const struct inode_operations swapover_dir_inode_operations = {
> static struct dentry *sel_make_swapover_dir(struct super_block *sb,
> unsigned long *ino)
> {
> - struct dentry *dentry = d_alloc_name(sb->s_root, ".swapover");
> + struct dentry *dentry;
> struct inode *inode;
>
> - if (!dentry)
> + inode = sel_make_inode(sb, S_IFDIR);
> + if (!inode)
> return ERR_PTR(-ENOMEM);
>
> - inode = sel_make_inode(sb, S_IFDIR);
> - if (!inode) {
> - dput(dentry);
> + dentry = simple_start_creating(sb->s_root, ".swapover");
> + if (!dentry) {
> + iput(inode);
> return ERR_PTR(-ENOMEM);
> }
Can this cause a kernel crash when simple_start_creating() fails?
The function simple_start_creating() returns ERR_PTR on failure, not
NULL. The check `if (!dentry)` only catches NULL values. When
simple_start_creating() fails and returns an ERR_PTR value like
ERR_PTR(-ENOMEM), this value is non-NULL and the error check is bypassed.
Execution would continue to d_make_persistent(dentry, inode), which
attempts to lock dentry->d_lock, dereferencing the ERR_PTR value and
causing a kernel crash.
The old code used d_alloc_name() which returns NULL on error. The new
code uses simple_start_creating() which returns ERR_PTR on error, but
the error checking pattern wasn't updated to match.
Should the check be `if (IS_ERR(dentry))` instead, and return dentry
rather than ERR_PTR(-ENOMEM) to preserve the actual error code?
>
> @@ -1947,11 +1948,9 @@ static struct dentry *sel_make_swapover_dir(struct super_block *sb,
> inode->i_ino = ++(*ino);
> /* directory inodes start off with i_nlink == 2 (for "." entry) */
> inc_nlink(inode);
> - inode_lock(sb->s_root->d_inode);
> d_make_persistent(dentry, inode);
> inc_nlink(sb->s_root->d_inode);
> - inode_unlock(sb->s_root->d_inode);
> - dput(dentry);
> + simple_done_creating(dentry);
> return dentry; // borrowed
> }
^ permalink raw reply
* Re: [syzbot] [kernel?] INFO: task hung in restrict_one_thread_callback
From: Frederic Weisbecker @ 2026-02-23 13:40 UTC (permalink / raw)
To: syzbot, Mickaël Salaün, Günther Noack, Paul Moore,
James Morris, Serge E. Hallyn, linux-security-module
Cc: anna-maria, linux-kernel, syzkaller-bugs, tglx
In-Reply-To: <69984159.050a0220.21cd75.01bb.GAE@google.com>
Le Fri, Feb 20, 2026 at 03:11:21AM -0800, syzbot a écrit :
> Hello,
>
> syzbot found the following issue on:
>
> HEAD commit: 635c467cc14e Add linux-next specific files for 20260213
> git tree: linux-next
> console output: https://syzkaller.appspot.com/x/log.txt?x=1452f6e6580000
> kernel config: https://syzkaller.appspot.com/x/.config?x=61690c38d1398936
> dashboard link: https://syzkaller.appspot.com/bug?extid=7ea2f5e9dfd468201817
> compiler: Debian clang version 21.1.8 (++20251221033036+2078da43e25a-1~exp1~20251221153213.50), Debian LLD 21.1.8
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=16e41c02580000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=15813652580000
>
> Downloadable assets:
> disk image: https://storage.googleapis.com/syzbot-assets/78b3d15ca8e6/disk-635c467c.raw.xz
> vmlinux: https://storage.googleapis.com/syzbot-assets/a95f3d108ef4/vmlinux-635c467c.xz
> kernel image: https://storage.googleapis.com/syzbot-assets/e58086838b24/bzImage-635c467c.xz
>
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+7ea2f5e9dfd468201817@syzkaller.appspotmail.com
>
> INFO: task syz.0.2812:14643 blocked for more than 143 seconds.
> Not tainted syzkaller #0
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> task:syz.0.2812 state:D stack:25600 pid:14643 tgid:14643 ppid:13375 task_flags:0x400040 flags:0x00080002
> Call Trace:
> <TASK>
> context_switch kernel/sched/core.c:5295 [inline]
> __schedule+0x1585/0x5340 kernel/sched/core.c:6907
> __schedule_loop kernel/sched/core.c:6989 [inline]
> schedule+0x164/0x360 kernel/sched/core.c:7004
> schedule_timeout+0xc3/0x2c0 kernel/time/sleep_timeout.c:75
> do_wait_for_common kernel/sched/completion.c:100 [inline]
> __wait_for_common kernel/sched/completion.c:121 [inline]
> wait_for_common kernel/sched/completion.c:132 [inline]
> wait_for_completion+0x2cc/0x5e0 kernel/sched/completion.c:153
> restrict_one_thread security/landlock/tsync.c:128 [inline]
> restrict_one_thread_callback+0x320/0x570 security/landlock/tsync.c:162
Seems to be related to landlock security module.
Cc'ing maintainers for awareness.
Thanks.
> task_work_run+0x1d9/0x270 kernel/task_work.c:233
> get_signal+0x11eb/0x1330 kernel/signal.c:2807
> arch_do_signal_or_restart+0xbc/0x830 arch/x86/kernel/signal.c:337
> __exit_to_user_mode_loop kernel/entry/common.c:64 [inline]
> exit_to_user_mode_loop+0x86/0x480 kernel/entry/common.c:98
> __exit_to_user_mode_prepare include/linux/irq-entry-common.h:226 [inline]
> syscall_exit_to_user_mode_prepare include/linux/irq-entry-common.h:256 [inline]
> syscall_exit_to_user_mode include/linux/entry-common.h:325 [inline]
> do_syscall_64+0x32d/0xf80 arch/x86/entry/syscall_64.c:100
> entry_SYSCALL_64_after_hwframe+0x77/0x7f
> RIP: 0033:0x7f8d7f19bf79
> RSP: 002b:00007ffe0b192a38 EFLAGS: 00000246 ORIG_RAX: 00000000000000db
> RAX: fffffffffffffdfc RBX: 00000000000389f1 RCX: 00007f8d7f19bf79
> RDX: 0000000000000000 RSI: 0000000000000080 RDI: 00007f8d7f41618c
> RBP: 0000000000000032 R08: 3fffffffffffffff R09: 0000000000000000
> R10: 00007ffe0b192b40 R11: 0000000000000246 R12: 00007ffe0b192b60
> R13: 00007f8d7f41618c R14: 0000000000038a23 R15: 00007ffe0b192b40
> </TASK>
> INFO: task syz.0.2812:14644 blocked for more than 143 seconds.
> Not tainted syzkaller #0
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> task:syz.0.2812 state:D stack:28216 pid:14644 tgid:14643 ppid:13375 task_flags:0x400040 flags:0x00080002
> Call Trace:
> <TASK>
> context_switch kernel/sched/core.c:5295 [inline]
> __schedule+0x1585/0x5340 kernel/sched/core.c:6907
> __schedule_loop kernel/sched/core.c:6989 [inline]
> schedule+0x164/0x360 kernel/sched/core.c:7004
> schedule_timeout+0xc3/0x2c0 kernel/time/sleep_timeout.c:75
> do_wait_for_common kernel/sched/completion.c:100 [inline]
> __wait_for_common kernel/sched/completion.c:121 [inline]
> wait_for_common kernel/sched/completion.c:132 [inline]
> wait_for_completion+0x2cc/0x5e0 kernel/sched/completion.c:153
> restrict_one_thread security/landlock/tsync.c:128 [inline]
> restrict_one_thread_callback+0x320/0x570 security/landlock/tsync.c:162
> task_work_run+0x1d9/0x270 kernel/task_work.c:233
> get_signal+0x11eb/0x1330 kernel/signal.c:2807
> arch_do_signal_or_restart+0xbc/0x830 arch/x86/kernel/signal.c:337
> __exit_to_user_mode_loop kernel/entry/common.c:64 [inline]
> exit_to_user_mode_loop+0x86/0x480 kernel/entry/common.c:98
> __exit_to_user_mode_prepare include/linux/irq-entry-common.h:226 [inline]
> syscall_exit_to_user_mode_prepare include/linux/irq-entry-common.h:256 [inline]
> syscall_exit_to_user_mode include/linux/entry-common.h:325 [inline]
> do_syscall_64+0x32d/0xf80 arch/x86/entry/syscall_64.c:100
> entry_SYSCALL_64_after_hwframe+0x77/0x7f
> RIP: 0033:0x7f8d7f19bf79
> RSP: 002b:00007f8d8007c0e8 EFLAGS: 00000246 ORIG_RAX: 00000000000000ca
> RAX: fffffffffffffe00 RBX: 00007f8d7f415fa8 RCX: 00007f8d7f19bf79
> RDX: 0000000000000000 RSI: 0000000000000080 RDI: 00007f8d7f415fa8
> RBP: 00007f8d7f415fa0 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
> R13: 00007f8d7f416038 R14: 00007ffe0b1927f0 R15: 00007ffe0b1928d8
> </TASK>
> INFO: task syz.0.2812:14645 blocked for more than 143 seconds.
> Not tainted syzkaller #0
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> task:syz.0.2812 state:D stack:28648 pid:14645 tgid:14643 ppid:13375 task_flags:0x400140 flags:0x00080006
> Call Trace:
> <TASK>
> context_switch kernel/sched/core.c:5295 [inline]
> __schedule+0x1585/0x5340 kernel/sched/core.c:6907
> __schedule_loop kernel/sched/core.c:6989 [inline]
> schedule+0x164/0x360 kernel/sched/core.c:7004
> schedule_timeout+0xc3/0x2c0 kernel/time/sleep_timeout.c:75
> do_wait_for_common kernel/sched/completion.c:100 [inline]
> __wait_for_common kernel/sched/completion.c:121 [inline]
> wait_for_common kernel/sched/completion.c:132 [inline]
> wait_for_completion+0x2cc/0x5e0 kernel/sched/completion.c:153
> landlock_restrict_sibling_threads+0xe9c/0x11f0 security/landlock/tsync.c:539
> __do_sys_landlock_restrict_self security/landlock/syscalls.c:574 [inline]
> __se_sys_landlock_restrict_self+0x540/0x810 security/landlock/syscalls.c:482
> do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
> do_syscall_64+0x14d/0xf80 arch/x86/entry/syscall_64.c:94
> entry_SYSCALL_64_after_hwframe+0x77/0x7f
> RIP: 0033:0x7f8d7f19bf79
> RSP: 002b:00007f8d8005b028 EFLAGS: 00000246 ORIG_RAX: 00000000000001be
> RAX: ffffffffffffffda RBX: 00007f8d7f416090 RCX: 00007f8d7f19bf79
> RDX: 0000000000000000 RSI: 000000000000000e RDI: 0000000000000003
> RBP: 00007f8d7f2327e0 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
> R13: 00007f8d7f416128 R14: 00007f8d7f416090 R15: 00007ffe0b1928d8
> </TASK>
> INFO: task syz.0.2812:14646 blocked for more than 144 seconds.
> Not tainted syzkaller #0
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> task:syz.0.2812 state:D stack:28832 pid:14646 tgid:14643 ppid:13375 task_flags:0x400140 flags:0x00080006
> Call Trace:
> <TASK>
> context_switch kernel/sched/core.c:5295 [inline]
> __schedule+0x1585/0x5340 kernel/sched/core.c:6907
> __schedule_loop kernel/sched/core.c:6989 [inline]
> schedule+0x164/0x360 kernel/sched/core.c:7004
> schedule_timeout+0xc3/0x2c0 kernel/time/sleep_timeout.c:75
> do_wait_for_common kernel/sched/completion.c:100 [inline]
> __wait_for_common kernel/sched/completion.c:121 [inline]
> wait_for_common kernel/sched/completion.c:132 [inline]
> wait_for_completion+0x2cc/0x5e0 kernel/sched/completion.c:153
> landlock_restrict_sibling_threads+0xe9c/0x11f0 security/landlock/tsync.c:539
> __do_sys_landlock_restrict_self security/landlock/syscalls.c:574 [inline]
> __se_sys_landlock_restrict_self+0x540/0x810 security/landlock/syscalls.c:482
> do_syscall_x64 arch/x86/entry/syscall_64.c:63 [inline]
> do_syscall_64+0x14d/0xf80 arch/x86/entry/syscall_64.c:94
> entry_SYSCALL_64_after_hwframe+0x77/0x7f
> RIP: 0033:0x7f8d7f19bf79
> RSP: 002b:00007f8d8003a028 EFLAGS: 00000246 ORIG_RAX: 00000000000001be
> RAX: ffffffffffffffda RBX: 00007f8d7f416180 RCX: 00007f8d7f19bf79
> RDX: 0000000000000000 RSI: 000000000000000e RDI: 0000000000000003
> RBP: 00007f8d7f2327e0 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
> R13: 00007f8d7f416218 R14: 00007f8d7f416180 R15: 00007ffe0b1928d8
> </TASK>
>
> Showing all locks held in the system:
> 1 lock held by khungtaskd/31:
> #0: ffffffff8e9602e0 (rcu_read_lock){....}-{1:3}, at: rcu_lock_acquire include/linux/rcupdate.h:312 [inline]
> #0: ffffffff8e9602e0 (rcu_read_lock){....}-{1:3}, at: rcu_read_lock include/linux/rcupdate.h:850 [inline]
> #0: ffffffff8e9602e0 (rcu_read_lock){....}-{1:3}, at: debug_show_all_locks+0x2e/0x180 kernel/locking/lockdep.c:6775
> 2 locks held by getty/5581:
> #0: ffff8880328890a0 (&tty->ldisc_sem){++++}-{0:0}, at: tty_ldisc_ref_wait+0x25/0x70 drivers/tty/tty_ldisc.c:243
> #1: ffffc9000332b2f0 (&ldata->atomic_read_lock){+.+.}-{4:4}, at: n_tty_read+0x45c/0x13c0 drivers/tty/n_tty.c:2211
>
> =============================================
>
> NMI backtrace for cpu 0
> CPU: 0 UID: 0 PID: 31 Comm: khungtaskd Not tainted syzkaller #0 PREEMPT(full)
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 02/12/2026
> Call Trace:
> <TASK>
> dump_stack_lvl+0xe8/0x150 lib/dump_stack.c:120
> nmi_cpu_backtrace+0x274/0x2d0 lib/nmi_backtrace.c:113
> nmi_trigger_cpumask_backtrace+0x17a/0x300 lib/nmi_backtrace.c:62
> trigger_all_cpu_backtrace include/linux/nmi.h:161 [inline]
> __sys_info lib/sys_info.c:157 [inline]
> sys_info+0x135/0x170 lib/sys_info.c:165
> check_hung_uninterruptible_tasks kernel/hung_task.c:346 [inline]
> watchdog+0xfd9/0x1030 kernel/hung_task.c:515
> kthread+0x388/0x470 kernel/kthread.c:467
> ret_from_fork+0x51e/0xb90 arch/x86/kernel/process.c:158
> ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
> </TASK>
> Sending NMI from CPU 0 to CPUs 1:
> NMI backtrace for cpu 1
> CPU: 1 UID: 0 PID: 86 Comm: kworker/u8:5 Not tainted syzkaller #0 PREEMPT(full)
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 02/12/2026
> Workqueue: events_unbound nsim_dev_trap_report_work
> RIP: 0010:native_save_fl arch/x86/include/asm/irqflags.h:26 [inline]
> RIP: 0010:arch_local_save_flags arch/x86/include/asm/irqflags.h:109 [inline]
> RIP: 0010:arch_local_irq_save arch/x86/include/asm/irqflags.h:127 [inline]
> RIP: 0010:lock_acquire+0xab/0x2e0 kernel/locking/lockdep.c:5864
> Code: 84 c1 00 00 00 65 8b 05 73 b8 9f 11 85 c0 0f 85 b2 00 00 00 65 48 8b 05 bb 72 9f 11 83 b8 14 0b 00 00 00 0f 85 9d 00 00 00 9c <5b> fa 48 c7 c7 8f a1 02 8e e8 57 40 17 0a 65 ff 05 40 b8 9f 11 45
> RSP: 0018:ffffc9000260f498 EFLAGS: 00000246
> RAX: ffff88801df81e40 RBX: ffffffff818f9166 RCX: 0000000080000002
> RDX: 0000000000000000 RSI: ffffffff8176da62 RDI: 1ffffffff1d2c05c
> RBP: 0000000000000000 R08: 0000000000000000 R09: 0000000000000000
> R10: ffffc9000260f638 R11: ffffffff81b11580 R12: 0000000000000002
> R13: ffffffff8e9602e0 R14: 0000000000000000 R15: 0000000000000000
> FS: 0000000000000000(0000) GS:ffff88812510b000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007fe09b2c1ff8 CR3: 000000000e74c000 CR4: 00000000003526f0
> Call Trace:
> <TASK>
> rcu_lock_acquire include/linux/rcupdate.h:312 [inline]
> rcu_read_lock include/linux/rcupdate.h:850 [inline]
> class_rcu_constructor include/linux/rcupdate.h:1193 [inline]
> unwind_next_frame+0xc2/0x23c0 arch/x86/kernel/unwind_orc.c:495
> arch_stack_walk+0x11b/0x150 arch/x86/kernel/stacktrace.c:25
> stack_trace_save+0xa9/0x100 kernel/stacktrace.c:122
> kasan_save_stack mm/kasan/common.c:57 [inline]
> kasan_save_track+0x3e/0x80 mm/kasan/common.c:78
> unpoison_slab_object mm/kasan/common.c:340 [inline]
> __kasan_slab_alloc+0x6c/0x80 mm/kasan/common.c:366
> kasan_slab_alloc include/linux/kasan.h:253 [inline]
> slab_post_alloc_hook mm/slub.c:4501 [inline]
> slab_alloc_node mm/slub.c:4830 [inline]
> kmem_cache_alloc_node_noprof+0x384/0x690 mm/slub.c:4882
> __alloc_skb+0x1d0/0x7d0 net/core/skbuff.c:702
> alloc_skb include/linux/skbuff.h:1383 [inline]
> nsim_dev_trap_skb_build drivers/net/netdevsim/dev.c:819 [inline]
> nsim_dev_trap_report drivers/net/netdevsim/dev.c:876 [inline]
> nsim_dev_trap_report_work+0x29a/0xb80 drivers/net/netdevsim/dev.c:922
> process_one_work+0x949/0x1650 kernel/workqueue.c:3279
> process_scheduled_works kernel/workqueue.c:3362 [inline]
> worker_thread+0xb46/0x1140 kernel/workqueue.c:3443
> kthread+0x388/0x470 kernel/kthread.c:467
> ret_from_fork+0x51e/0xb90 arch/x86/kernel/process.c:158
> ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
> </TASK>
>
>
> ---
> This report is generated by a bot. It may contain errors.
> See https://goo.gl/tpsmEJ for more information about syzbot.
> syzbot engineers can be reached at syzkaller@googlegroups.com.
>
> syzbot will keep track of this issue. See:
> https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
>
> If the report is already addressed, let syzbot know by replying with:
> #syz fix: exact-commit-title
>
> If you want syzbot to run the reproducer, reply with:
> #syz test: git://repo/address.git branch-or-commit-hash
> If you attach or paste a git patch, syzbot will apply it before testing.
>
> If you want to overwrite report's subsystems, reply with:
> #syz set subsystems: new-subsystem
> (See the list of subsystem names on the web dashboard)
>
> If the report is a duplicate of another one, reply with:
> #syz dup: exact-subject-of-another-report
>
> If you want to undo deduplication, reply with:
> #syz undup
--
Frederic Weisbecker
SUSE Labs
^ permalink raw reply
* Re: [PATCH v2 09/15] ovl: Simplify ovl_lookup_real_one()
From: Amir Goldstein @ 2026-02-23 13:42 UTC (permalink / raw)
To: Chris Mason
Cc: NeilBrown, Christian Brauner, Alexander Viro, David Howells,
Jan Kara, Chuck Lever, Jeff Layton, Miklos Szeredi, John Johansen,
Paul Moore, James Morris, Serge E. Hallyn, Stephen Smalley,
Darrick J. Wong, linux-kernel, netfs, linux-fsdevel, linux-nfs,
linux-unionfs, apparmor, linux-security-module, selinux
In-Reply-To: <20260223132027.4165509-1-clm@meta.com>
On Mon, Feb 23, 2026 at 2:20 PM Chris Mason <clm@meta.com> wrote:
>
> NeilBrown <neilb@ownmail.net> wrote:
> > From: NeilBrown <neil@brown.name>
> >
> > The primary purpose of this patch is to remove the locking from
> > ovl_lookup_real_one() as part of centralising all locking of directories
> > for name operations.
> >
> > The locking here isn't needed. By performing consistency tests after
> > the lookup we can be sure that the result of the lookup was valid at
> > least for a moment, which is all the original code promised.
> >
> > lookup_noperm_unlocked() is used for the lookup and it will take the
> > lock if needed only where it is needed.
> >
> > Also:
> > - don't take a reference to real->d_parent. The parent is
> > only use for a pointer comparison, and no reference is needed for
> > that.
> > - Several "if" statements have a "goto" followed by "else" - the
> > else isn't needed: the following statement can directly follow
> > the "if" as a new statement
> > - Use a consistent pattern of setting "err" before performing a test
> > and possibly going to "fail".
> > - remove the "out" label (now that we don't need to dput(parent) or
> > unlock) and simply return from fail:.
>
> Hi everyone,
>
> Amir asked me to run these through, and this commit was flagged. I recently
> changed the prompts to include some vfs knowledge:
Thanks Chris!
>
> https://github.com/masoncl/review-prompts/blob/main/kernel/subsystem/vfs.md
>
> And I also put in some words to make it prefer reporting problems that might
> not be bugs, but that do violate some rule in the .md files. This might
> be a false positive from that new shift toward blindly trusting the guides,
> but I'll let everyone here decide:
>
> commit d1bfcef048896cdc9e4c57e3249b6ad344d64c02
> Author: NeilBrown <neil@brown.name>
>
> ovl: Simplify ovl_lookup_real_one()
>
> This patch removes locking from ovl_lookup_real_one() and reorganizes the
> control flow. The function now uses lookup_noperm_unlocked() instead of
> lookup_noperm() and performs consistency validation after the lookup rather
> than holding a lock during the operation.
>
> > diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> > index 83f80fdb15674..b448fc9424b64 100644
> > --- a/fs/overlayfs/export.c
> > +++ b/fs/overlayfs/export.c
> > @@ -349,69 +349,64 @@ static struct dentry *ovl_dentry_real_at(struct dentry *dentry, int idx)
> > return NULL;
> > }
> >
> > -/*
> > - * Lookup a child overlay dentry to get a connected overlay dentry whose real
> > - * dentry is @real. If @real is on upper layer, we lookup a child overlay
> > - * dentry with the same name as the real dentry. Otherwise, we need to consult
> > - * index for lookup.
> > +/**
> > + * ovl_lookup_real_one - Lookup a child overlay dentry to get an overlay dentry whose real dentry is given
> > + * @connected: parent overlay dentry
> > + * @real: given child real dentry
> > + * @layer: layer in which @real exists
> > + *
> > + *
> > + * Lookup a child overlay dentry in @connected with the same name as the @real
> > + * dentry. Then check that the parent of the result is the real dentry for
> > + * @connected, and @real is the real dentry for the result.
> > + *
> > + * Returns:
> > + * %-ECHILD if the parent of @real is no longer the real dentry for @connected.
> > + * %-ESTALE if @real is no the real dentry of the found dentry.
> ^^
> There's a typo here: "is no the" should be "is not the".
>
> > + * Otherwise the found dentry is returned.
> > */
> > static struct dentry *ovl_lookup_real_one(struct dentry *connected,
> > struct dentry *real,
> > const struct ovl_layer *layer)
> > {
> > - struct inode *dir = d_inode(connected);
> > - struct dentry *this, *parent = NULL;
> > + struct dentry *this;
> > struct name_snapshot name;
> > int err;
> >
> > /*
> > - * Lookup child overlay dentry by real name. The dir mutex protects us
> > - * from racing with overlay rename. If the overlay dentry that is above
> > - * real has already been moved to a parent that is not under the
> > - * connected overlay dir, we return -ECHILD and restart the lookup of
> > - * connected real path from the top.
> > - */
> > - inode_lock_nested(dir, I_MUTEX_PARENT);
> > - err = -ECHILD;
> > - parent = dget_parent(real);
> > - if (ovl_dentry_real_at(connected, layer->idx) != parent)
> > - goto fail;
> > -
> > - /*
> > - * We also need to take a snapshot of real dentry name to protect us
> > + * We need to take a snapshot of real dentry name to protect us
> > * from racing with underlying layer rename. In this case, we don't
> > * care about returning ESTALE, only from dereferencing a free name
> > * pointer because we hold no lock on the real dentry.
> > */
> > take_dentry_name_snapshot(&name, real);
> > - /*
> > - * No idmap handling here: it's an internal lookup.
> > - */
> > - this = lookup_noperm(&name.name, connected);
> > + this = lookup_noperm_unlocked(&name.name, connected);
> > release_dentry_name_snapshot(&name);
> > +
> > + err = -ECHILD;
> > + if (ovl_dentry_real_at(connected, layer->idx) != real->d_parent)
> > + goto fail;
> > +
>
> A subsystem pattern flags this as potentially concerning: the VFS subsystem
> guide indicates that TOCTOU validation checking d_parent should also check
> d_unhashed() because a dentry can be unhashed (removed from dcache by
> concurrent operations) while its d_parent pointer remains unchanged.
>
> The old code held inode_lock_nested(dir, I_MUTEX_PARENT) during the entire
> lookup and validation, which serialized with operations that could unhash the
> dentry. The new lockless pattern creates a window where:
>
> 1. lookup_noperm_unlocked() completes and returns dentry `this`
> 2. A concurrent unlink/remove operation unhashes `this`
> 3. The d_parent validation passes (parent pointer unchanged)
> 4. The function returns an unhashed dentry
>
> Other overlayfs code uses the complete pattern. For example, in
> ovl_lock_rename_workdir() at fs/overlayfs/util.c:
>
> if (work->d_parent != workdir || d_unhashed(work)) {
> ...
> }
>
> Could this validation miss a race where the dentry is unhashed but d_parent
> still matches? The subsequent checks (!this->d_inode for -ENOENT,
> wrong real dentry for -ESTALE) might catch some cases, but is that
> sufficient for all the scenarios where an unhashed dentry could cause
> problems?
>
It's a very good comment and very important rule to check, since
we have at least 3 fix commits on breaking this rule, but as this
code is utterly confusing to most human I do not blame LLM for getting
confused here.
The lock not taken on 'dir' which is the overlayfs inode and the checked
'real' dentry is on the underlying fs.
Therefore, the check of real->d_parent was not protected in old code as
well as in new code - it is a mere best effort sanity check, so I think
there is no added risk here.
Neil, do you agree?
Thanks,
Amir.
^ permalink raw reply
* Re: [PATCH v2 01/15] VFS: note error returns is documentation for various lookup functions
From: Chris Mason @ 2026-02-23 13:52 UTC (permalink / raw)
To: NeilBrown
Cc: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Jeff Layton, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley, Darrick J. Wong, linux-kernel, netfs,
linux-fsdevel, linux-nfs, linux-unionfs, apparmor,
linux-security-module, selinux
In-Reply-To: <20260223011210.3853517-2-neilb@ownmail.net>
NeilBrown <neilb@ownmail.net> wrote:
> From: NeilBrown <neil@brown.name>
>
> Darrick recently noted that try_lookup_noperm() is documented as
> "Look up a dentry by name in the dcache, returning NULL if it does not
> currently exist." but it can in fact return an error.
>
> So update the documentation for that and related function.
>
Hi everyone,
I don't normally forward the typos, but since this is a documentation-y patch:
commit 0254b9b974f23889898562aa94f6428bf30eb6b5
Author: NeilBrown <neil@brown.name>
VFS: note error returns is documentation for various lookup functions
^^^^^ in?
Darrick recently noted that try_lookup_noperm() is documented as
"Look up a dentry by name in the dcache, returning NULL if it does not
currently exist." but it can in fact return an error. So update the
documentation for that and related function.
Link: https://lore.kernel.org/all/20260218234917.GA6490@frogsfrogsfrogs/
> diff --git a/fs/namei.c b/fs/namei.c
> index 58f715f7657e2..e4ac07a4090e7 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3124,7 +3124,8 @@ static int lookup_one_common(struct mnt_idmap *idmap,
> * @base: base directory to lookup from
> *
> * Look up a dentry by name in the dcache, returning NULL if it does not
> - * currently exist. The function does not try to create a dentry and if one
> + * currently exist or an error is there is a problem with the name.
^^
There's a grammar error: "is there is" should be "if there is".
> + * The function does not try to create a dentry and if one
> * is found it doesn't try to revalidate it.
> *
> * Note that this routine is purely a helper for filesystem usage and should
> @@ -3132,6 +3133,11 @@ static int lookup_one_common(struct mnt_idmap *idmap,
> *
> * No locks need be held - only a counted reference to @base is needed.
> *
> + * Returns:
> + * - ref-counted dentry on success, or
> + * - %NULL if name could not be found, or
> + * - ERR_PTR(-EACCES) if name is dot or dotdot or contains a slash or nul, or
> + * - ERR_PTR() if fs provide ->d_hash, and this returned an error.
> */
> struct dentry *try_lookup_noperm(struct qstr *name, struct dentry *base)
> {
[ ... ]
> @@ -3208,6 +3214,11 @@ EXPORT_SYMBOL(lookup_one);
> *
> * Unlike lookup_one, it should be called without the parent
> * i_rwsem held, and will take the i_rwsem itself if necessary.
> + *
> + * Returns:= A dentry, possibly negative, or
^^^
There's a typo: "Returns:=" should be "Returns:".
> + * - same errors as try_lookup_noperm() or
> + * - ERR_PTR(-ENOENT) if parent has been removed, or
> + * - ERR_PTR(-EACCES) if parent directory is not searchable.
> */
> struct dentry *lookup_one_unlocked(struct mnt_idmap *idmap, struct qstr *name,
> struct dentry *base)
[ ... ]
> @@ -3244,6 +3255,10 @@ EXPORT_SYMBOL(lookup_one_unlocked);
> * It should be called without the parent i_rwsem held, and will take
> * the i_rwsem itself if necessary. If a fatal signal is pending or
> * delivered, it will return %-EINTR if the lock is needed.
> + *
> + * Returns: A dentry, possibly negative, or
> + * - same errors as lookup_one_unlocked() or
> + * - ERR_PTR(-EINTR) is a fatal signal is pending.
^^
There's a grammar error: "is a fatal signal is pending" should be "if a
fatal signal is pending".
> + */
> struct dentry *lookup_one_positive_killable(struct mnt_idmap *idmap,
> struct qstr *name,
^ permalink raw reply
* [PATCH v5] ima_fs: Avoid creating measurement lists for unsupported hash algos
From: Dmitry Safonov via B4 Relay @ 2026-02-23 14:56 UTC (permalink / raw)
To: Mimi Zohar, Roberto Sassu, Dmitry Kasatkin, Eric Snowberg,
Paul Moore, James Morris, Serge E. Hallyn, Silvia Sisinni,
Enrico Bravi
Cc: Jonathan McDowell, linux-integrity, linux-security-module,
linux-kernel, stable, Dmitry Safonov, Dmitry Safonov
From: Dmitry Safonov <dima@arista.com>
ima_tpm_chip->allocated_banks[i].crypto_id is initialized to
HASH_ALGO__LAST if the TPM algorithm is not supported. However there
are places relying on the algorithm to be valid because it is accessed
by hash_algo_name[].
On 6.12.40 I observe the following read out-of-bounds in hash_algo_name:
==================================================================
BUG: KASAN: global-out-of-bounds in create_securityfs_measurement_lists+0x396/0x440
Read of size 8 at addr ffffffff83e18138 by task swapper/0/1
CPU: 4 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.12.40 #3
Call Trace:
<TASK>
dump_stack_lvl+0x61/0x90
print_report+0xc4/0x580
? kasan_addr_to_slab+0x26/0x80
? create_securityfs_measurement_lists+0x396/0x440
kasan_report+0xc2/0x100
? create_securityfs_measurement_lists+0x396/0x440
create_securityfs_measurement_lists+0x396/0x440
ima_fs_init+0xa3/0x300
ima_init+0x7d/0xd0
init_ima+0x28/0x100
do_one_initcall+0xa6/0x3e0
kernel_init_freeable+0x455/0x740
kernel_init+0x24/0x1d0
ret_from_fork+0x38/0x80
ret_from_fork_asm+0x11/0x20
</TASK>
The buggy address belongs to the variable:
hash_algo_name+0xb8/0x420
Memory state around the buggy address:
ffffffff83e18000: 00 01 f9 f9 f9 f9 f9 f9 00 01 f9 f9 f9 f9 f9 f9
ffffffff83e18080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>ffffffff83e18100: 00 00 00 00 00 00 00 f9 f9 f9 f9 f9 00 05 f9 f9
^
ffffffff83e18180: f9 f9 f9 f9 00 00 00 00 00 00 00 04 f9 f9 f9 f9
ffffffff83e18200: 00 00 00 00 00 00 00 00 04 f9 f9 f9 f9 f9 f9 f9
==================================================================
Seems like the TPM chip supports sha3_256, which isn't yet in
tpm_algorithms:
tpm tpm0: TPM with unsupported bank algorithm 0x0027
Thus solve the problem by creating a file name with "_tpm_alg_<ID>"
postfix if the crypto algorithm isn't initialized.
This is how it looks on the test machine (patch ported to v6.12 release):
# ls -1 /sys/kernel/security/ima/
ascii_runtime_measurements
ascii_runtime_measurements_tpm_alg_27
ascii_runtime_measurements_sha1
ascii_runtime_measurements_sha256
binary_runtime_measurements
binary_runtime_measurements_tpm_alg_27
binary_runtime_measurements_sha1
binary_runtime_measurements_sha256
policy
runtime_measurements_count
violations
Fixes: 9fa8e7625008 ("ima: add crypto agility support for template-hash algorithm")
Signed-off-by: Dmitry Safonov <dima@arista.com>
Cc: Enrico Bravi <enrico.bravi@polito.it>
Cc: Silvia Sisinni <silvia.sisinni@polito.it>
Cc: Roberto Sassu <roberto.sassu@huawei.com>
Cc: Mimi Zohar <zohar@linux.ibm.com>
---
Changes in v5:
- Use lower-case for sysfs file name (as suggested-by Jonathan and Roberto)
- Don't use email quotes for patch description (Roberto)
- Re-word the patch description (suggested-by Roberto)
- Link to v4: https://lore.kernel.org/r/20260127-ima-oob-v4-1-bf0cd7f9b4d4@arista.com
Changes in v4:
- Use ima_tpm_chip->allocated_banks[algo_idx].digest_size instead of hash_digest_size[algo]
(Roberto Sassu)
- Link to v3: https://lore.kernel.org/r/20260127-ima-oob-v3-1-1dd09f4c2a6a@arista.com
Testing note: I test it on v6.12.40 kernel backport, which slightly differs as
lookup_template_data_hash_algo() was yet present.
Changes in v3:
- Now fix the spelling *for real* (sorry, messed it up in v2)
- Link to v2: https://lore.kernel.org/r/20260127-ima-oob-v2-1-f38a18c850cf@arista.com
Changes in v2:
- Instead of skipping unknown algorithms, add files under their TPM_ALG_ID (Roberto Sassu)
- Fix spelling (Roberto Sassu)
- Copy @stable on the fix
- Link to v1: https://lore.kernel.org/r/20260127-ima-oob-v1-1-2d42f3418e57@arista.com
---
security/integrity/ima/ima_fs.c | 34 ++++++++++++++++++----------------
1 file changed, 18 insertions(+), 16 deletions(-)
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 012a58959ff0..3d9996ed486d 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -132,16 +132,12 @@ int ima_measurements_show(struct seq_file *m, void *v)
char *template_name;
u32 pcr, namelen, template_data_len; /* temporary fields */
bool is_ima_template = false;
- enum hash_algo algo;
int i, algo_idx;
algo_idx = ima_sha1_idx;
- algo = HASH_ALGO_SHA1;
- if (m->file != NULL) {
+ if (m->file != NULL)
algo_idx = (unsigned long)file_inode(m->file)->i_private;
- algo = ima_algo_array[algo_idx].algo;
- }
/* get entry */
e = qe->entry;
@@ -160,7 +156,8 @@ int ima_measurements_show(struct seq_file *m, void *v)
ima_putc(m, &pcr, sizeof(e->pcr));
/* 2nd: template digest */
- ima_putc(m, e->digests[algo_idx].digest, hash_digest_size[algo]);
+ ima_putc(m, e->digests[algo_idx].digest,
+ ima_tpm_chip->allocated_banks[algo_idx].digest_size);
/* 3rd: template name size */
namelen = !ima_canonical_fmt ? strlen(template_name) :
@@ -229,16 +226,12 @@ static int ima_ascii_measurements_show(struct seq_file *m, void *v)
struct ima_queue_entry *qe = v;
struct ima_template_entry *e;
char *template_name;
- enum hash_algo algo;
int i, algo_idx;
algo_idx = ima_sha1_idx;
- algo = HASH_ALGO_SHA1;
- if (m->file != NULL) {
+ if (m->file != NULL)
algo_idx = (unsigned long)file_inode(m->file)->i_private;
- algo = ima_algo_array[algo_idx].algo;
- }
/* get entry */
e = qe->entry;
@@ -252,7 +245,8 @@ static int ima_ascii_measurements_show(struct seq_file *m, void *v)
seq_printf(m, "%2d ", e->pcr);
/* 2nd: template hash */
- ima_print_digest(m, e->digests[algo_idx].digest, hash_digest_size[algo]);
+ ima_print_digest(m, e->digests[algo_idx].digest,
+ ima_tpm_chip->allocated_banks[algo_idx].digest_size);
/* 3th: template name */
seq_printf(m, " %s", template_name);
@@ -404,16 +398,24 @@ static int __init create_securityfs_measurement_lists(void)
char file_name[NAME_MAX + 1];
struct dentry *dentry;
- sprintf(file_name, "ascii_runtime_measurements_%s",
- hash_algo_name[algo]);
+ if (algo == HASH_ALGO__LAST)
+ sprintf(file_name, "ascii_runtime_measurements_tpm_alg_%x",
+ ima_tpm_chip->allocated_banks[i].alg_id);
+ else
+ sprintf(file_name, "ascii_runtime_measurements_%s",
+ hash_algo_name[algo]);
dentry = securityfs_create_file(file_name, S_IRUSR | S_IRGRP,
ima_dir, (void *)(uintptr_t)i,
&ima_ascii_measurements_ops);
if (IS_ERR(dentry))
return PTR_ERR(dentry);
- sprintf(file_name, "binary_runtime_measurements_%s",
- hash_algo_name[algo]);
+ if (algo == HASH_ALGO__LAST)
+ sprintf(file_name, "binary_runtime_measurements_tpm_alg_%x",
+ ima_tpm_chip->allocated_banks[i].alg_id);
+ else
+ sprintf(file_name, "binary_runtime_measurements_%s",
+ hash_algo_name[algo]);
dentry = securityfs_create_file(file_name, S_IRUSR | S_IRGRP,
ima_dir, (void *)(uintptr_t)i,
&ima_measurements_ops);
---
base-commit: 6de23f81a5e08be8fbf5e8d7e9febc72a5b5f27f
change-id: 20260127-ima-oob-9fa83a634d7b
Best regards,
--
Dmitry Safonov <dima@arista.com>
^ permalink raw reply related
* Re: [PATCH v4] ima_fs: Avoid creating measurement lists for unsupported hash algos
From: Dmitry Safonov @ 2026-02-23 14:59 UTC (permalink / raw)
To: Roberto Sassu
Cc: Mimi Zohar, Roberto Sassu, Dmitry Kasatkin, Eric Snowberg,
Paul Moore, James Morris, Serge E. Hallyn, Silvia Sisinni,
Enrico Bravi, linux-integrity, linux-security-module,
linux-kernel, stable, Dmitry Safonov
In-Reply-To: <89f51356ba5e630a8c305e5f65abd2f3ace37a48.camel@huaweicloud.com>
Hi Roberto,
On Thu, Feb 19, 2026 at 8:55 AM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> On Tue, 2026-01-27 at 16:20 +0100, Roberto Sassu wrote:
> > On Tue, 2026-01-27 at 15:03 +0000, Dmitry Safonov via B4 Relay wrote:
> > > From: Dmitry Safonov <dima@arista.com>
> > >
> > > ima_init_crypto() skips initializing ima_algo_array[i] if the algorithm
> > > from ima_tpm_chip->allocated_banks[i].crypto_id is not supported.
> > > It seems avoid adding the unsupported algorithm to ima_algo_array will
> > > break all the logic that relies on indexing by NR_BANKS(ima_tpm_chip).
> >
> > The patch looks good, although I didn't try yet myself.
> >
> > I would make the commit message slightly better, with a more fluid
> > explanation.
> >
> > ima_tpm_chip->allocated_banks[i].crypto_id is initialized to
> > HASH_ALGO__LAST if the TPM algorithm is not supported. However there
> > are places relying on the algorithm to be valid because it is accessed
> > by hash_algo_name[].
> >
> > Thus solve the problem by creating a file name that does not depend on
> > the crypto algorithm to be initialized, ...
> >
> > Also print the template entry digest as populated by IMA.
> >
> > Something along these lines.
> >
> > Also, I have a preference for lower case instead of capital case for
> > the file name, given the other names.
>
> Hi Dmitry
>
> do you have time to make these small changes, so that we queue the
> patch for the next kernel?
I've just sent v5. Sorry for the delay — I got busy with the local release bugs.
Thanks,
Dmitry
^ permalink raw reply
* Re: [PATCH v15 3/9] rust: Add missing SAFETY documentation for `ARef` example
From: Andreas Hindborg @ 2026-02-23 14:59 UTC (permalink / raw)
To: Alice Ryhl
Cc: Miguel Ojeda, Gary Guo, Björn Roy Baron, Benno Lossin,
Trevor Gross, Danilo Krummrich, Greg Kroah-Hartman, Dave Ertman,
Ira Weiny, Leon Romanovsky, Paul Moore, Serge Hallyn,
Rafael J. Wysocki, David Airlie, Simona Vetter, Alexander Viro,
Christian Brauner, Jan Kara, Igor Korotin, Daniel Almeida,
Lorenzo Stoakes, Liam R. Howlett, Viresh Kumar, Nishanth Menon,
Stephen Boyd, Bjorn Helgaas, Krzysztof Wilczyński,
Boqun Feng, linux-kernel, rust-for-linux, linux-block,
linux-security-module, dri-devel, linux-fsdevel, linux-mm,
linux-pm, linux-pci, Oliver Mangold
In-Reply-To: <CAH5fLggNjCZ3AvHnhO8O0cmd33B3zMbfq+hhNvonznTsLLtgYw@mail.gmail.com>
Alice Ryhl <aliceryhl@google.com> writes:
> On Fri, Feb 20, 2026 at 10:52 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> From: Oliver Mangold <oliver.mangold@pm.me>
>>
>> SAFETY comment in rustdoc example was just 'TODO'. Fixed.
>>
>> Signed-off-by: Oliver Mangold <oliver.mangold@pm.me>
>> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
>> Co-developed-by: Andreas Hindborg <a.hindborg@kernel.org>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>> ---
>> rust/kernel/sync/aref.rs | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/rust/kernel/sync/aref.rs b/rust/kernel/sync/aref.rs
>> index 61caddfd89619..efe16a7fdfa5d 100644
>> --- a/rust/kernel/sync/aref.rs
>> +++ b/rust/kernel/sync/aref.rs
>> @@ -129,12 +129,14 @@ pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {
>> /// # Examples
>> ///
>> /// ```
>> - /// use core::ptr::NonNull;
>> - /// use kernel::sync::aref::{ARef, RefCounted};
>> + /// # use core::ptr::NonNull;
>> + /// # use kernel::sync::aref::{ARef, RefCounted};
>> ///
>
> Either keep the imports visible or delete this empty line. And either
> way, it doesn't really fit in this commit.
I'll drop this for this commit.
Best regards,
Andreas Hindborg
^ permalink raw reply
* Re: [PATCH v15 9/9] rust: page: add `from_raw()`
From: Andreas Hindborg @ 2026-02-23 14:59 UTC (permalink / raw)
To: Alice Ryhl
Cc: Miguel Ojeda, Gary Guo, Björn Roy Baron, Benno Lossin,
Trevor Gross, Danilo Krummrich, Greg Kroah-Hartman, Dave Ertman,
Ira Weiny, Leon Romanovsky, Paul Moore, Serge Hallyn,
Rafael J. Wysocki, David Airlie, Simona Vetter, Alexander Viro,
Christian Brauner, Jan Kara, Igor Korotin, Daniel Almeida,
Lorenzo Stoakes, Liam R. Howlett, Viresh Kumar, Nishanth Menon,
Stephen Boyd, Bjorn Helgaas, Krzysztof Wilczyński,
Boqun Feng, linux-kernel, rust-for-linux, linux-block,
linux-security-module, dri-devel, linux-fsdevel, linux-mm,
linux-pm, linux-pci
In-Reply-To: <CAH5fLggNQD+TbA7rXVB5w+O+qHcJcYC4u0b3W+mHR2DZiUe4eQ@mail.gmail.com>
Alice Ryhl <aliceryhl@google.com> writes:
> On Fri, Feb 20, 2026 at 10:52 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> Add a method to `Page` that allows construction of an instance from `struct
>> page` pointer.
>>
>> Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com>
>> ---
>> rust/kernel/page.rs | 11 +++++++++++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/rust/kernel/page.rs b/rust/kernel/page.rs
>> index 4591b7b01c3d2..803f3e3d76b22 100644
>> --- a/rust/kernel/page.rs
>> +++ b/rust/kernel/page.rs
>> @@ -191,6 +191,17 @@ pub fn nid(&self) -> i32 {
>> unsafe { bindings::page_to_nid(self.as_ptr()) }
>> }
>>
>> + /// Create a `&Page` from a raw `struct page` pointer
>> + ///
>> + /// # Safety
>> + ///
>> + /// `ptr` must be valid for use as a reference for the duration of `'a`.
>> + pub unsafe fn from_raw<'a>(ptr: *const bindings::page) -> &'a Self {
>> + // SAFETY: By function safety requirements, ptr is not null and is
>> + // valid for use as a reference.
>> + unsafe { &*Opaque::cast_from(ptr).cast::<Self>() }
>
> If you're going to do a pointer cast, then keep it simple and just do
> &*ptr.cast().
Ok.
Best regards,
Andreas Hindborg
^ permalink raw reply
* Re: [PATCH v15 1/9] rust: types: Add Ownable/Owned types
From: Andreas Hindborg @ 2026-02-23 14:59 UTC (permalink / raw)
To: Alice Ryhl
Cc: Miguel Ojeda, Gary Guo, Björn Roy Baron, Benno Lossin,
Trevor Gross, Danilo Krummrich, Greg Kroah-Hartman, Dave Ertman,
Ira Weiny, Leon Romanovsky, Paul Moore, Serge Hallyn,
Rafael J. Wysocki, David Airlie, Simona Vetter, Alexander Viro,
Christian Brauner, Jan Kara, Igor Korotin, Daniel Almeida,
Lorenzo Stoakes, Liam R. Howlett, Viresh Kumar, Nishanth Menon,
Stephen Boyd, Bjorn Helgaas, Krzysztof Wilczyński,
Boqun Feng, linux-kernel, rust-for-linux, linux-block,
linux-security-module, dri-devel, linux-fsdevel, linux-mm,
linux-pm, linux-pci, Asahi Lina, Oliver Mangold
In-Reply-To: <aZg44EmMWKK-z5KP@google.com>
Alice Ryhl <aliceryhl@google.com> writes:
> On Fri, Feb 20, 2026 at 10:51:10AM +0100, Andreas Hindborg wrote:
>> From: Asahi Lina <lina+kernel@asahilina.net>
>>
>> By analogy to `AlwaysRefCounted` and `ARef`, an `Ownable` type is a
>> (typically C FFI) type that *may* be owned by Rust, but need not be. Unlike
>> `AlwaysRefCounted`, this mechanism expects the reference to be unique
>> within Rust, and does not allow cloning.
>>
>> Conceptually, this is similar to a `KBox<T>`, except that it delegates
>> resource management to the `T` instead of using a generic allocator.
>>
>> [ om:
>> - Split code into separate file and `pub use` it from types.rs.
>> - Make from_raw() and into_raw() public.
>> - Remove OwnableMut, and make DerefMut dependent on Unpin instead.
>> - Usage example/doctest for Ownable/Owned.
>> - Fixes to documentation and commit message.
>> ]
>>
>> Link: https://lore.kernel.org/all/20250202-rust-page-v1-1-e3170d7fe55e@asahilina.net/
>> Signed-off-by: Asahi Lina <lina+kernel@asahilina.net>
>> Co-developed-by: Oliver Mangold <oliver.mangold@pm.me>
>> Signed-off-by: Oliver Mangold <oliver.mangold@pm.me>
>> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
>> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
>> [ Andreas: Updated documentation, examples, and formatting ]
>> Reviewed-by: Gary Guo <gary@garyguo.net>
>> Co-developed-by: Andreas Hindborg <a.hindborg@kernel.org>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>
>> +/// let result = NonNull::new(KBox::into_raw(result))
>> +/// .expect("Raw pointer to newly allocation KBox is null, this should never happen.");
>
> KBox should probably have an into_raw_nonnull().
I can add that.
>
>> +/// let foo = Foo::new().expect("Failed to allocate a Foo. This shouldn't happen");
>> +/// assert!(*FOO_ALLOC_COUNT.lock() == 1);
>
> Use ? here.
Ok.
>
>> +/// }
>> +/// // `foo` is out of scope now, so we expect no live allocations.
>> +/// assert!(*FOO_ALLOC_COUNT.lock() == 0);
>> +/// ```
>> +pub unsafe trait Ownable {
>> + /// Releases the object.
>> + ///
>> + /// # Safety
>> + ///
>> + /// Callers must ensure that:
>> + /// - `this` points to a valid `Self`.
>> + /// - `*this` is no longer used after this call.
>> + unsafe fn release(this: NonNull<Self>);
>
> Honestly, not using it after this call may be too strong. I can imagine
> wanting a value where I have both an ARef<_> and Owned<_> reference to
> something similar to the existing Arc<_>/ListArc<_> pattern, and in that
> case the value may in fact be accessed after this call if you still have
> an ARef<_>.
I do not understand your use case.
You are not supposed to have both an `ARef` and an `Owned` at the same
time. The `Owned` is to `ARef` what `UniqueArc` is to `Arc`. It is
supposed to be unique and no `ARef` can be live while the `Owned` is
live.
A `ListArc` is "at most one per list link" and it takes a refcount on
the object by owning an `Arc`. As far as I recall, it does not provide
mutable access to anything but the list link. To me, that is a very
different situation.
>
> If you modify Owned<_> invariants and Owned::from_raw() safety
> requirements along the lines of what I say below, then this could just
> say that the caller must have permission to call this function. The
> concrete implementer can specify what that means more directly, but here
> all it means is that a prior call to Owned::from_raw() promised to give
> you permission to call it.
I don't think we need the "permission" wording. How about this:
/// A mutable reference to an owned `T`.
///
/// The [`Ownable`] is automatically freed or released when an instance of [`Owned`] is
/// dropped.
///
/// # Invariants
///
/// - Until `T::release` is called, this `Owned<T>` exclusively owns the underlying `T`.
/// - The `T` value is pinned.
pub struct Owned<T: Ownable> {...}
impl<T: Ownable> Owned<T> {
/// Creates a new instance of [`Owned`].
///
/// This function takes over ownership of the underlying object.
///
/// # Safety
///
/// Callers must ensure that:
/// - `ptr` points to a valid instance of `T`.
/// - Until `T::release` is called, the returned `Owned<T>` exclusively owns the underlying `T`.
pub unsafe fn from_raw(ptr: NonNull<T>) -> Self {...}
}
pub trait Ownable {
/// Tear down this `Ownable`.
///
/// Implementers of `Ownable` can use this function to clean up the use of `Self`. This can
/// include freeing the underlying object.
///
/// # Safety
///
/// Callers must ensure that the caller has exclusive ownership of `T`, and this ownership can
/// be transferred to the `release` method.
unsafe fn release(&mut self);
}
Note `Ownable` not being an unsafe trait.
>
>> +/// A mutable reference to an owned `T`.
>> +///
>> +/// The [`Ownable`] is automatically freed or released when an instance of [`Owned`] is
>> +/// dropped.
>> +///
>> +/// # Invariants
>> +///
>> +/// - The [`Owned<T>`] has exclusive access to the instance of `T`.
>> +/// - The instance of `T` will stay alive at least as long as the [`Owned<T>`] is alive.
>> +pub struct Owned<T: Ownable> {
>> + ptr: NonNull<T>,
>> +}
>
> I think some more direct and less fuzzy invariants would be:
>
> - This `Owned<T>` holds permissions to call `T::release()` on the value once.
> - Until `T::release()` is called, this `Owned<T>` may perform mutable access on the `T`.
I do not like the wording for mutable access. Formulating safety
requirements for `from_raw` and safety comments for that function
becomes convoluted like this. I'd rather formulate the
access capability in terms of ownership;
- Until `T::release()` is called, this `Owned<T>` exclusively owns the
underlying `T`.
How is that?
> - The `T` value is pinned.
I am unsure about the pinning terminology. If we say that `T` is pinned,
does this mean that it will never move, even if `T: Unpin`? Or is it
implied that `T` may move if it is `Unpin`?
>
>> + /// Get a pinned mutable reference to the data owned by this `Owned<T>`.
>> + pub fn as_pin_mut(&mut self) -> Pin<&mut T> {
>> + // SAFETY: The type invariants guarantee that the object is valid, and that we can safely
>> + // return a mutable reference to it.
>> + let unpinned = unsafe { self.ptr.as_mut() };
>> +
>> + // SAFETY: We never hand out unpinned mutable references to the data in
>> + // `Self`, unless the contained type is `Unpin`.
>> + unsafe { Pin::new_unchecked(unpinned) }
>
> I'd prefer if "pinned" was a type invariant, rather than make an
> argument about what kind of APIs exist.
Ok.
>
>> +impl<T: Ownable + Unpin> DerefMut for Owned<T> {
>> + fn deref_mut(&mut self) -> &mut Self::Target {
>> + // SAFETY: The type invariants guarantee that the object is valid, and that we can safely
>> + // return a mutable reference to it.
>> + unsafe { self.ptr.as_mut() }
>
> Surely this safety comment should say something about pinning.
Yes.
Best regards,
Andreas Hindborg
^ permalink raw reply
* Re: [PATCH v15 9/9] rust: page: add `from_raw()`
From: Andreas Hindborg @ 2026-02-23 15:00 UTC (permalink / raw)
To: Miguel Ojeda, Tamir Duberstein, Benno Lossin
Cc: Miguel Ojeda, Gary Guo, Björn Roy Baron, Alice Ryhl,
Trevor Gross, Danilo Krummrich, Greg Kroah-Hartman, Dave Ertman,
Ira Weiny, Leon Romanovsky, Paul Moore, Serge Hallyn,
Rafael J. Wysocki, David Airlie, Simona Vetter, Alexander Viro,
Christian Brauner, Jan Kara, Igor Korotin, Daniel Almeida,
Lorenzo Stoakes, Liam R. Howlett, Viresh Kumar, Nishanth Menon,
Stephen Boyd, Bjorn Helgaas, Krzysztof Wilczyński,
Boqun Feng, linux-kernel, rust-for-linux, linux-block,
linux-security-module, dri-devel, linux-fsdevel, linux-mm,
linux-pm, linux-pci
In-Reply-To: <CANiq72myc+tCEHm0WtZspZHWwsSzvesxsmUvk31=GCdUN_zVNA@mail.gmail.com>
Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> writes:
> On Fri, Feb 20, 2026 at 10:52 AM Andreas Hindborg <a.hindborg@kernel.org> wrote:
>>
>> + /// Create a `&Page` from a raw `struct page` pointer
>
> Please end sentences with a period.
Ok.
>
>> + // SAFETY: By function safety requirements, ptr is not null and is
>
> Please use Markdown in comments: `ptr`.
Ok.
>
>> + /// `ptr` must be valid for use as a reference for the duration of `'a`.
>
> Since we will likely try to starting introducing at least a subset of
> the Safety Standard soon, we should try to use standard terms.
>
> So I think this "valid for use as a reference" is not an established
> one, no? Isn't "convertible to a shared reference" the official term?
>
> https://doc.rust-lang.org/std/ptr/index.html#pointer-to-reference-conversion
>
> In fact, I see `as_ref_unchecked()` and `as_mut_unchecked()` just got
> stabilized for 1.95.0, so we should probably starting using those were
> applicable as we bump the minimum, but we should probably use already
> a similar wording as the standard library for the safety section and
> the comment:
>
> "`ptr` must be [convertible to a reference](...)."
I'll change the wording to the "convertible" one.
Best regards,
Andreas Hindborg
^ permalink raw reply
* Re: [syzbot] [kernel?] INFO: task hung in restrict_one_thread_callback
From: Günther Noack @ 2026-02-23 15:15 UTC (permalink / raw)
To: Frederic Weisbecker
Cc: syzbot, Mickaël Salaün, Paul Moore, James Morris,
Serge E. Hallyn, linux-security-module, anna-maria, linux-kernel,
syzkaller-bugs, tglx
In-Reply-To: <aZxYv7MxxExj9fjM@localhost.localdomain>
On Mon, Feb 23, 2026 at 02:40:15PM +0100, Frederic Weisbecker wrote:
> Le Fri, Feb 20, 2026 at 03:11:21AM -0800, syzbot a écrit :
> > Call Trace:
> > <TASK>
> > context_switch kernel/sched/core.c:5295 [inline]
> > __schedule+0x1585/0x5340 kernel/sched/core.c:6907
> > __schedule_loop kernel/sched/core.c:6989 [inline]
> > schedule+0x164/0x360 kernel/sched/core.c:7004
> > schedule_timeout+0xc3/0x2c0 kernel/time/sleep_timeout.c:75
> > do_wait_for_common kernel/sched/completion.c:100 [inline]
> > __wait_for_common kernel/sched/completion.c:121 [inline]
> > wait_for_common kernel/sched/completion.c:132 [inline]
> > wait_for_completion+0x2cc/0x5e0 kernel/sched/completion.c:153
> > restrict_one_thread security/landlock/tsync.c:128 [inline]
> > restrict_one_thread_callback+0x320/0x570 security/landlock/tsync.c:162
>
> Seems to be related to landlock security module.
> Cc'ing maintainers for awareness.
Thank you! That is correct. We are already discussing it in
https://lore.kernel.org/all/00A9E53EDC82309F+7b1dfc69-95f8-4ffc-a67c-967de0e2dfee@uniontech.com/
—Günther
^ permalink raw reply
* Re: [syzbot] [kernel?] INFO: task hung in restrict_one_thread_callback
From: Günther Noack @ 2026-02-23 15:16 UTC (permalink / raw)
To: Ding Yihan
Cc: Günther Noack, syzbot, Mickaël Salaün,
linux-security-module, Jann Horn, Paul Moore
In-Reply-To: <32095877A7CB47CB+bb9e1be8-59c2-46d9-b1ef-f22d2d8c386e@uniontech.com>
Hello!
On Mon, Feb 23, 2026 at 07:29:56PM +0800, Ding Yihan wrote:
> Thank you for the detailed analysis and the clear breakdown.
> Apologies for the delayed response. I spent the last couple of days
> thoroughly reading through the previous mailing list discussions. I
> was trying hard to see if there was any viable pure lockless design
> that could solve this concurrency issue while preserving the original
> architecture.
>
> However, after looking at the complexities you outlined, I completely
> agree with your conclusion: serializing the TSYNC operations is indeed
> the most robust and reasonable path forward to prevent the deadlock.
>
> Regarding the lock choice, since 'cred_guard_mutex' is explicitly
> marked as deprecated for new code in the kernel,maybe we can use its
> modern replacement: 'exec_update_lock' (using down_write_trylock /
> up_write on current->signal). This aligns with the current subsystem
> standards and was also briefly touched upon by Jann in the older
> discussions.
>
> I fully understand the requirement for the two-part patch series:
> 1. Cleaning up the cancellation logic and comments.
> 2. Introducing the serialization lock for TSYNC.
>
> I will take some time to draft and test this patch series properly.
> I also plan to discuss this with my kernel colleagues here at
> UnionTech to see if they have any additional suggestions on the
> implementation details before I submit it.
>
> I will send out the v1 patch series to the list as soon as it is
> ready. Thanks again for your guidance and the great discussion!
Thank you, Ding, this is much appreciated!
I agree, the `exec_update_lock` might be the better solution;
I also need to familiarize myself more with it to double-check.
—Günther
^ permalink raw reply
* Re: [PATCH v6] lsm: Add LSM hook security_unix_find
From: Mickaël Salaün @ 2026-02-23 16:09 UTC (permalink / raw)
To: Justin Suess
Cc: Günther Noack, brauner, demiobenour, fahimitahera, hi, horms,
ivanov.mikhail1, jannh, jmorris, john.johansen,
konstantin.meskhidze, linux-security-module, m, matthieu, netdev,
paul, samasth.norway.ananda, serge, viro
In-Reply-To: <aZmxpoy1oxSl5yGq@suesslenovo>
On Sat, Feb 21, 2026 at 08:22:46AM -0500, Justin Suess wrote:
> On Fri, Feb 20, 2026 at 04:49:34PM +0100, Günther Noack wrote:
> > Hello!
> >
> > On Thu, Feb 19, 2026 at 03:04:59PM -0500, Justin Suess wrote:
> > > diff --git a/security/security.c b/security/security.c
> > > index 67af9228c4e9..c73196b8db4b 100644
> > > --- a/security/security.c
> > > +++ b/security/security.c
> > > @@ -4731,6 +4731,26 @@ int security_mptcp_add_subflow(struct sock *sk, struct sock *ssk)
> > >
> > > #endif /* CONFIG_SECURITY_NETWORK */
> > >
> > > +#if defined(CONFIG_SECURITY_NETWORK) && defined(CONFIG_SECURITY_PATH)
> > > +/**
> > > + * security_unix_find() - Check if a named AF_UNIX socket can connect
> > > + * @path: path of the socket being connected to
> > > + * @other: peer sock
> > > + * @flags: flags associated with the socket
> > > + *
> > > + * This hook is called to check permissions before connecting to a named
> > > + * AF_UNIX socket.
> >
> > Nit: Could we please insert a sentence about locking here?
> >
> > Something like:
> >
> > The caller holds no locks on @other.
> >
> > (Originally brought up by Mickaël in
> > https://lore.kernel.org/all/20260217.lievaS8eeng8@digikod.net/)
> >
> > Thanks,
> > –Günther
> Sounds good. Would a "Link:" to the mentioned thread be appropriate in the commit
> message?
Feel free to include relevant parts of our discussion in the commit
message, which would make a Link redundant. I think a Link is useful if
the commit message doesn't contain the whole context or misses
information, which is often the case wrt discussions or long emails.
>
> I feel like the reasoning for this is subtle but important for hook
> consumers.
Indeed. That should be explained in the hook comment.
>
> Justin
>
^ permalink raw reply
* Re: [PATCH v2 06/15] selinux: Use simple_start_creating() / simple_done_creating()
From: Paul Moore @ 2026-02-23 17:31 UTC (permalink / raw)
To: Chris Mason
Cc: NeilBrown, Christian Brauner, Alexander Viro, David Howells,
Jan Kara, Chuck Lever, Jeff Layton, Miklos Szeredi,
Amir Goldstein, John Johansen, James Morris, Serge E. Hallyn,
Stephen Smalley, Darrick J. Wong, linux-kernel, netfs,
linux-fsdevel, linux-nfs, linux-unionfs, apparmor,
linux-security-module, selinux
In-Reply-To: <20260223132533.136328-1-clm@meta.com>
On Mon, Feb 23, 2026 at 8:26 AM Chris Mason <clm@meta.com> wrote:
> NeilBrown <neilb@ownmail.net> wrote:
> > From: NeilBrown <neil@brown.name>
> >
> > Instead of explicitly locking the parent and performing a lookup in
> > selinux, use simple_start_creating(), and then use
> > simple_done_creating() to unlock.
> >
>
> Hi everyone,
>
> Amir suggested I run these through, and this commit was flagged:
>
> commit bc981629767980bbaee2ca3c7348b5a648665a9d
> Author: NeilBrown <neil@brown.name>
>
> selinux: Use simple_start_creating() / simple_done_creating()
>
> Instead of explicitly locking the parent and performing a lookup in
> selinux, use simple_start_creating(), and then use
> simple_done_creating() to unlock.
>
> This extends the region that the directory is locked for, and also
> performs a lookup. The lock extension is of no real consequence.
> The lookup uses simple_lookup() and so always succeeds. Thus when
> d_make_persistent() is called the dentry will already be hashed.
> d_make_persistent() handles this case.
>
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> Acked-by: Paul Moore <paul@paul-moore.com>
>
> > diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> > index 3245cc531555b..7d4f90e5b12a0 100644
> > --- a/security/selinux/selinuxfs.c
> > +++ b/security/selinux/selinuxfs.c
> > @@ -1931,15 +1931,16 @@ static const struct inode_operations swapover_dir_inode_operations = {
> > static struct dentry *sel_make_swapover_dir(struct super_block *sb,
> > unsigned long *ino)
> > {
> > - struct dentry *dentry = d_alloc_name(sb->s_root, ".swapover");
> > + struct dentry *dentry;
> > struct inode *inode;
> >
> > - if (!dentry)
> > + inode = sel_make_inode(sb, S_IFDIR);
> > + if (!inode)
> > return ERR_PTR(-ENOMEM);
> >
> > - inode = sel_make_inode(sb, S_IFDIR);
> > - if (!inode) {
> > - dput(dentry);
> > + dentry = simple_start_creating(sb->s_root, ".swapover");
> > + if (!dentry) {
> > + iput(inode);
> > return ERR_PTR(-ENOMEM);
> > }
>
> Can this cause a kernel crash when simple_start_creating() fails?
>
> The function simple_start_creating() returns ERR_PTR on failure, not
> NULL. The check `if (!dentry)` only catches NULL values. When
> simple_start_creating() fails and returns an ERR_PTR value like
> ERR_PTR(-ENOMEM), this value is non-NULL and the error check is bypassed.
>
> Execution would continue to d_make_persistent(dentry, inode), which
> attempts to lock dentry->d_lock, dereferencing the ERR_PTR value and
> causing a kernel crash.
>
> The old code used d_alloc_name() which returns NULL on error. The new
> code uses simple_start_creating() which returns ERR_PTR on error, but
> the error checking pattern wasn't updated to match.
>
> Should the check be `if (IS_ERR(dentry))` instead, and return dentry
> rather than ERR_PTR(-ENOMEM) to preserve the actual error code?
Good catch Chris, yes, please change this Neil and feel free to preserve my ACK.
--
paul-moore.com
^ permalink raw reply
* Re: [PATCH v4 15/17] module: Introduce hash-based integrity checking
From: Nicolas Schier @ 2026-02-23 18:41 UTC (permalink / raw)
To: Thomas Weißschuh
Cc: Nathan Chancellor, Arnd Bergmann, Luis Chamberlain, Petr Pavlu,
Sami Tolvanen, Daniel Gomez, Paul Moore, James Morris,
Serge E. Hallyn, Jonathan Corbet, Madhavan Srinivasan,
Michael Ellerman, Nicholas Piggin, Naveen N Rao, Mimi Zohar,
Roberto Sassu, Dmitry Kasatkin, Eric Snowberg, Daniel Gomez,
Aaron Tomlin, Christophe Leroy (CS GROUP), Nicolas Bouchinet,
Xiu Jianfeng, Fabian Grünbichler, Arnout Engelen,
Mattia Rizzolo, kpcyrd, Christian Heusel, Câju Mihai-Drosi,
Sebastian Andrzej Siewior, linux-kbuild, linux-kernel, linux-arch,
linux-modules, linux-security-module, linux-doc, linuxppc-dev,
linux-integrity
In-Reply-To: <0d70db8d-702b-46ec-a010-298fe6515aab@t-8ch.de>
On Mon, Feb 23, 2026 at 08:53:29AM +0100, Thomas Weißschuh wrote:
> On 2026-02-21 22:38:29+0100, Nicolas Schier wrote:
> > On Tue, Jan 13, 2026 at 01:28:59PM +0100, Thomas Weißschuh wrote:
> > > The current signature-based module integrity checking has some drawbacks
> > > in combination with reproducible builds. Either the module signing key
> > > is generated at build time, which makes the build unreproducible, or a
> > > static signing key is used, which precludes rebuilds by third parties
> > > and makes the whole build and packaging process much more complicated.
> > >
> > > The goal is to reach bit-for-bit reproducibility. Excluding certain
> > > parts of the build output from the reproducibility analysis would be
> > > error-prone and force each downstream consumer to introduce new tooling.
> > >
> > > Introduce a new mechanism to ensure only well-known modules are loaded
> > > by embedding a merkle tree root of all modules built as part of the full
> > > kernel build into vmlinux.
> > >
> > > Non-builtin modules can be validated as before through signatures.
> > >
> > > Normally the .ko module files depend on a fully built vmlinux to be
> > > available for modpost validation and BTF generation. With
> > > CONFIG_MODULE_HASHES, vmlinux now depends on the modules
> > > to build a merkle tree. This introduces a dependency cycle which is
> > > impossible to satisfy. Work around this by building the modules during
> > > link-vmlinux.sh, after vmlinux is complete enough for modpost and BTF
> > > but before the final module hashes are
> > >
> > > The PKCS7 format which is used for regular module signatures can not
> > > represent Merkle proofs, so a new kind of module signature is
> > > introduced. As this signature type is only ever used for builtin
> > > modules, no compatibility issues can arise.
> > >
> > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > > ---
> > > .gitignore | 1 +
> > > Documentation/kbuild/reproducible-builds.rst | 5 +-
> > > Makefile | 8 +-
> > > include/asm-generic/vmlinux.lds.h | 11 +
> > > include/linux/module_hashes.h | 25 ++
> > > include/linux/module_signature.h | 1 +
> > > kernel/module/Kconfig | 21 +-
> > > kernel/module/Makefile | 1 +
> > > kernel/module/hashes.c | 92 ++++++
> > > kernel/module/hashes_root.c | 6 +
> > > kernel/module/internal.h | 1 +
> > > kernel/module/main.c | 4 +-
> > > scripts/.gitignore | 1 +
> > > scripts/Makefile | 3 +
> > > scripts/Makefile.modfinal | 11 +
> > > scripts/Makefile.modinst | 13 +
> > > scripts/Makefile.vmlinux | 5 +
> > > scripts/link-vmlinux.sh | 14 +-
> > > scripts/modules-merkle-tree.c | 467 +++++++++++++++++++++++++++
> > > security/lockdown/Kconfig | 2 +-
> > > 20 files changed, 685 insertions(+), 7 deletions(-)
> > >
> > [...]
> >
> > > diff --git a/kernel/module/hashes_root.c b/kernel/module/hashes_root.c
> > > new file mode 100644
> > > index 000000000000..1abfcd3aa679
> > > --- /dev/null
> > > +++ b/kernel/module/hashes_root.c
> > > @@ -0,0 +1,6 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +
> > > +#include <linux/module_hashes.h>
> > > +
> > > +/* Blank dummy data. Will be overridden by link-vmlinux.sh */
> > > +const struct module_hashes_root module_hashes_root __module_hashes_section = {};
> > > diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> > > index e2d49122c2a1..e22837d3ac76 100644
> > > --- a/kernel/module/internal.h
> > > +++ b/kernel/module/internal.h
> > > @@ -338,6 +338,7 @@ void module_mark_ro_after_init(const Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
> > > const char *secstrings);
> > >
> > > int module_sig_check(struct load_info *info, const u8 *sig, size_t sig_len);
> > > +int module_hash_check(struct load_info *info, const u8 *sig, size_t sig_len);
> > >
> > > #ifdef CONFIG_DEBUG_KMEMLEAK
> > > void kmemleak_load_module(const struct module *mod, const struct load_info *info);
> > > diff --git a/kernel/module/main.c b/kernel/module/main.c
> > > index 2a28a0ece809..fa30b6387936 100644
> > > --- a/kernel/module/main.c
> > > +++ b/kernel/module/main.c
> > > @@ -3362,8 +3362,10 @@ static int module_integrity_check(struct load_info *info, int flags)
> > >
> > > if (IS_ENABLED(CONFIG_MODULE_SIG) && sig_type == PKEY_ID_PKCS7) {
> > > err = module_sig_check(info, sig, sig_len);
> > > + } else if (IS_ENABLED(CONFIG_MODULE_HASHES) && sig_type == PKEY_ID_MERKLE) {
> > > + err = module_hash_check(info, sig, sig_len);
> > > } else {
> > > - pr_err("module: not signed with expected PKCS#7 message\n");
> > > + pr_err("module: not signed with signature mechanism\n");
> > > err = -ENOPKG;
> >
> > To prevent others from running into the same issue:
> >
> > My first test got stuck here, as I tested with virtme-ng, which symlinks
> > modules from build tree to /lib/modules/$(uname -r)/..., resulting in
> >
> > [ 15.956855] module: not signed with signature mechanism
> > modprobe: ERROR: could not insert 'efivarfs': Package not installed
> >
> > As the modules_install step was missing, modules were not being signed.
>
> Currently the signing is deferred to installation time to keep in sync
> with regular module signing and to keep the logic simpler by not having
> to gracefully handle previously-signed files.
> But this could be changed.
I did not want to suggest changing the behaviour, that would make things
more complicated to prevent needless rebuilds. I just wanted to mention
it here to prevent others from burning time.
> > [...]
> > > diff --git a/scripts/modules-merkle-tree.c b/scripts/modules-merkle-tree.c
> > > new file mode 100644
> > > index 000000000000..a6ec0e21213b
> > > --- /dev/null
> > > +++ b/scripts/modules-merkle-tree.c
> > > @@ -0,0 +1,467 @@
> > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > +/*
> > > + * Compute hashes for modules files and build a merkle tree.
> > > + *
> > > + * Copyright (C) 2025 Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
> > > + * Copyright (C) 2025 Thomas Weißschuh <linux@weissschuh.net>
> > > + *
> > > + */
> > > +#define _GNU_SOURCE 1
> > > +#include <arpa/inet.h>
> > > +#include <err.h>
> > > +#include <unistd.h>
> > > +#include <fcntl.h>
> > > +#include <stdarg.h>
> > > +#include <stdio.h>
> > > +#include <string.h>
> > > +#include <stdbool.h>
> > > +#include <stdlib.h>
> > > +
> > > +#include <sys/stat.h>
> > > +#include <sys/mman.h>
> > > +
> > > +#include <openssl/evp.h>
> > > +#include <openssl/err.h>
> > > +
> > > +#include "ssl-common.h"
> > > +
> > > +static int hash_size;
> > > +static EVP_MD_CTX *ctx;
> > > +
> > > +struct module_signature {
> > > + uint8_t algo; /* Public-key crypto algorithm [0] */
> > > + uint8_t hash; /* Digest algorithm [0] */
> > > + uint8_t id_type; /* Key identifier type [PKEY_ID_PKCS7] */
> > > + uint8_t signer_len; /* Length of signer's name [0] */
> > > + uint8_t key_id_len; /* Length of key identifier [0] */
> > > + uint8_t __pad[3];
> > > + uint32_t sig_len; /* Length of signature data */
> > > +};
> > > +
> > > +#define PKEY_ID_MERKLE 3
> > > +
> > > +static const char magic_number[] = "~Module signature appended~\n";
> >
> > This here will be the forth definition of struct module_signature,
> > increasing the risk of unwanted diversion. I second Petr's suggestion
> > to reuse a _common_ definition instead.
>
> Ack.
>
> > (Here, even include/linux/module_signature.h could be included itself.)
>
> I'd like to avoid including internal headers from other components.
> We could move it to an UAPI header. Various other subsystems use those
> for not-really-UAPI but still ABI definitions.
Yeah, ack.
> (...)
>
> > > +static inline char *xasprintf(const char *fmt, ...)
> > > +{
> > > + va_list ap;
> > > + char *strp;
> > > + int ret;
> > > +
> > > + va_start(ap, fmt);
> > > + ret = vasprintf(&strp, fmt, ap);
> > > + va_end(ap);
> > > + if (ret == -1)
> > > + err(1, "Memory allocation failed");
> > > +
> > > + return strp;
> > > +}
> >
> > Please consider moving these x* functions into scripts/include/xalloc.h
> > for reuse. (I am sure someone else wrote this already, but I can't find
> > it...)
>
> Petr suggested it somewhere, it is done for the next revision.
>
> > thanks for all your efforts for reproducibility!
> >
> > As I have no clue about that: Is the patent for merkle trees [1] a
> > problem when integrating that here?
>
> That should have expired a long time ago [2].
> And fs-verity is also using merkle trees.
Great, thanks.
> > Can you verify if I get the mechanics roughly correct?
> >
> > * Modules are merkle tree leaves. Modules are built and logically
> > paired by the order from modules.order; a single left-over module is
> > paired with itself.
> >
> > * Hashes of paired modules are hashed again (branch node hash);
> > hashes of pairs of branch nodes' hashes are hashed again;
> > repeat until we reach the single merkle tree root hash
> >
> > * The final merkle tree root hash (and the count of tree levels) is
> > included in vmlinux
>
> The merkle tree code was written by Sebastian so he will have the best
> knowledge about it. But this is also my understanding.
I'd like to see some (rough) description in Documentation or in a commit
message at least, otherwise future me will have to ask that again.
> > 'make && find . -name '*.ko' -exec rm {} \; && make' does not rebuild
> > the in-tree modules. Shifting the module-hashes support from
> > scripts/link-vmlinux.sh to scripts/Makefile.vmlinux might (make it
> > easier) to fix this again.
>
> I'll take a look at it.
Thanks!
Kind regards,
Nicolas
> > [1]: https://worldwide.espacenet.com/patent/search/family/022107098/publication/US4309569A?q=pn%3DUS4309569
>
> [2] https://patents.stackexchange.com/questions/17901/validity-of-patent-on-merkle-trees
>
>
> Thomas
--
Nicolas
^ permalink raw reply
* Re: [PATCH v4 15/17] module: Introduce hash-based integrity checking
From: Thomas Weißschuh @ 2026-02-23 21:43 UTC (permalink / raw)
To: Nicolas Schier, Nathan Chancellor, Arnd Bergmann,
Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez,
Paul Moore, James Morris, Serge E. Hallyn, Jonathan Corbet,
Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin,
Naveen N Rao, Mimi Zohar, Roberto Sassu, Dmitry Kasatkin,
Eric Snowberg, Daniel Gomez, Aaron Tomlin,
Christophe Leroy (CS GROUP), Nicolas Bouchinet, Xiu Jianfeng,
Fabian Grünbichler, Arnout Engelen, Mattia Rizzolo, kpcyrd,
Christian Heusel, Câju Mihai-Drosi,
Sebastian Andrzej Siewior, linux-kbuild, linux-kernel, linux-arch,
linux-modules, linux-security-module, linux-doc, linuxppc-dev,
linux-integrity
In-Reply-To: <aZyfcDCWOBJJztQ2@levanger>
On 2026-02-23 19:41:52+0100, Nicolas Schier wrote:
> On Mon, Feb 23, 2026 at 08:53:29AM +0100, Thomas Weißschuh wrote:
> > On 2026-02-21 22:38:29+0100, Nicolas Schier wrote:
> > > On Tue, Jan 13, 2026 at 01:28:59PM +0100, Thomas Weißschuh wrote:
> > > > The current signature-based module integrity checking has some drawbacks
> > > > in combination with reproducible builds. Either the module signing key
> > > > is generated at build time, which makes the build unreproducible, or a
> > > > static signing key is used, which precludes rebuilds by third parties
> > > > and makes the whole build and packaging process much more complicated.
> > > >
> > > > The goal is to reach bit-for-bit reproducibility. Excluding certain
> > > > parts of the build output from the reproducibility analysis would be
> > > > error-prone and force each downstream consumer to introduce new tooling.
> > > >
> > > > Introduce a new mechanism to ensure only well-known modules are loaded
> > > > by embedding a merkle tree root of all modules built as part of the full
> > > > kernel build into vmlinux.
> > > >
> > > > Non-builtin modules can be validated as before through signatures.
> > > >
> > > > Normally the .ko module files depend on a fully built vmlinux to be
> > > > available for modpost validation and BTF generation. With
> > > > CONFIG_MODULE_HASHES, vmlinux now depends on the modules
> > > > to build a merkle tree. This introduces a dependency cycle which is
> > > > impossible to satisfy. Work around this by building the modules during
> > > > link-vmlinux.sh, after vmlinux is complete enough for modpost and BTF
> > > > but before the final module hashes are
> > > >
> > > > The PKCS7 format which is used for regular module signatures can not
> > > > represent Merkle proofs, so a new kind of module signature is
> > > > introduced. As this signature type is only ever used for builtin
> > > > modules, no compatibility issues can arise.
> > > >
> > > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > > > ---
> > > > .gitignore | 1 +
> > > > Documentation/kbuild/reproducible-builds.rst | 5 +-
> > > > Makefile | 8 +-
> > > > include/asm-generic/vmlinux.lds.h | 11 +
> > > > include/linux/module_hashes.h | 25 ++
> > > > include/linux/module_signature.h | 1 +
> > > > kernel/module/Kconfig | 21 +-
> > > > kernel/module/Makefile | 1 +
> > > > kernel/module/hashes.c | 92 ++++++
> > > > kernel/module/hashes_root.c | 6 +
> > > > kernel/module/internal.h | 1 +
> > > > kernel/module/main.c | 4 +-
> > > > scripts/.gitignore | 1 +
> > > > scripts/Makefile | 3 +
> > > > scripts/Makefile.modfinal | 11 +
> > > > scripts/Makefile.modinst | 13 +
> > > > scripts/Makefile.vmlinux | 5 +
> > > > scripts/link-vmlinux.sh | 14 +-
> > > > scripts/modules-merkle-tree.c | 467 +++++++++++++++++++++++++++
> > > > security/lockdown/Kconfig | 2 +-
> > > > 20 files changed, 685 insertions(+), 7 deletions(-)
> > > >
> > > [...]
> > >
> > > > diff --git a/kernel/module/hashes_root.c b/kernel/module/hashes_root.c
> > > > new file mode 100644
> > > > index 000000000000..1abfcd3aa679
> > > > --- /dev/null
> > > > +++ b/kernel/module/hashes_root.c
> > > > @@ -0,0 +1,6 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > +
> > > > +#include <linux/module_hashes.h>
> > > > +
> > > > +/* Blank dummy data. Will be overridden by link-vmlinux.sh */
> > > > +const struct module_hashes_root module_hashes_root __module_hashes_section = {};
> > > > diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> > > > index e2d49122c2a1..e22837d3ac76 100644
> > > > --- a/kernel/module/internal.h
> > > > +++ b/kernel/module/internal.h
> > > > @@ -338,6 +338,7 @@ void module_mark_ro_after_init(const Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
> > > > const char *secstrings);
> > > >
> > > > int module_sig_check(struct load_info *info, const u8 *sig, size_t sig_len);
> > > > +int module_hash_check(struct load_info *info, const u8 *sig, size_t sig_len);
> > > >
> > > > #ifdef CONFIG_DEBUG_KMEMLEAK
> > > > void kmemleak_load_module(const struct module *mod, const struct load_info *info);
> > > > diff --git a/kernel/module/main.c b/kernel/module/main.c
> > > > index 2a28a0ece809..fa30b6387936 100644
> > > > --- a/kernel/module/main.c
> > > > +++ b/kernel/module/main.c
> > > > @@ -3362,8 +3362,10 @@ static int module_integrity_check(struct load_info *info, int flags)
> > > >
> > > > if (IS_ENABLED(CONFIG_MODULE_SIG) && sig_type == PKEY_ID_PKCS7) {
> > > > err = module_sig_check(info, sig, sig_len);
> > > > + } else if (IS_ENABLED(CONFIG_MODULE_HASHES) && sig_type == PKEY_ID_MERKLE) {
> > > > + err = module_hash_check(info, sig, sig_len);
> > > > } else {
> > > > - pr_err("module: not signed with expected PKCS#7 message\n");
> > > > + pr_err("module: not signed with signature mechanism\n");
> > > > err = -ENOPKG;
> > >
> > > To prevent others from running into the same issue:
> > >
> > > My first test got stuck here, as I tested with virtme-ng, which symlinks
> > > modules from build tree to /lib/modules/$(uname -r)/..., resulting in
> > >
> > > [ 15.956855] module: not signed with signature mechanism
> > > modprobe: ERROR: could not insert 'efivarfs': Package not installed
> > >
> > > As the modules_install step was missing, modules were not being signed.
> >
> > Currently the signing is deferred to installation time to keep in sync
> > with regular module signing and to keep the logic simpler by not having
> > to gracefully handle previously-signed files.
> > But this could be changed.
>
> I did not want to suggest changing the behaviour, that would make things
> more complicated to prevent needless rebuilds. I just wanted to mention
> it here to prevent others from burning time.
Understood.
> > > [...]
> > > > diff --git a/scripts/modules-merkle-tree.c b/scripts/modules-merkle-tree.c
> > > > new file mode 100644
> > > > index 000000000000..a6ec0e21213b
> > > > --- /dev/null
> > > > +++ b/scripts/modules-merkle-tree.c
> > > > @@ -0,0 +1,467 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-or-later
> > > > +/*
> > > > + * Compute hashes for modules files and build a merkle tree.
> > > > + *
> > > > + * Copyright (C) 2025 Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
> > > > + * Copyright (C) 2025 Thomas Weißschuh <linux@weissschuh.net>
> > > > + *
> > > > + */
> > > > +#define _GNU_SOURCE 1
> > > > +#include <arpa/inet.h>
> > > > +#include <err.h>
> > > > +#include <unistd.h>
> > > > +#include <fcntl.h>
> > > > +#include <stdarg.h>
> > > > +#include <stdio.h>
> > > > +#include <string.h>
> > > > +#include <stdbool.h>
> > > > +#include <stdlib.h>
> > > > +
> > > > +#include <sys/stat.h>
> > > > +#include <sys/mman.h>
> > > > +
> > > > +#include <openssl/evp.h>
> > > > +#include <openssl/err.h>
> > > > +
> > > > +#include "ssl-common.h"
> > > > +
> > > > +static int hash_size;
> > > > +static EVP_MD_CTX *ctx;
> > > > +
> > > > +struct module_signature {
> > > > + uint8_t algo; /* Public-key crypto algorithm [0] */
> > > > + uint8_t hash; /* Digest algorithm [0] */
> > > > + uint8_t id_type; /* Key identifier type [PKEY_ID_PKCS7] */
> > > > + uint8_t signer_len; /* Length of signer's name [0] */
> > > > + uint8_t key_id_len; /* Length of key identifier [0] */
> > > > + uint8_t __pad[3];
> > > > + uint32_t sig_len; /* Length of signature data */
> > > > +};
> > > > +
> > > > +#define PKEY_ID_MERKLE 3
> > > > +
> > > > +static const char magic_number[] = "~Module signature appended~\n";
> > >
> > > This here will be the forth definition of struct module_signature,
> > > increasing the risk of unwanted diversion. I second Petr's suggestion
> > > to reuse a _common_ definition instead.
> >
> > Ack.
> >
> > > (Here, even include/linux/module_signature.h could be included itself.)
> >
> > I'd like to avoid including internal headers from other components.
> > We could move it to an UAPI header. Various other subsystems use those
> > for not-really-UAPI but still ABI definitions.
>
> Yeah, ack.
What exactly is the 'ack' for?
* Avoiding to include internal headers?
* Moving the definition to UAPI headers?
(...)
> > > Can you verify if I get the mechanics roughly correct?
> > >
> > > * Modules are merkle tree leaves. Modules are built and logically
> > > paired by the order from modules.order; a single left-over module is
> > > paired with itself.
> > >
> > > * Hashes of paired modules are hashed again (branch node hash);
> > > hashes of pairs of branch nodes' hashes are hashed again;
> > > repeat until we reach the single merkle tree root hash
> > >
> > > * The final merkle tree root hash (and the count of tree levels) is
> > > included in vmlinux
> >
> > The merkle tree code was written by Sebastian so he will have the best
> > knowledge about it. But this is also my understanding.
>
> I'd like to see some (rough) description in Documentation or in a commit
> message at least, otherwise future me will have to ask that again.
Ack in general. I'd prefer to document it in a source code comment,
though. That feels like the best fit to me.
Thomas
^ permalink raw reply
* Re: [PATCH v2 01/15] VFS: note error returns is documentation for various lookup functions
From: NeilBrown @ 2026-02-23 22:04 UTC (permalink / raw)
To: Chris Mason
Cc: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Jeff Layton, Miklos Szeredi, Amir Goldstein,
John Johansen, Paul Moore, James Morris, Serge E. Hallyn,
Stephen Smalley, Darrick J. Wong, linux-kernel, netfs,
linux-fsdevel, linux-nfs, linux-unionfs, apparmor,
linux-security-module, selinux
In-Reply-To: <20260223135517.1229434-1-clm@meta.com>
On Tue, 24 Feb 2026, Chris Mason wrote:
> NeilBrown <neilb@ownmail.net> wrote:
> > From: NeilBrown <neil@brown.name>
> >
> > Darrick recently noted that try_lookup_noperm() is documented as
> > "Look up a dentry by name in the dcache, returning NULL if it does not
> > currently exist." but it can in fact return an error.
> >
> > So update the documentation for that and related function.
> >
>
> Hi everyone,
>
> I don't normally forward the typos, but since this is a documentation-y patch:
I'm certainly happy to receive them. Thanks for these and the others
I also found ....
>
> commit 0254b9b974f23889898562aa94f6428bf30eb6b5
> Author: NeilBrown <neil@brown.name>
>
> VFS: note error returns is documentation for various lookup functions
> ^^^^^ in?
>
> Darrick recently noted that try_lookup_noperm() is documented as
> "Look up a dentry by name in the dcache, returning NULL if it does not
> currently exist." but it can in fact return an error. So update the
> documentation for that and related function.
^^functions
Thanks,
NeilBrown
^ permalink raw reply
* Re: [PATCH v2 06/15] selinux: Use simple_start_creating() / simple_done_creating()
From: NeilBrown @ 2026-02-23 22:07 UTC (permalink / raw)
To: Paul Moore
Cc: Chris Mason, Christian Brauner, Alexander Viro, David Howells,
Jan Kara, Chuck Lever, Jeff Layton, Miklos Szeredi,
Amir Goldstein, John Johansen, James Morris, Serge E. Hallyn,
Stephen Smalley, Darrick J. Wong, linux-kernel, netfs,
linux-fsdevel, linux-nfs, linux-unionfs, apparmor,
linux-security-module, selinux
In-Reply-To: <CAHC9VhSVjLNeTdxHmwYsGX75Z4FOAP+26=PjVdFxpmEkTrPvxA@mail.gmail.com>
On Tue, 24 Feb 2026, Paul Moore wrote:
> On Mon, Feb 23, 2026 at 8:26 AM Chris Mason <clm@meta.com> wrote:
> > NeilBrown <neilb@ownmail.net> wrote:
> > > From: NeilBrown <neil@brown.name>
> > >
> > > Instead of explicitly locking the parent and performing a lookup in
> > > selinux, use simple_start_creating(), and then use
> > > simple_done_creating() to unlock.
> > >
> >
> > Hi everyone,
> >
> > Amir suggested I run these through, and this commit was flagged:
> >
> > commit bc981629767980bbaee2ca3c7348b5a648665a9d
> > Author: NeilBrown <neil@brown.name>
> >
> > selinux: Use simple_start_creating() / simple_done_creating()
> >
> > Instead of explicitly locking the parent and performing a lookup in
> > selinux, use simple_start_creating(), and then use
> > simple_done_creating() to unlock.
> >
> > This extends the region that the directory is locked for, and also
> > performs a lookup. The lock extension is of no real consequence.
> > The lookup uses simple_lookup() and so always succeeds. Thus when
> > d_make_persistent() is called the dentry will already be hashed.
> > d_make_persistent() handles this case.
> >
> > Reviewed-by: Jeff Layton <jlayton@kernel.org>
> > Acked-by: Paul Moore <paul@paul-moore.com>
> >
> > > diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> > > index 3245cc531555b..7d4f90e5b12a0 100644
> > > --- a/security/selinux/selinuxfs.c
> > > +++ b/security/selinux/selinuxfs.c
> > > @@ -1931,15 +1931,16 @@ static const struct inode_operations swapover_dir_inode_operations = {
> > > static struct dentry *sel_make_swapover_dir(struct super_block *sb,
> > > unsigned long *ino)
> > > {
> > > - struct dentry *dentry = d_alloc_name(sb->s_root, ".swapover");
> > > + struct dentry *dentry;
> > > struct inode *inode;
> > >
> > > - if (!dentry)
> > > + inode = sel_make_inode(sb, S_IFDIR);
> > > + if (!inode)
> > > return ERR_PTR(-ENOMEM);
> > >
> > > - inode = sel_make_inode(sb, S_IFDIR);
> > > - if (!inode) {
> > > - dput(dentry);
> > > + dentry = simple_start_creating(sb->s_root, ".swapover");
> > > + if (!dentry) {
> > > + iput(inode);
> > > return ERR_PTR(-ENOMEM);
> > > }
> >
> > Can this cause a kernel crash when simple_start_creating() fails?
> >
> > The function simple_start_creating() returns ERR_PTR on failure, not
> > NULL. The check `if (!dentry)` only catches NULL values. When
> > simple_start_creating() fails and returns an ERR_PTR value like
> > ERR_PTR(-ENOMEM), this value is non-NULL and the error check is bypassed.
> >
> > Execution would continue to d_make_persistent(dentry, inode), which
> > attempts to lock dentry->d_lock, dereferencing the ERR_PTR value and
> > causing a kernel crash.
> >
> > The old code used d_alloc_name() which returns NULL on error. The new
> > code uses simple_start_creating() which returns ERR_PTR on error, but
> > the error checking pattern wasn't updated to match.
> >
> > Should the check be `if (IS_ERR(dentry))` instead, and return dentry
> > rather than ERR_PTR(-ENOMEM) to preserve the actual error code?
>
> Good catch Chris, yes, please change this Neil and feel free to
> preserve my ACK.
Thanks.
I've made it
dentry = simple_start_creating(sb->s_root, ".swapover");
if (IS_ERR(dentry)) {
iput(inode);
return dentry;
}
NeilBrown
^ permalink raw reply
* Re: [PATCH] lsm: move inode IS_PRIVATE checks to individual LSMs
From: Paul Moore @ 2026-02-23 22:21 UTC (permalink / raw)
To: Casey Schaufler
Cc: danieldurning.work, linux-security-module, selinux,
linux-integrity, stephen.smalley.work, jmorris, serge,
john.johansen, zohar, roberto.sassu, dmitry.kasatkin, mic,
takedakn, penguin-kernel
In-Reply-To: <9229d70d-aa7a-459f-b005-695e99888783@schaufler-ca.com>
On Fri, Feb 20, 2026 at 4:13 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 2/20/2026 11:54 AM, danieldurning.work@gmail.com wrote:
> > From: Daniel Durning <danieldurning.work@gmail.com>
> >
> > Move responsibility of bypassing S_PRIVATE inodes to the
> > individual LSMs. Originally the LSM framework would skip calling
> > the hooks on any inode that was marked S_PRIVATE. This would
> > prevent the LSMs from controlling access to any inodes marked as
> > such (ie. pidfds). We now perform the same IS_PRIVATE checks
> > within the LSMs instead. This is consistent with the general goal
> > of deferring as much as possible to the individual LSMs.
>
> Um ... ick?
>
> Sure, we generally want the LSMs to be responsible for their own
> decisions, but that doesn't look like the point to me. What appears
> to be the issue is that pidfs isn't using S_PRIVATE in a way that
> conveys the necessary information to the LSMs.
First off, consider this the annual reminder for everyone to *please*
trim their replies when discussing things on-list. Everything is
archived on lore, we're not losing anything, and it makes things *so*
much easier to read if we don't have to skim over the entire email to
make sure we haven't missed any comments.
Now, back to the S_PRIVATE issue ...
I was the one who first suggested (it may have been on the SELinux
list, or in an off-list discussion, not sure?) that moving the
S_PRIVATE check into the individual LSMs was a way to work around the
issue with pidfd/pidfs, so please don't blame Daniel for this, he has
been doing good work trying to solve a rather ugly problem.
> > This reorganization enables the LSMs to eventually implement
> > checks or labeling for some specific S_PRIVATE inodes like pidfds.
>
> We could consider these or similar changes when that eventuality occurs.
To be clear, that time is now, that is just a dependency of that which
needs to be sorted out first.
> I would strongly suggest that this is a pidfs issue, not an LSM
> infrastructure issue.
I'm not going to argue with that, and perhaps that is a good next
step: send a quick RFC patch to the VFS folks, with the LSM list CC'd,
that drops setting the S_PRIVATE flag to see if they complain too
loudly. Based on other threads, Christian is aware that we are
starting to look at better/proper handling of pidfds/pidfs so he may
be open to dropping S_PRIVATE since it doesn't really have much impact
outside of the LSM, but who knows; the VFS folks have been growing a
bit more anti-LSM as of late.
diff --git a/fs/pidfs.c b/fs/pidfs.c
index 318253344b5c..4cec73b4cbcf 100644
--- a/fs/pidfs.c
+++ b/fs/pidfs.c
@@ -921,7 +921,7 @@ static int pidfs_init_inode(struct inode *inode, void *data)
const struct pid *pid = data;
inode->i_private = data;
- inode->i_flags |= S_PRIVATE | S_ANON_INODE;
+ inode->i_flags |= S_ANON_INODE;
/* We allow to set xattrs. */
inode->i_flags &= ~S_IMMUTABLE;
inode->i_mode |= S_IRWXU;
--
paul-moore.com
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox