* Re: [PATCH v14 2/9] rust: rename `AlwaysRefCounted` to `RefCounted`.
From: Gary Guo @ 2026-02-05 13:31 UTC (permalink / raw)
To: Andreas Hindborg, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, 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
Cc: 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: <20260204-unique-ref-v14-2-17cb29ebacbb@kernel.org>
On Wed Feb 4, 2026 at 11:56 AM GMT, Andreas Hindborg wrote:
> From: Oliver Mangold <oliver.mangold@pm.me>
>
> There are types where it may both be reference counted in some cases and
> owned in others. In such cases, obtaining `ARef<T>` from `&T` would be
> unsound as it allows creation of `ARef<T>` copy from `&Owned<T>`.
>
> Therefore, we split `AlwaysRefCounted` into `RefCounted` (which `ARef<T>`
> would require) and a marker trait to indicate that the type is always
> reference counted (and not `Ownable`) so the `&T` -> `ARef<T>` conversion
> is possible.
>
> - Rename `AlwaysRefCounted` to `RefCounted`.
> - Add a new unsafe trait `AlwaysRefCounted`.
> - Implement the new trait `AlwaysRefCounted` for the newly renamed
> `RefCounted` implementations. This leaves functionality of existing
> implementers of `AlwaysRefCounted` intact.
>
> Original patch by Oliver Mangold <oliver.mangold@pm.me> [1].
>
> Link: https://lore.kernel.org/r/20251117-unique-ref-v13-2-b5b243df1250@pm.me [1]
> Suggested-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
I think you also need to update the `AlwaysRefCounted` reference mentioned in
the `Owned` patch too? (Or perhaps this patch should be moved before `Owned`
instead?)
With that fixed:
Reviewed-by: Gary Guo <gary@garyguo.net>
> ---
> rust/kernel/auxiliary.rs | 7 +++++-
> rust/kernel/block/mq/request.rs | 15 +++++++------
> rust/kernel/cred.rs | 13 ++++++++++--
> rust/kernel/device.rs | 10 ++++++---
> rust/kernel/device/property.rs | 7 +++++-
> rust/kernel/drm/device.rs | 10 ++++++---
> rust/kernel/drm/gem/mod.rs | 8 ++++---
> rust/kernel/fs/file.rs | 16 ++++++++++----
> rust/kernel/i2c.rs | 16 +++++++++-----
> rust/kernel/mm.rs | 15 +++++++++----
> rust/kernel/mm/mmput_async.rs | 9 ++++++--
> rust/kernel/opp.rs | 10 ++++++---
> rust/kernel/owned.rs | 2 +-
> rust/kernel/pci.rs | 10 ++++++++-
> rust/kernel/pid_namespace.rs | 12 +++++++++--
> rust/kernel/platform.rs | 7 +++++-
> rust/kernel/sync/aref.rs | 47 ++++++++++++++++++++++++++---------------
> rust/kernel/task.rs | 10 ++++++---
> rust/kernel/types.rs | 3 ++-
> 19 files changed, 164 insertions(+), 63 deletions(-)
^ permalink raw reply
* Re: [PATCH v14 2/9] rust: rename `AlwaysRefCounted` to `RefCounted`.
From: Gary Guo @ 2026-02-05 13:33 UTC (permalink / raw)
To: Gary Guo, Andreas Hindborg, Miguel Ojeda, Boqun Feng,
Björn Roy Baron, Benno Lossin, 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
Cc: 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: <DG72CY2P36F9.2O7OIN36KW8F8@garyguo.net>
On Thu Feb 5, 2026 at 1:31 PM GMT, Gary Guo wrote:
> On Wed Feb 4, 2026 at 11:56 AM GMT, Andreas Hindborg wrote:
>> From: Oliver Mangold <oliver.mangold@pm.me>
>>
>> There are types where it may both be reference counted in some cases and
>> owned in others. In such cases, obtaining `ARef<T>` from `&T` would be
>> unsound as it allows creation of `ARef<T>` copy from `&Owned<T>`.
>>
>> Therefore, we split `AlwaysRefCounted` into `RefCounted` (which `ARef<T>`
>> would require) and a marker trait to indicate that the type is always
>> reference counted (and not `Ownable`) so the `&T` -> `ARef<T>` conversion
>> is possible.
>>
>> - Rename `AlwaysRefCounted` to `RefCounted`.
>> - Add a new unsafe trait `AlwaysRefCounted`.
>> - Implement the new trait `AlwaysRefCounted` for the newly renamed
>> `RefCounted` implementations. This leaves functionality of existing
>> implementers of `AlwaysRefCounted` intact.
>>
>> Original patch by Oliver Mangold <oliver.mangold@pm.me> [1].
>>
>> Link: https://lore.kernel.org/r/20251117-unique-ref-v13-2-b5b243df1250@pm.me [1]
>> Suggested-by: Alice Ryhl <aliceryhl@google.com>
>> Reviewed-by: Daniel Almeida <daniel.almeida@collabora.com>
>> Signed-off-by: Andreas Hindborg <a.hindborg@kernel.org>
>
> I think you also need to update the `AlwaysRefCounted` reference mentioned in
> the `Owned` patch too? (Or perhaps this patch should be moved before `Owned`
> instead?)
Actually I re-read the comment in first patch, the text indeed should refer to
`AlwaysRefCounted`. Please disregard this comment.
Best,
Gary
>
> With that fixed:
>
> Reviewed-by: Gary Guo <gary@garyguo.net>
>
>> ---
>> rust/kernel/auxiliary.rs | 7 +++++-
>> rust/kernel/block/mq/request.rs | 15 +++++++------
>> rust/kernel/cred.rs | 13 ++++++++++--
>> rust/kernel/device.rs | 10 ++++++---
>> rust/kernel/device/property.rs | 7 +++++-
>> rust/kernel/drm/device.rs | 10 ++++++---
>> rust/kernel/drm/gem/mod.rs | 8 ++++---
>> rust/kernel/fs/file.rs | 16 ++++++++++----
>> rust/kernel/i2c.rs | 16 +++++++++-----
>> rust/kernel/mm.rs | 15 +++++++++----
>> rust/kernel/mm/mmput_async.rs | 9 ++++++--
>> rust/kernel/opp.rs | 10 ++++++---
>> rust/kernel/owned.rs | 2 +-
>> rust/kernel/pci.rs | 10 ++++++++-
>> rust/kernel/pid_namespace.rs | 12 +++++++++--
>> rust/kernel/platform.rs | 7 +++++-
>> rust/kernel/sync/aref.rs | 47 ++++++++++++++++++++++++++---------------
>> rust/kernel/task.rs | 10 ++++++---
>> rust/kernel/types.rs | 3 ++-
>> 19 files changed, 164 insertions(+), 63 deletions(-)
^ permalink raw reply
* Re: [PATCH 1/5] export file_close_fd and task_work_add
From: Alice Ryhl @ 2026-02-05 13:45 UTC (permalink / raw)
To: Lorenzo Stoakes
Cc: Greg Kroah-Hartman, Carlos Llamas, Alexander Viro,
Christian Brauner, Jan Kara, Paul Moore, James Morris,
Serge E. Hallyn, Andrew Morton, Dave Chinner, Qi Zheng,
Roman Gushchin, Muchun Song, David Hildenbrand, Liam R. Howlett,
Vlastimil Babka, Mike Rapoport, Suren Baghdasaryan, Michal Hocko,
Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Trevor Gross, Danilo Krummrich,
kernel-team, linux-fsdevel, linux-kernel, linux-security-module,
linux-mm, rust-for-linux
In-Reply-To: <9a037fdf-1a98-437f-8b80-7fdc53d5b0fa@lucifer.local>
On Thu, Feb 05, 2026 at 11:53:19AM +0000, Lorenzo Stoakes wrote:
> On Thu, Feb 05, 2026 at 11:42:46AM +0000, Alice Ryhl wrote:
> > On Thu, Feb 05, 2026 at 11:20:33AM +0000, Lorenzo Stoakes wrote:
> > > On Thu, Feb 05, 2026 at 10:51:26AM +0000, Alice Ryhl wrote:
> > > > This exports the functionality needed by Binder to close file
> > > > descriptors.
> > > >
> > > > When you send a fd over Binder, what happens is this:
> > > >
> > > > 1. The sending process turns the fd into a struct file and stores it in
> > > > the transaction object.
> > > > 2. When the receiving process gets the message, the fd is installed as a
> > > > fd into the current process.
> > > > 3. When the receiving process is done handling the message, it tells
> > > > Binder to clean up the transaction. As part of this, fds embedded in
> > > > the transaction are closed.
> > > >
> > > > Note that it was not always implemented like this. Previously the
> > > > sending process would install the fd directly into the receiving proc in
> > > > step 1, but as discussed previously [1] this is not ideal and has since
> > > > been changed so that fd install happens during receive.
> > > >
> > > > The functions being exported here are for closing the fd in step 3. They
> > > > are required because closing a fd from an ioctl is in general not safe.
> > > > This is to meet the requirements for using fdget(), which is used by the
> > > > ioctl framework code before calling into the driver's implementation of
> > > > the ioctl. Binder works around this with this sequence of operations:
> > > >
> > > > 1. file_close_fd()
> > > > 2. get_file()
> > > > 3. filp_close()
> > > > 4. task_work_add(current, TWA_RESUME)
> > > > 5. <binder returns from ioctl>
> > > > 6. fput()
> > > >
> > > > This ensures that when fput() is called in the task work, the fdget()
> > > > that the ioctl framework code uses has already been fdput(), so if the
> > > > fd being closed happens to be the same fd, then the fd is not closed
> > > > in violation of the fdget() rules.
> > >
> > > I'm not really familiar with this mechanism but you're already talking about
> > > this being a workaround so strikes me the correct thing to do is to find a way
> > > to do this in the kernel sensibly rather than exporting internal implementation
> > > details and doing it in binder.
> >
> > I did previously submit a patch that implemented this logic outside of
> > Binder, but I was advised to move it into Binder.
>
> Right yeah that's just odd to me, we really do not want to be adding internal
> implementation details to drivers.
>
> This is based on bitter experience of bugs being caused by drivers abusing every
> interface they get, which is basically exactly what always happens, sadly.
>
> And out-of-tree is heavily discouraged.
>
> Also can we use EXPORT_SYMBOL_FOR_MODULES() for anything we do need to export to
> make it explicitly only for binder, perhaps?
>
> >
> > But I'm happy to submit a patch to extract this logic into some sort of
> > close_fd_safe() method that can be called even if said fd is currently
> > held using fdget().
>
> Yup, especially given Christian's view on the kernel task export here I think
> that's a more sensible approach.
>
> But obviously I defer the sensible-ness of this to him as I am but an mm dev :)
Quick sketch of how this would look:
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index adde1e40cccd..6fb7175ff69b 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -64,7 +64,6 @@
#include <linux/spinlock.h>
#include <linux/ratelimit.h>
#include <linux/syscalls.h>
-#include <linux/task_work.h>
#include <linux/sizes.h>
#include <linux/ktime.h>
@@ -1962,68 +1961,6 @@ static bool binder_validate_fixup(struct binder_proc *proc,
return (fixup_offset >= last_min_offset);
}
-/**
- * struct binder_task_work_cb - for deferred close
- *
- * @twork: callback_head for task work
- * @file: file to close
- *
- * Structure to pass task work to be handled after
- * returning from binder_ioctl() via task_work_add().
- */
-struct binder_task_work_cb {
- struct callback_head twork;
- struct file *file;
-};
-
-/**
- * binder_do_fd_close() - close list of file descriptors
- * @twork: callback head for task work
- *
- * It is not safe to call ksys_close() during the binder_ioctl()
- * function if there is a chance that binder's own file descriptor
- * might be closed. This is to meet the requirements for using
- * fdget() (see comments for __fget_light()). Therefore use
- * task_work_add() to schedule the close operation once we have
- * returned from binder_ioctl(). This function is a callback
- * for that mechanism and does the actual ksys_close() on the
- * given file descriptor.
- */
-static void binder_do_fd_close(struct callback_head *twork)
-{
- struct binder_task_work_cb *twcb = container_of(twork,
- struct binder_task_work_cb, twork);
-
- fput(twcb->file);
- kfree(twcb);
-}
-
-/**
- * binder_deferred_fd_close() - schedule a close for the given file-descriptor
- * @fd: file-descriptor to close
- *
- * See comments in binder_do_fd_close(). This function is used to schedule
- * a file-descriptor to be closed after returning from binder_ioctl().
- */
-static void binder_deferred_fd_close(int fd)
-{
- struct binder_task_work_cb *twcb;
-
- twcb = kzalloc(sizeof(*twcb), GFP_KERNEL);
- if (!twcb)
- return;
- init_task_work(&twcb->twork, binder_do_fd_close);
- twcb->file = file_close_fd(fd);
- if (twcb->file) {
- // pin it until binder_do_fd_close(); see comments there
- get_file(twcb->file);
- filp_close(twcb->file, current->files);
- task_work_add(current, &twcb->twork, TWA_RESUME);
- } else {
- kfree(twcb);
- }
-}
-
static void binder_transaction_buffer_release(struct binder_proc *proc,
struct binder_thread *thread,
struct binder_buffer *buffer,
@@ -2183,7 +2120,10 @@ static void binder_transaction_buffer_release(struct binder_proc *proc,
offset, sizeof(fd));
WARN_ON(err);
if (!err) {
- binder_deferred_fd_close(fd);
+ /*
+ * Intentionally ignore EBADF errors here.
+ */
+ close_fd_safe(fd, GFP_KERNEL | __GFP_NOFAIL);
/*
* Need to make sure the thread goes
* back to userspace to complete the
diff --git a/fs/file.c b/fs/file.c
index 0a4f3bdb2dec..58e3825e846c 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -21,6 +21,7 @@
#include <linux/rcupdate.h>
#include <linux/close_range.h>
#include <linux/file_ref.h>
+#include <linux/task_work.h>
#include <net/sock.h>
#include <linux/init_task.h>
@@ -1525,3 +1526,47 @@ int iterate_fd(struct files_struct *files, unsigned n,
return res;
}
EXPORT_SYMBOL(iterate_fd);
+
+struct close_fd_safe_task_work {
+ struct callback_head twork;
+ struct file *file;
+};
+
+static void close_fd_safe_callback(struct callback_head *twork)
+{
+ struct close_fd_safe_task_work *twcb = container_of(twork,
+ struct close_fd_safe_task_work, twork);
+
+ fput(twcb->file);
+ kfree(twcb);
+}
+
+/**
+ * close_fd_safe - close the given fd
+ * @fd: file descriptor to close
+ * @flags: gfp flags for allocation of task work
+ *
+ * This closes an fd. Unlike close_fd(), this may be used even if the fd is
+ * currently held with fdget().
+ *
+ * Returns: 0 or an error code
+ */
+int close_fd_safe(unsigned int fd, gfp_t flags)
+{
+ struct close_fd_safe_task_work *twcb;
+
+ twcb = kzalloc(sizeof(*twcb), flags);
+ if (!twcb)
+ return -ENOMEM;
+ init_task_work(&twcb->twork, close_fd_safe_callback);
+ twcb->file = file_close_fd(fd);
+ if (!twcb->file) {
+ kfree(twcb);
+ return -EBADF;
+ }
+
+ get_file(twcb->file);
+ filp_close(twcb->file, current->files);
+ task_work_add(current, &twcb->twork, TWA_RESUME);
+ return 0;
+}
diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index c45306a9f007..1c99a56c0cdf 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -111,6 +111,7 @@ int iterate_fd(struct files_struct *, unsigned,
const void *);
extern int close_fd(unsigned int fd);
+extern int close_fd_safe(unsigned int fd, gfp_t flags);
extern struct file *file_close_fd(unsigned int fd);
extern struct kmem_cache *files_cachep;
^ permalink raw reply related
* Re: [PATCH v2 0/6] Landlock: Implement scope control for pathname Unix sockets
From: Justin Suess @ 2026-02-05 15:22 UTC (permalink / raw)
To: mic
Cc: demiobenour, fahimitahera, gnoack3000, gnoack, hi, jannh,
linux-security-module, m, matthieu, utilityemal77
In-Reply-To: <20260204.vug7Osheiwou@digikod.net>
On 2/4/26 13:28, Mickaël Salaün wrote:
>> [...]
>> Tingmao:
>>
>> For connecting a pathname unix socket, the order of the hooks landlock sees is something like:
>>
>> 1. security_unix_find. (to look up the paths)
>>
>> 2. security_unix_may_send, security_unix_stream_connect (after the path is looked up)
>>
>> Which for is called in DGRAM:
>>
>> unix_dgram_connect OR unix_dgram_sendmsg
>>
>> and for STREAM:
>>
>> unix_stream_connect
>>
>> IIRC, the path lookup only occurs in this order always, so in the logic as you have it the domain_is_scoped()
>> would be called twice, once from the security_unix_find when you call it in step two, and once from the
>> domain scope hooks. (If access was allowed from security_unix_find)
>>
>> There are a couple of things to consider.
>>
>> ---
>>
>> Audit blockers need special handling:
>>
>> Here's an example:
>>
>> 1. Program A enforces a ruleset with RESOLVE_UNIX and the unix pathname scope bit, with no rules with that
>> access bit (deny all for RESOLVE_UNIX).
>>
>> 2. Program A connects to /tmp/mysock.sock ran by program B, which is outside the domain.
>>
>> 2. security_unix_find is hit to lookup the path, and the RESOLVE_UNIX code doesn't grant access to
>> /tmp/mysock.sock, so it calls domain_is_scoped
>>
>> 3. domain_is_scoped denies it as well, so now we must log an audit record.
>>
>> When logging the denial, we have to include both blockers "scope.unix_socket" and "fs.resolve_unix" for the
>> denial, because it is the absence of both that caused the denial. I think the refer right has similar cases for auditing, so there is precedent for this (multiple blockers for an audit message).
> That's a good point, and it would give more informations to diagnose
> issues. However, being able to identify if both accesses are denied
> would require to check both, whereas the first is enough to know that
> Landlock denies the access. So, if we can return both records without
> continuing the security checks, that's good, otherwise we should stop
> ASAP and return the error.
Maybe I'm missing something, but if the flags interact in an "OR" manner
wouldn't we need to check both? In your proposal where RESOLVE_UNIX
implies the scoped flag, if a program connects to a unix socket that is within
the domain but does not have an explicit RESOLVE_UNIX exception, we must
still check for the case that the access is scoped.
---
(Given LANDLOCK_ACCESS_FS_RESOLVE_UNIX and LANDLOCK_SCOPE_PATHNAME_UNIX_SOCKET
are set)
case 1: access to socket within domain and no RESOLVE_UNIX rule covers the access
We check first in security_unix_find hook and find there is no rule allowing the access.
After the check fails, because LANDLOCK_SCOPE_PATHNAME_UNIX_SOCKET is set
we then check is_domain_scoped, the check allows the access.
case 2: access to socket outside domain but RESOLVE_UNIX rule covers the access
We check first in security_unix_find hook and find there is a rule allowing the access.
We can allow the access early (short-circuit eval) without calling is_domain_scoped.
case 4: access to socket inside domain and RESOLVE_UNIX covers the access
We check first in security_unix_find hook and find there is a rule allowing the access.
We can allow the access early (short-circuit eval) without calling is_domain_scoped. (same as case 2)
case 4: access to socket outside domain and no RESOLVE_UNIX covers the access
We check first in security_unix_find hook and find there is no rule allowing the access.
After the check fails, because LANDLOCK_SCOPE_PATHNAME_UNIX_SOCKET is set
we then check is_domain_scoped, the check does not allow the access. (it is the combination
of the two checks failing that denied the access).
---
Case 4 is what I'm specifically considering would need to have both blockers listed in a denial audit.
We can't short circuit in that case because we have to check the scoping before denying.
Let me know if I'm misunderstanding this.
(PS: IIRC the hooks used by the LANDLOCK_SCOPE_PATHNAME_UNIX_SOCKET
will never be hit if the check in security_unix_find fails. So some logic to check this
access in security_unix_find will be needed).
> Anyway, that might not be needed if we end up with my latest proposal
> about always setting scope.unix_socket when fs.resolve_unix is set.
>
>> ---
>>
>> Dual lookup for domain_is_scoped. Consider this case:
>>
>> 1. Program A enforces a ruleset with RESOLVE_UNIX and the unix pathname scope bit, with no rules with that
>> access bit (deny all for RESOLVE_UNIX).
>>
>> 2. Program A connects to Program C's /tmp/foo.sock, which for the purposes of this example is in the domain of program A.
>>
>> 3. security_unix_find is hit to lookup the path, and the RESOLVE_UNIX code doesn't grant access to
>> /tmp/mysock.sock, so it calls domain_is_scoped. Access is granted, and continues. (LSM hook complete)
>>
>> 4. The connection proceeds past the path lookup stage, and now security_unix_may_send, or security_unix_stream_connect gets called. This requires ANOTHER domain_is_scoped access check.
>>
>> While I don't THINK this introduces a TOCTOU, it is a little confusing.
>>
>> This does mean that we look up the domain twice, if this is implemented naively. I think we can then just
>> skip the task credential checks then for security_unix_may_send and security_unix_stream_connect **for
>> connecting to pathname sockets**, since the domain_is_scoped will already have been called in landlock's
>> security_unix_find hook, eliminating the need for handling pathname socket domain checks layer on.
>>
>>>> I definitely agree that it is tricky, but making same-scope access be
>>>> allowed (i.e. the suggested idea above) would only get rid of step 1,
>>>> which I think is the "simpler" bit. The extra logic in step 2 is still
>>>> needed.
>>>>
>>>> I definitely agree with pro1 tho.
>>> Yes, you are describing the logic for the variant where
>>> LANDLOCK_ACCESS_FS_RESOLVE_UNIX does not implicitly permit access from
>>> within the same scope. In that variant, there can be situations where
>>> the first hook can deny the action immediately.
>>>
>>> In the variant where LANDLOCK_ACCESS_FS_RESOLVE_UNIX *does* implicitly
>>> allow access from within the same scope, that shortcutting is not
>>> possible. On the upside however, there is no need to distinguish
>>> whether the scope flag is set when we are in the security_unix_find()
>>> hook, because access from within the same scope is always permitted.
>>> (That is the simplification I meant.)
>>>
>>>
>>>>> AGAINST:
>>>>>
>>>>> (con1) It would work differently than the other scoped access rights
>>>>> that we already have.
>>>>>
>>>>> A speculative feature that could potentially be built with the
>>>>> scoped access rights is that we could add a rule to permit IPC
>>>>> to other Landlock scopes, e.g. introducing a new rule type
>>>>>
>>>>> struct landlock_scope_attr {
>>>>> __u64 allowed_access; /* for "scoped" bits */
>>>>> /* some way to identify domains */
>>>>> }
>>>>>
>>>>> so that we could make IPC access to other Landlock domains
>>>>> configurable.
>>>>>
>>>>> If the scoped bit and the FS RESOLVE_UNIX bit were both
>>>>> conflated in RESOLVE_UNIX, it would not be possible to make
>>>>> UNIX connections configurable in such a way.
>>>> This exact API would no longer work, but if we give up the equivalence
>>>> between scope bits and the landlock_scope_attr struct, then we can do
>>>> something like:
>>>>
>>>> struct landlock_scope_attr {
>>>> __u64 ptrace:1; /* Note that this is not a (user controllable) scope bit! */
>>>> __u64 abstract_unix_socket:1;
>>>> __u64 pathname_unix_socket:1;
>>>> /* ... */
>>>>
>>>> __u64 allowed_signals;
>>>>
>>>> /*
>>>> * some way to identify domains, maybe we could use the audit domain
>>>> * ID, with 0 denoting "allow access to non-Landlocked processes?
>>>> */
>>>> }
>>> Yes, it would be possible to use such a struct for that scenario where
>>> IPC access gets allowed for other Landlock scopes. It would mean that
>>> we would not need to introduce a scoped flag for the pathname UNIX
>>> socket connections. But the relationship between that struct
>>> landlock_scope_attr and the flags and access rights in struct
>>> landlock_ruleset_attr would become less clear, which is a slight
>>> downside, and maybe error prone for users to work with.
>>>
>>> If we introduced an additional scoped flag, it would also be
>>> consistent though.
>>>
>>> (con1) was written under the assumption that we do not have an
>>> additional scoped flag. If that is lacking, it is not possible to
>>> express UNIX connect() access to other Landlock domains with that
>>> struct. But as outlined in the proposal below, if we *do* (later)
>>> introduce the additional scoped flag *in addition* to the FS access
>>> right, this *both* stays consistent in semantics with the signal and
>>> abstract UNIX support, *and* it starts working in a world where ICP
>>> access can be allowed to talk to other Landlock domains.
>>>
>>>>> (con2) Consistent behaviour between scoped flags and their
>>>>> interactions with other access rights:
>>>>>
>>>>> The existing scoped access rights (signal, abstract sockets)
>>>>> could hypothetically be extended with a related access right of
>>>>> another type. For instance, there could be an access right type
>>>>>
>>>>> __u64 handled_signal_number;
>>>>>
>>>>> and then you could add a rule to permit the use of certain
>>>>> signal numbers. The interaction between the scoped flags and
>>>>> other access rights should work the same.
>>>>>
>>>>>
>>>>> Constructive Proposal for consideration: Why not both?
>>>>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>> I will think about the following a bit more but I'm afraid that I feel
>>>> like it might get slightly confusing. With this, the only reason for
>>>> having LANDLOCK_SCOPE_PATHNAME_UNIX_SOCKET is for API consistency when we
>>>> later enable allowing access to other domains (if I understood correctly),
>>>> in which case I personally feel like the suggestion on landlock_scope_attr
>>>> above, where we essentially accept that it is decoupled with the scope
>>>> bits in the ruleset, might be simpler...?
>>> Mickaël expressed the opinion to me that he would like to APIs to stay
>>> consistent between signals, abstract UNIX sockets, named UNIX sockets
>>> and other future "scoped" operations, in scenarios where:
>>>
>>> * the "scoped" (IPC) operations can be configured to give access to
>>> other Landlock domains (and that should work for UNIX connections too)
>>> * the existing "scoped" operations also start having matching access rights
>>>
>>> I think with the way I proposed, that would be consistent.
>>>
>>>
>>>>> Why not do both what Tingmao proposed in [1] **and** reserve the
>>>>> option to add the matching "scoped flag" later?
>>>>>
>>>>> * Introduce LANDLOCK_ACCESS_FS_RESOLVE_UNIX.
>>>>>
>>>>> If it is handled, UNIX connections are allowed either:
>>>>>
>>>>> (1) if the connection is to a service in the same scope, or
>>>>> (2) if the path was allow-listed with a "path beneath" rule.
>>>>>
>>>>> * Add LANDLOCK_SCOPE_PATHNAME_UNIX_SOCKET later, if needed.
>>>>>
>>>>>
>>>>> Let's go through the arguments again:
>>>>>
>>>>> We have observed that it is harmless to allow connections to services
>>>>> in the same scope (1), and that if users absolutely don't want that,
>>>>> they can actually prohibit it through LANDLOCK_ACCESS_FS_MAKE_SOCK
>>>>> (pro1).
>>>>>
>>>>> (con1): Can we still implement the feature idea where we poke a hole
>>>>> to get UNIX-connect() access to other Landlock domains?
>>>>>
>>>>> I think the answer is yes. The implementation strategy is:
>>>>>
>>>>> * Add the scoped bit LANDLOCK_SCOPE_PATHNAME_UNIX_SOCKET
>>>>> * The scoped bit can now be used to allow-list connections to
>>>>> other Landlock domains.
>>>>>
>>>>> For users, just setting the scoped bit on its own does the same as
>>>>> handling LANDLOCK_ACCESS_FS_RESOLVE_UNIX. That way, the kernel-side
>>>>> implementation can also stay simple. The only reason why the scoped
>>>>> bit is needed is because it makes it possible to allow-list
>>>>> connections to other Landlock domains, but at the same time, it is
>>>>> safe if libraries set the scoped bit once it exists, as it does not
>>>>> have any bad runtime impact either.
>>>>>
>>>>> (con2): Consistency: Do all the scoped flags interact with their
>>>>> corresponding access rights in the same way?
>>>>>
>>>>> The other scope flags do not have corresponding access rights, so
>>>>> far.
>>>>>
>>>>> If we were to add corresponding access rights for the other scope
>>>>> flags, I would argue that we could apply a consistent logic there,
>>>>> because IPC access within the same scope is always safe:
>>>>>
>>>>> - A hypothetical access right type for "signal numbers" would only
>>>>> restrict signals that go beyond the current scope.
>>>>>
>>>>> - A hypothetical access right type for "abstract UNIX domain socket
>>>>> names" would only restrict connections to abstract UNIX domain
>>>>> servers that go beyond the current scope.
>>>>>
>>>>> I can not come up with a scenario where this doesn't work.
>> Gunther / Tingmao / Mickaël:
>>
>> I have a potential idea to make this concept cleaner.
>>
>> The docs for landlock currently say:
>>
>>
>> IPC scoping does not support exceptions via landlock_add_rule(2).
>> If an operation is scoped within a domain, no rules can be added
>> to allow access to resources or processes outside of the scope.
> This part might indeed be confusing. The idea was to explain the
> difference between scoped rights and handled access rights (which may
> have rules).
>
>> So if we go with the solution where we are now saying IPC scoping DOES support exceptions
>> we will need to update the documentation, to say scoping for pathname unix sockets is an exception,
>> and have to have the "exemptible scopes" (like this one) alongside "non-exemptible" scopes
>> (ie the existing ones). This creates some friction for users.
> The documentation will definitely require some updates. I think it can
> be explained in a simple way.
>
>> If we foresee other "exempt-able scopes" (which are scopes that also support creating exemptions w/ corresponding access rights) in the future, maybe we should consider separating the two in the ruleset
>> attributes (I used scoped_fs as an example for the attribute name):
>>
>> structlandlock_ruleset_attrruleset_attr={
>> .handled_access_fs=
>> LANDLOCK_ACCESS_FS_EXECUTE|
>> LANDLOCK_ACCESS_FS_WRITE_FILE|
>> LANDLOCK_ACCESS_FS_READ_FILE|
>> LANDLOCK_ACCESS_FS_READ_DIR|
>> LANDLOCK_ACCESS_FS_REMOVE_DIR|
>> LANDLOCK_ACCESS_FS_REMOVE_FILE|
>> LANDLOCK_ACCESS_FS_MAKE_CHAR|
>> LANDLOCK_ACCESS_FS_MAKE_DIR|
>> LANDLOCK_ACCESS_FS_MAKE_REG|
>> LANDLOCK_ACCESS_FS_MAKE_SOCK|
>> LANDLOCK_ACCESS_FS_MAKE_FIFO|
>> LANDLOCK_ACCESS_FS_MAKE_BLOCK|
>> LANDLOCK_ACCESS_FS_MAKE_SYM|
>> LANDLOCK_ACCESS_FS_REFER|
>> LANDLOCK_ACCESS_FS_TRUNCATE|
>> LANDLOCK_ACCESS_FS_IOCTL_DEV,
>> .handled_access_net=
>> LANDLOCK_ACCESS_NET_BIND_TCP|
>> LANDLOCK_ACCESS_NET_CONNECT_TCP,
>> .scoped=
>> LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET|
>> LANDLOCK_SCOPE_SIGNAL,
>> .scoped_fs =
>> LANDLOCK_SCOPE_FS_PATHNAME_UNIX_SOCKET
>> };
>>
>> This more clearly distinguishes between scopes that have exceptions/corresponding fs rights,
>> and ones that don't. Later we could add scoped_net, if needed. I feel like this would be more
>> intuitive and better categorize future scoping rights. An obvious con is increasing the size of
>> the ruleset attributes.
> I see your point but I don't think it would be worth it to add
> sub-scoped fields. Each field has a clear semantic, and the scoped one
> is related to the domain wrt other domains.
As long as it's documented clearly, and future IPCs have similar behavior
I agree that a separate field probably isn't needed.
>> Of course this separation is only worth it if there are other "exempt-able" rights in the future.
>> I can think of a few potential future rights which COULD be scoped and have corresponding rights
>> (binder, sysv-ipc, pipes, tcp/udp between two local programs).
> Yes, it would definitely be useful to add exception for other kind of
> IPCs. The idea would be to be able to describe the peer, either with a
> file path, or PID, or cgroups, or a Landlock domain... The inet case
> is an interesting idea but that might be a challenging task to
> implement, if even possible.
>>>>> In conclusion, I think the approach has significant upsides:
>>>>>
>>>>> * Simpler UAPI: Users only have one access bit to deal with, in the
>>>>> near future. Once we do add a scope flag for UNIX connections, it
>>>>> does not interact in a surprising way with the corresponding FS
>>>>> access right, because with either of these, scoped access is
>>>>> allowed.
>>>>>
>>>>> If users absolutely need to restrict scoped access, they can
>>>>> restrict LANDLOCK_ACCESS_FS_MAKE_SOCK. It is a slightly obscure
>>>>> API, but in line with the "make easy things easy, make hard things
>>>>> possible" API philosophy. And needing this should be the
>>>>> exception rather than the norm, after all.
>>>>>
>>>>> * Consistent behaviour between scoped flags and regular access
>>>>> rights, also for speculative access rights affecting the existing
>>>>> scoped flags for signals and abstract UNIX domain sockets.
>>>>>
>>>>> [1] https://lore.kernel.org/all/f07fe41a-96c5-4d3a-9966-35b30b3a71f1@maowtm.org/
>>> —Günther
^ permalink raw reply
* Re: [PATCH v3 0/3] Landlock multithreaded enforcement
From: Mickaël Salaün @ 2026-02-05 18:53 UTC (permalink / raw)
To: Günther Noack
Cc: linux-security-module, Jann Horn, Serge Hallyn,
Konstantin Meskhidze, Tingmao Wang, Matthieu Buffet
In-Reply-To: <20251127115136.3064948-1-gnoack@google.com>
Good job for writing this complex mechanic (and the related doc), this
patch series is great! It's been in linux-next for a few weeks and I'll
take it for Linux 7.0
I did some cosmetic changes though, you'll find them in my commits.
Some more tests are needed but I'll take this series for now.
Thanks!
On Thu, Nov 27, 2025 at 12:51:33PM +0100, Günther Noack wrote:
> This patch set adds the LANDLOCK_RESTRICT_SELF_TSYNC flag to
> landlock_restrict_self(). With this flag, the passed Landlock ruleset
> will not only be applied to the calling thread, but to all threads
> which belong to the same process.
>
> Motivation
> ==========
>
> TL;DR: The libpsx/nptl(7) signal hack which we use in user space for
> multi-threaded Landlock enforcement is incompatible with Landlock's
> signal scoping support. Landlock can restrict the use of signals
> across Landlock domains, but we need signals ourselves in user space
> in ways that are not permitted any more under these restrictions.
>
> Enabling Landlock proves to be difficult in processes that are already
> multi-threaded at the time of enforcement:
>
> * Enforcement in only one thread is usually a mistake because threads
> do not normally have proper security boundaries between them.
>
> * Also, multithreading is unavoidable in some circumstances, such as
> when using Landlock from a Go program. Go programs are already
> multithreaded by the time that they enter the "func main()".
>
> So far, the approach in Go[1] was to use libpsx[2]. This library
> implements the mechanism described in nptl(7) [3]: It keeps track of
> all threads with a linker hack and then makes all threads do the same
> syscall by registering a signal handler for them and invoking it.
>
> With commit 54a6e6bbf3be ("landlock: Add signal scoping"), Landlock
> gained the ability to restrict the use of signals across different
> Landlock domains.
>
> Landlock's signal scoping support is incompatible with the libpsx
> approach of enabling Landlock:
>
> (1) With libpsx, although all threads enforce the same ruleset object,
> they technically do the operation separately and end up in
> distinct Landlock domains. This breaks signaling across threads
> when using LANDLOCK_SCOPE_SIGNAL.
>
> (2) Cross-thread Signals are themselves needed to enforce further
> nested Landlock domains across multiple threads. So nested
> Landlock policies become impossible there.
>
> In addition to Landlock itself, cross-thread signals are also needed
> for other seemingly-harmless API calls like the setuid(2) [4] and for
> the use of libcap (co-developed with libpsx), which have the same
> problem where the underlying syscall only applies to the calling
> thread.
>
> Implementation details
> ======================
>
> Enforcement prerequisites
> -------------------------
>
> Normally, the prerequisite for enforcing a Landlock policy is to
> either have CAP_SYS_ADMIN or the no_new_privs flag. With
> LANDLOCK_RESTRICT_SELF_TSYNC, the no_new_privs flag will automatically
> be applied for sibling threads if the caller had it.
>
> These prerequisites and the "TSYNC" behavior work the same as for
> Seccomp and its SECCOMP_FILTER_FLAG_TSYNC flag.
>
> Pseudo-signals
> --------------
>
> Landlock domains are stored in struct cred, and a task's struct cred
> can only be modified by the task itself [6].
>
> To make that work, we use task_work_add() to register a pseudo-signal
> for each of the affected threads. At signal execution time, these
> tasks will coordinate to switch out their Landlock policy in lockstep
> with each other, guaranteeing all-or-nothing semantics.
>
> This implementation can be thought of as a kernel-side implementation
> of the userspace hack that glibc/NPTL use for setuid(2) [3] [4], and
> which libpsx implements for libcap [2].
>
> Finding all sibling threads
> ---------------------------
>
> In order to avoid grabbing the global task_list_lock, we employ the
> scheme proposed by Jann Horn in [7]:
>
> 1. Loop through the list of sibling threads
> 2. Schedule a pseudo-signal for each and make each thread wait in the
> pseudo-signal
> 3. Go back to 1. and look for more sibling thread that we have not
> seen yet
>
> Do this until no more new threads are found. As all threads were
> waiting in their pseudo-signals, they can not spawn additional threads
> and we found them all.
>
> Coordination between tasks
> --------------------------
>
> As tasks run their pseudo-signal task work, they coordinate through
> the following completions:
>
> - all_prepared (with counter num_preparing)
>
> When done, all new sibling threads in the inner loop(!) of finding
> new threads are now in their pseudo-signal handlers and have
> prepared the struct cred object to commit (or written an error into
> the shared "preparation_error").
>
> The lifetime of all_prepared is only the inner loop of finding new
> threads.
>
> - ready_to_commit
>
> When done, the outer loop of finding new threads is done and all
> sibling threads have prepared their struct cred object. Marked
> completed by the calling thread.
>
> - all_finished
>
> When done, all sibling threads are done executing their
> pseudo-signal handlers.
>
> Use of credentials API
> ----------------------
>
> Under normal circumstances, sibling threads share the same struct cred
> object. To avoid unnecessary duplication, if we find that a thread
> uses the same struct cred as the calling thread, we side-step the
> normal use of the credentials API [6] and place a pointer to that
> existing struct cred instead of creating a new one using
> prepare_creds() in the sibling thread.
>
> Noteworthy discussion points
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> * We are side-stepping the normal credentials API [6], by re-wiring an
> existing struct cred object instead of calling prepare_creds().
>
> We can technically avoid it, but it would create unnecessary
> duplicate struct cred objects in multithreaded scenarios.
>
> Change Log
> ==========
>
> v3:
> - bigger organizational changes
> - move tsync logic into own file
> - tsync: extract count_additional_threads() and
> schedule_task_work()
> - code style
> - restrict_one_thread, syscalls.c: use err instead of res (mic)
> - restrict_one_thread: inline current_cred variable
> - restrict_one_thread: add comment to shortcut logic (mic)
> - rsync_works helpers: use size_t i for loop vars
> - landlock_cred_copy: skip redundant NULL checks
> - function name: s,tsync_works_free,tsync_works_release, (mic)
> - tsync_works_grow_by: kzalloc into a temporary variable for
> clarity (mic)
> - tsync_works_contains_task: make struct task_works const
> - bugs
> - handle kmalloc family failures correctly (jannh)
> - tsync_works_release: check task NULL ptr before put
> - s/put_task_struct_rcu_user/put_task_struct/ (jannh)
> - concurrency bugs
> - schedule_task_work: do not return error when encountering exiting
> tasks This can happen during normal operation, we should not
> error due to it (jannh)
> - landlock_restrict_sibling_threads: make current hold the
> num_unfinished/all_finished barrier (more robust, jannh)
> - un-wedge the deadlock using wait_for_completion_interruptible
> (jannh) See "testing" below and discussion in
> https://lore.kernel.org/all/CAG48ez1oS9kANZBq1bt+D76MX03DPHAFp76GJt7z5yx-Na1VLQ@mail.gmail.com/
> - logic
> - tsync_works_grow_by(): grow to size+n, not capacity+n
> - tsync_works_grow_by(): add overflow check for capacity increase
> - landlock_restrict_self(): make TSYNC and LOG flags work together
> - set no_new_privs in the same way as seccomp,
> whenever the calling thread had it
> - testing
> - add test where multiple threads call landlock_restrict_self()
> concurrently
> - test that no_new_privs is implicitly enabled for sibling threads
> - bump ABI version to v8
> - documentation improvements
> - document ABI v8
> - move flag documentation into the landlock.h header
> - comment: Explain why we do not need sighand->siglock or
> cred_guard_mutex
> - various comment improvements
> - reminder above struct landlock_cred_security about updating
> landlock_cred_copy on changes
>
> v2:
> - https://lore.kernel.org/all/20250221184417.27954-2-gnoack3000@gmail.com/
> - Semantics:
> - Threads implicitly set NO_NEW_PRIVS unless they have
> CAP_SYS_ADMIN, to fulfill Landlock policy enforcement
> prerequisites
> - Landlock policy gets unconditionally overridden even if the
> previously established Landlock domains in sibling threads were
> diverging.
> - Restructure discovery of all sibling threads, with the algorithm
> proposed by Jann Horn [7]: Loop through threads multiple times, and
> get them all stuck in the pseudo signal (task work), until no new
> sibling threads show up.
> - Use RCU lock when iterating over sibling threads.
> - Override existing Landlock domains of other threads,
> instead of applying a new Landlock policy on top
> - Directly re-wire the struct cred for sibling threads,
> instread of creating a new one with prepare_creds().
> - Tests:
> - Remove multi_threaded_failure test
> (The only remaining failure case is ENOMEM,
> there is no good way to provoke that in a selftest)
> - Add test for success despite diverging Landlock domains.
>
> [1] https://github.com/landlock-lsm/go-landlock
> [2] https://sites.google.com/site/fullycapable/who-ordered-libpsx
> [3] https://man.gnoack.org/7/nptl
> [4] https://man.gnoack.org/2/setuid#VERSIONS
> [5] https://lore.kernel.org/all/20240805-remove-cred-transfer-v2-0-a2aa1d45e6b8@google.com/
> [6] https://www.kernel.org/doc/html/latest/security/credentials.html
> [7] https://lore.kernel.org/all/CAG48ez0pWg3OTABfCKRk5sWrURM-HdJhQMcWedEppc_z1rrVJw@mail.gmail.com/
>
> Günther Noack (3):
> landlock: Multithreading support for landlock_restrict_self()
> landlock: selftests for LANDLOCK_RESTRICT_SELF_TSYNC
> landlock: Document LANDLOCK_RESTRICT_SELF_TSYNC
>
> Documentation/userspace-api/landlock.rst | 8 +
> include/uapi/linux/landlock.h | 13 +
> security/landlock/Makefile | 2 +-
> security/landlock/cred.h | 12 +
> security/landlock/limits.h | 2 +-
> security/landlock/syscalls.c | 66 ++-
> security/landlock/tsync.c | 555 ++++++++++++++++++
> security/landlock/tsync.h | 16 +
> tools/testing/selftests/landlock/base_test.c | 8 +-
> tools/testing/selftests/landlock/tsync_test.c | 161 +++++
> 10 files changed, 810 insertions(+), 33 deletions(-)
> create mode 100644 security/landlock/tsync.c
> create mode 100644 security/landlock/tsync.h
> create mode 100644 tools/testing/selftests/landlock/tsync_test.c
>
> --
> 2.52.0.177.g9f829587af-goog
>
>
^ permalink raw reply
* Re: [PATCH v3 2/3] landlock: selftests for LANDLOCK_RESTRICT_SELF_TSYNC
From: Mickaël Salaün @ 2026-02-05 18:54 UTC (permalink / raw)
To: Günther Noack
Cc: linux-security-module, Jann Horn, Serge Hallyn,
Konstantin Meskhidze, Tingmao Wang, Andrew G. Morgan,
John Johansen, Paul Moore
In-Reply-To: <20251127115136.3064948-3-gnoack@google.com>
These tests are useful but there are two missing parts:
- testing the restriction of each thread;
- testing the domain ID of each thread.
I'll merge this whole series but could you please extend this test file
with a new patch?
On Thu, Nov 27, 2025 at 12:51:35PM +0100, Günther Noack wrote:
> Exercise various scenarios where Landlock domains are enforced across
> all of a processes' threads.
>
> Cc: Andrew G. Morgan <morgan@kernel.org>
> Cc: John Johansen <john.johansen@canonical.com>
> Cc: Mickaël Salaün <mic@digikod.net>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: linux-security-module@vger.kernel.org
> Signed-off-by: Günther Noack <gnoack@google.com>
> ---
> tools/testing/selftests/landlock/base_test.c | 6 +-
> tools/testing/selftests/landlock/tsync_test.c | 161 ++++++++++++++++++
> 2 files changed, 164 insertions(+), 3 deletions(-)
> create mode 100644 tools/testing/selftests/landlock/tsync_test.c
>
> diff --git a/tools/testing/selftests/landlock/base_test.c b/tools/testing/selftests/landlock/base_test.c
> index f4b1a275d8d9..0fea236ef4bd 100644
> --- a/tools/testing/selftests/landlock/base_test.c
> +++ b/tools/testing/selftests/landlock/base_test.c
> @@ -288,7 +288,7 @@ TEST(restrict_self_fd)
> EXPECT_EQ(EBADFD, errno);
> }
>
> -TEST(restrict_self_fd_flags)
> +TEST(restrict_self_fd_logging_flags)
> {
> int fd;
>
> @@ -304,9 +304,9 @@ TEST(restrict_self_fd_flags)
> EXPECT_EQ(EBADFD, errno);
> }
>
> -TEST(restrict_self_flags)
> +TEST(restrict_self_logging_flags)
> {
> - const __u32 last_flag = LANDLOCK_RESTRICT_SELF_LOG_SUBDOMAINS_OFF;
> + const __u32 last_flag = LANDLOCK_RESTRICT_SELF_TSYNC;
>
> /* Tests invalid flag combinations. */
>
> diff --git a/tools/testing/selftests/landlock/tsync_test.c b/tools/testing/selftests/landlock/tsync_test.c
> new file mode 100644
> index 000000000000..3971e0f02c49
> --- /dev/null
> +++ b/tools/testing/selftests/landlock/tsync_test.c
> @@ -0,0 +1,161 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Landlock tests - Enforcing the same restrictions across multiple threads
> + *
> + * Copyright © 2025 Günther Noack <gnoack3000@gmail.com>
> + */
> +
> +#define _GNU_SOURCE
> +#include <pthread.h>
> +#include <sys/prctl.h>
> +#include <linux/landlock.h>
> +
> +#include "common.h"
> +
> +/* create_ruleset - Create a simple ruleset FD common to all tests */
> +static int create_ruleset(struct __test_metadata *const _metadata)
> +{
> + struct landlock_ruleset_attr ruleset_attr = {
> + .handled_access_fs = (LANDLOCK_ACCESS_FS_WRITE_FILE |
> + LANDLOCK_ACCESS_FS_TRUNCATE),
> + };
> + const int ruleset_fd =
> + landlock_create_ruleset(&ruleset_attr, sizeof(ruleset_attr), 0);
> +
> + ASSERT_LE(0, ruleset_fd)
> + {
> + TH_LOG("landlock_create_ruleset: %s", strerror(errno));
> + }
> + return ruleset_fd;
> +}
> +
> +TEST(single_threaded_success)
> +{
> + const int ruleset_fd = create_ruleset(_metadata);
> +
> + disable_caps(_metadata);
> +
> + ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0));
> + ASSERT_EQ(0, landlock_restrict_self(ruleset_fd,
> + LANDLOCK_RESTRICT_SELF_TSYNC));
> +
> + ASSERT_EQ(0, close(ruleset_fd));
> +}
> +
> +void store_no_new_privs(void *data)
I fixed the helpers to make them static.
> +{
> + bool *nnp = data;
> +
> + if (!nnp)
> + return;
> + *nnp = prctl(PR_GET_NO_NEW_PRIVS, 0, 0, 0, 0);
> +}
> +
> +void *idle(void *data)
> +{
> + pthread_cleanup_push(store_no_new_privs, data);
> +
> + while (true)
> + sleep(1);
> +
> + pthread_cleanup_pop(1);
> +}
> +
> +TEST(multi_threaded_success)
> +{
> + pthread_t t1, t2;
> + bool no_new_privs1, no_new_privs2;
> + const int ruleset_fd = create_ruleset(_metadata);
> +
> + disable_caps(_metadata);
> +
> + ASSERT_EQ(0, pthread_create(&t1, NULL, idle, &no_new_privs1));
> + ASSERT_EQ(0, pthread_create(&t2, NULL, idle, &no_new_privs2));
> +
> + ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0));
> +
> + EXPECT_EQ(0, landlock_restrict_self(ruleset_fd,
> + LANDLOCK_RESTRICT_SELF_TSYNC));
> +
> + ASSERT_EQ(0, pthread_cancel(t1));
> + ASSERT_EQ(0, pthread_cancel(t2));
> + ASSERT_EQ(0, pthread_join(t1, NULL));
> + ASSERT_EQ(0, pthread_join(t2, NULL));
> +
> + /* The no_new_privs flag was implicitly enabled on all threads. */
> + EXPECT_TRUE(no_new_privs1);
> + EXPECT_TRUE(no_new_privs2);
> +
> + ASSERT_EQ(0, close(ruleset_fd));
> +}
> +
> +TEST(multi_threaded_success_despite_diverging_domains)
> +{
> + pthread_t t1, t2;
> + const int ruleset_fd = create_ruleset(_metadata);
> +
> + disable_caps(_metadata);
> +
> + ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0));
> +
> + ASSERT_EQ(0, pthread_create(&t1, NULL, idle, NULL));
> + ASSERT_EQ(0, pthread_create(&t2, NULL, idle, NULL));
> +
> + /*
> + * The main thread enforces a ruleset,
> + * thereby bringing the threads' Landlock domains out of sync.
> + */
> + EXPECT_EQ(0, landlock_restrict_self(ruleset_fd, 0));
> +
> + /* Still, TSYNC succeeds, bringing the threads in sync again. */
> + EXPECT_EQ(0, landlock_restrict_self(ruleset_fd,
> + LANDLOCK_RESTRICT_SELF_TSYNC));
> +
> + ASSERT_EQ(0, pthread_cancel(t1));
> + ASSERT_EQ(0, pthread_cancel(t2));
> + ASSERT_EQ(0, pthread_join(t1, NULL));
> + ASSERT_EQ(0, pthread_join(t2, NULL));
> + ASSERT_EQ(0, close(ruleset_fd));
> +}
> +
> +struct thread_restrict_data {
> + pthread_t t;
> + int ruleset_fd;
> + int result;
> +};
> +
> +void *thread_restrict(void *data)
> +{
> + struct thread_restrict_data *d = data;
> +
> + d->result = landlock_restrict_self(d->ruleset_fd,
> + LANDLOCK_RESTRICT_SELF_TSYNC);
> + return NULL;
> +}
> +
> +TEST(competing_enablement)
> +{
> + const int ruleset_fd = create_ruleset(_metadata);
> + struct thread_restrict_data d[] = {
> + { .ruleset_fd = ruleset_fd },
> + { .ruleset_fd = ruleset_fd },
> + };
> +
> + disable_caps(_metadata);
> +
> + ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0));
> + ASSERT_EQ(0, pthread_create(&d[0].t, NULL, thread_restrict, &d[0]));
> + ASSERT_EQ(0, pthread_create(&d[1].t, NULL, thread_restrict, &d[1]));
> +
> + /* Wait for threads to finish. */
> + ASSERT_EQ(0, pthread_join(d[0].t, NULL));
> + ASSERT_EQ(0, pthread_join(d[1].t, NULL));
> +
> + /* Expect that both succeeded. */
> + EXPECT_EQ(0, d[0].result);
> + EXPECT_EQ(0, d[1].result);
> +
> + ASSERT_EQ(0, close(ruleset_fd));
> +}
> +
> +TEST_HARNESS_MAIN
> --
> 2.52.0.177.g9f829587af-goog
>
>
^ permalink raw reply
* Re: [PATCH v2 0/6] Landlock: Implement scope control for pathname Unix sockets
From: Mickaël Salaün @ 2026-02-05 19:15 UTC (permalink / raw)
To: Justin Suess
Cc: Günther Noack, Tingmao Wang, Günther Noack,
Demi Marie Obenour, Alyssa Ross, Jann Horn, Tahera Fahimi,
linux-security-module, Matthieu Buffet
In-Reply-To: <44d216aa-9680-4cf5-bbf0-173869111212@gmail.com>
On Thu, Feb 05, 2026 at 10:18:54AM -0500, Justin Suess wrote:
>
> On 2/4/26 13:28, Mickaël Salaün wrote:
>
> >> [...]
> >> Tingmao:
> >>
> >> For connecting a pathname unix socket, the order of the hooks landlock sees is something like:
> >>
> >> 1. security_unix_find. (to look up the paths)
> >>
> >> 2. security_unix_may_send, security_unix_stream_connect (after the path is looked up)
> >>
> >> Which for is called in DGRAM:
> >>
> >> unix_dgram_connect OR unix_dgram_sendmsg
> >>
> >> and for STREAM:
> >>
> >> unix_stream_connect
> >>
> >> IIRC, the path lookup only occurs in this order always, so in the logic as you have it the domain_is_scoped()
> >> would be called twice, once from the security_unix_find when you call it in step two, and once from the
> >> domain scope hooks. (If access was allowed from security_unix_find)
> >>
> >> There are a couple of things to consider.
> >>
> >> ---
> >>
> >> Audit blockers need special handling:
> >>
> >> Here's an example:
> >>
> >> 1. Program A enforces a ruleset with RESOLVE_UNIX and the unix pathname scope bit, with no rules with that
> >> access bit (deny all for RESOLVE_UNIX).
> >>
> >> 2. Program A connects to /tmp/mysock.sock ran by program B, which is outside the domain.
> >>
> >> 2. security_unix_find is hit to lookup the path, and the RESOLVE_UNIX code doesn't grant access to
> >> /tmp/mysock.sock, so it calls domain_is_scoped
> >>
> >> 3. domain_is_scoped denies it as well, so now we must log an audit record.
> >>
> >> When logging the denial, we have to include both blockers "scope.unix_socket" and "fs.resolve_unix" for the
> >> denial, because it is the absence of both that caused the denial. I think the refer right has similar cases for auditing, so there is precedent for this (multiple blockers for an audit message).
> > That's a good point, and it would give more informations to diagnose
> > issues. However, being able to identify if both accesses are denied
> > would require to check both, whereas the first is enough to know that
> > Landlock denies the access. So, if we can return both records without
> > continuing the security checks, that's good, otherwise we should stop
> > ASAP and return the error.
> Maybe I'm missing something, but if the flags interact in an "OR" manner
> wouldn't we need to check both?
Yes, but my point is that as soon as one or the other *denies* an
access, there is no need to check the other access type.
> In your proposal where RESOLVE_UNIX
> implies the scoped flag, if a program connects to a unix socket that is within
> the domain but does not have an explicit RESOLVE_UNIX exception, we must
> still check for the case that the access is scoped.
>
> ---
>
> (Given LANDLOCK_ACCESS_FS_RESOLVE_UNIX and LANDLOCK_SCOPE_PATHNAME_UNIX_SOCKET
> are set)
>
> case 1: access to socket within domain and no RESOLVE_UNIX rule covers the access
>
> We check first in security_unix_find hook and find there is no rule allowing the access.
> After the check fails, because LANDLOCK_SCOPE_PATHNAME_UNIX_SOCKET is set
> we then check is_domain_scoped, the check allows the access.
>
> case 2: access to socket outside domain but RESOLVE_UNIX rule covers the access
>
> We check first in security_unix_find hook and find there is a rule allowing the access.
> We can allow the access early (short-circuit eval) without calling is_domain_scoped.
>
> case 4: access to socket inside domain and RESOLVE_UNIX covers the access
>
> We check first in security_unix_find hook and find there is a rule allowing the access.
> We can allow the access early (short-circuit eval) without calling is_domain_scoped. (same as case 2)
>
>
> case 4: access to socket outside domain and no RESOLVE_UNIX covers the access
>
> We check first in security_unix_find hook and find there is no rule allowing the access.
> After the check fails, because LANDLOCK_SCOPE_PATHNAME_UNIX_SOCKET is set
> we then check is_domain_scoped, the check does not allow the access. (it is the combination
> of the two checks failing that denied the access).
BTW, we should first check is_domain_scoped() because it is quicker than
the FS checks.
>
> ---
>
> Case 4 is what I'm specifically considering would need to have both blockers listed in a denial audit.
> We can't short circuit in that case because we have to check the scoping before denying.
> Let me know if I'm misunderstanding this.
Indeed, if both deny the request, both should be listed.
Anyway, we're now going to only have one access flag that would merge
both semantic, so this should not be an issue.
However, this specific case will be relevant when we'll add e.g., a
signal attr that would then be complementary to the signal scope. At
this point, I think it would be enough to only log the signal.* record
because it would be a superset of the scope.signal
>
> (PS: IIRC the hooks used by the LANDLOCK_SCOPE_PATHNAME_UNIX_SOCKET
> will never be hit if the check in security_unix_find fails. So some logic to check this
> access in security_unix_find will be needed).
>
> >
> > Anyway, that might not be needed if we end up with my latest proposal
> > about always setting scope.unix_socket when fs.resolve_unix is set.
> >
> >> ---
> >>
> >> Dual lookup for domain_is_scoped. Consider this case:
> >>
> >> 1. Program A enforces a ruleset with RESOLVE_UNIX and the unix pathname scope bit, with no rules with that
> >> access bit (deny all for RESOLVE_UNIX).
> >>
> >> 2. Program A connects to Program C's /tmp/foo.sock, which for the purposes of this example is in the domain of program A.
> >>
> >> 3. security_unix_find is hit to lookup the path, and the RESOLVE_UNIX code doesn't grant access to
> >> /tmp/mysock.sock, so it calls domain_is_scoped. Access is granted, and continues. (LSM hook complete)
> >>
> >> 4. The connection proceeds past the path lookup stage, and now security_unix_may_send, or security_unix_stream_connect gets called. This requires ANOTHER domain_is_scoped access check.
> >>
> >> While I don't THINK this introduces a TOCTOU, it is a little confusing.
> >>
> >> This does mean that we look up the domain twice, if this is implemented naively. I think we can then just
> >> skip the task credential checks then for security_unix_may_send and security_unix_stream_connect **for
> >> connecting to pathname sockets**, since the domain_is_scoped will already have been called in landlock's
> >> security_unix_find hook, eliminating the need for handling pathname socket domain checks layer on.
> >>
> >>>> I definitely agree that it is tricky, but making same-scope access be
> >>>> allowed (i.e. the suggested idea above) would only get rid of step 1,
> >>>> which I think is the "simpler" bit. The extra logic in step 2 is still
> >>>> needed.
> >>>>
> >>>> I definitely agree with pro1 tho.
> >>> Yes, you are describing the logic for the variant where
> >>> LANDLOCK_ACCESS_FS_RESOLVE_UNIX does not implicitly permit access from
> >>> within the same scope. In that variant, there can be situations where
> >>> the first hook can deny the action immediately.
> >>>
> >>> In the variant where LANDLOCK_ACCESS_FS_RESOLVE_UNIX *does* implicitly
> >>> allow access from within the same scope, that shortcutting is not
> >>> possible. On the upside however, there is no need to distinguish
> >>> whether the scope flag is set when we are in the security_unix_find()
> >>> hook, because access from within the same scope is always permitted.
> >>> (That is the simplification I meant.)
> >>>
> >>>
> >>>>> AGAINST:
> >>>>>
> >>>>> (con1) It would work differently than the other scoped access rights
> >>>>> that we already have.
> >>>>>
> >>>>> A speculative feature that could potentially be built with the
> >>>>> scoped access rights is that we could add a rule to permit IPC
> >>>>> to other Landlock scopes, e.g. introducing a new rule type
> >>>>>
> >>>>> struct landlock_scope_attr {
> >>>>> __u64 allowed_access; /* for "scoped" bits */
> >>>>> /* some way to identify domains */
> >>>>> }
> >>>>>
> >>>>> so that we could make IPC access to other Landlock domains
> >>>>> configurable.
> >>>>>
> >>>>> If the scoped bit and the FS RESOLVE_UNIX bit were both
> >>>>> conflated in RESOLVE_UNIX, it would not be possible to make
> >>>>> UNIX connections configurable in such a way.
> >>>> This exact API would no longer work, but if we give up the equivalence
> >>>> between scope bits and the landlock_scope_attr struct, then we can do
> >>>> something like:
> >>>>
> >>>> struct landlock_scope_attr {
> >>>> __u64 ptrace:1; /* Note that this is not a (user controllable) scope bit! */
> >>>> __u64 abstract_unix_socket:1;
> >>>> __u64 pathname_unix_socket:1;
> >>>> /* ... */
> >>>>
> >>>> __u64 allowed_signals;
> >>>>
> >>>> /*
> >>>> * some way to identify domains, maybe we could use the audit domain
> >>>> * ID, with 0 denoting "allow access to non-Landlocked processes?
> >>>> */
> >>>> }
> >>> Yes, it would be possible to use such a struct for that scenario where
> >>> IPC access gets allowed for other Landlock scopes. It would mean that
> >>> we would not need to introduce a scoped flag for the pathname UNIX
> >>> socket connections. But the relationship between that struct
> >>> landlock_scope_attr and the flags and access rights in struct
> >>> landlock_ruleset_attr would become less clear, which is a slight
> >>> downside, and maybe error prone for users to work with.
> >>>
> >>> If we introduced an additional scoped flag, it would also be
> >>> consistent though.
> >>>
> >>> (con1) was written under the assumption that we do not have an
> >>> additional scoped flag. If that is lacking, it is not possible to
> >>> express UNIX connect() access to other Landlock domains with that
> >>> struct. But as outlined in the proposal below, if we *do* (later)
> >>> introduce the additional scoped flag *in addition* to the FS access
> >>> right, this *both* stays consistent in semantics with the signal and
> >>> abstract UNIX support, *and* it starts working in a world where ICP
> >>> access can be allowed to talk to other Landlock domains.
> >>>
> >>>>> (con2) Consistent behaviour between scoped flags and their
> >>>>> interactions with other access rights:
> >>>>>
> >>>>> The existing scoped access rights (signal, abstract sockets)
> >>>>> could hypothetically be extended with a related access right of
> >>>>> another type. For instance, there could be an access right type
> >>>>>
> >>>>> __u64 handled_signal_number;
> >>>>>
> >>>>> and then you could add a rule to permit the use of certain
> >>>>> signal numbers. The interaction between the scoped flags and
> >>>>> other access rights should work the same.
> >>>>>
> >>>>>
> >>>>> Constructive Proposal for consideration: Why not both?
> >>>>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>>> I will think about the following a bit more but I'm afraid that I feel
> >>>> like it might get slightly confusing. With this, the only reason for
> >>>> having LANDLOCK_SCOPE_PATHNAME_UNIX_SOCKET is for API consistency when we
> >>>> later enable allowing access to other domains (if I understood correctly),
> >>>> in which case I personally feel like the suggestion on landlock_scope_attr
> >>>> above, where we essentially accept that it is decoupled with the scope
> >>>> bits in the ruleset, might be simpler...?
> >>> Mickaël expressed the opinion to me that he would like to APIs to stay
> >>> consistent between signals, abstract UNIX sockets, named UNIX sockets
> >>> and other future "scoped" operations, in scenarios where:
> >>>
> >>> * the "scoped" (IPC) operations can be configured to give access to
> >>> other Landlock domains (and that should work for UNIX connections too)
> >>> * the existing "scoped" operations also start having matching access rights
> >>>
> >>> I think with the way I proposed, that would be consistent.
> >>>
> >>>
> >>>>> Why not do both what Tingmao proposed in [1] **and** reserve the
> >>>>> option to add the matching "scoped flag" later?
> >>>>>
> >>>>> * Introduce LANDLOCK_ACCESS_FS_RESOLVE_UNIX.
> >>>>>
> >>>>> If it is handled, UNIX connections are allowed either:
> >>>>>
> >>>>> (1) if the connection is to a service in the same scope, or
> >>>>> (2) if the path was allow-listed with a "path beneath" rule.
> >>>>>
> >>>>> * Add LANDLOCK_SCOPE_PATHNAME_UNIX_SOCKET later, if needed.
> >>>>>
> >>>>>
> >>>>> Let's go through the arguments again:
> >>>>>
> >>>>> We have observed that it is harmless to allow connections to services
> >>>>> in the same scope (1), and that if users absolutely don't want that,
> >>>>> they can actually prohibit it through LANDLOCK_ACCESS_FS_MAKE_SOCK
> >>>>> (pro1).
> >>>>>
> >>>>> (con1): Can we still implement the feature idea where we poke a hole
> >>>>> to get UNIX-connect() access to other Landlock domains?
> >>>>>
> >>>>> I think the answer is yes. The implementation strategy is:
> >>>>>
> >>>>> * Add the scoped bit LANDLOCK_SCOPE_PATHNAME_UNIX_SOCKET
> >>>>> * The scoped bit can now be used to allow-list connections to
> >>>>> other Landlock domains.
> >>>>>
> >>>>> For users, just setting the scoped bit on its own does the same as
> >>>>> handling LANDLOCK_ACCESS_FS_RESOLVE_UNIX. That way, the kernel-side
> >>>>> implementation can also stay simple. The only reason why the scoped
> >>>>> bit is needed is because it makes it possible to allow-list
> >>>>> connections to other Landlock domains, but at the same time, it is
> >>>>> safe if libraries set the scoped bit once it exists, as it does not
> >>>>> have any bad runtime impact either.
> >>>>>
> >>>>> (con2): Consistency: Do all the scoped flags interact with their
> >>>>> corresponding access rights in the same way?
> >>>>>
> >>>>> The other scope flags do not have corresponding access rights, so
> >>>>> far.
> >>>>>
> >>>>> If we were to add corresponding access rights for the other scope
> >>>>> flags, I would argue that we could apply a consistent logic there,
> >>>>> because IPC access within the same scope is always safe:
> >>>>>
> >>>>> - A hypothetical access right type for "signal numbers" would only
> >>>>> restrict signals that go beyond the current scope.
> >>>>>
> >>>>> - A hypothetical access right type for "abstract UNIX domain socket
> >>>>> names" would only restrict connections to abstract UNIX domain
> >>>>> servers that go beyond the current scope.
> >>>>>
> >>>>> I can not come up with a scenario where this doesn't work.
> >> Gunther / Tingmao / Mickaël:
> >>
> >> I have a potential idea to make this concept cleaner.
> >>
> >> The docs for landlock currently say:
> >>
> >>
> >> IPC scoping does not support exceptions via landlock_add_rule(2).
> >> If an operation is scoped within a domain, no rules can be added
> >> to allow access to resources or processes outside of the scope.
> > This part might indeed be confusing. The idea was to explain the
> > difference between scoped rights and handled access rights (which may
> > have rules).
> >
> >> So if we go with the solution where we are now saying IPC scoping DOES support exceptions
> >> we will need to update the documentation, to say scoping for pathname unix sockets is an exception,
> >> and have to have the "exemptible scopes" (like this one) alongside "non-exemptible" scopes
> >> (ie the existing ones). This creates some friction for users.
> > The documentation will definitely require some updates. I think it can
> > be explained in a simple way.
> >
> >> If we foresee other "exempt-able scopes" (which are scopes that also support creating exemptions w/ corresponding access rights) in the future, maybe we should consider separating the two in the ruleset
> >> attributes (I used scoped_fs as an example for the attribute name):
> >>
> >> structlandlock_ruleset_attrruleset_attr={
> >> .handled_access_fs=
> >> LANDLOCK_ACCESS_FS_EXECUTE|
> >> LANDLOCK_ACCESS_FS_WRITE_FILE|
> >> LANDLOCK_ACCESS_FS_READ_FILE|
> >> LANDLOCK_ACCESS_FS_READ_DIR|
> >> LANDLOCK_ACCESS_FS_REMOVE_DIR|
> >> LANDLOCK_ACCESS_FS_REMOVE_FILE|
> >> LANDLOCK_ACCESS_FS_MAKE_CHAR|
> >> LANDLOCK_ACCESS_FS_MAKE_DIR|
> >> LANDLOCK_ACCESS_FS_MAKE_REG|
> >> LANDLOCK_ACCESS_FS_MAKE_SOCK|
> >> LANDLOCK_ACCESS_FS_MAKE_FIFO|
> >> LANDLOCK_ACCESS_FS_MAKE_BLOCK|
> >> LANDLOCK_ACCESS_FS_MAKE_SYM|
> >> LANDLOCK_ACCESS_FS_REFER|
> >> LANDLOCK_ACCESS_FS_TRUNCATE|
> >> LANDLOCK_ACCESS_FS_IOCTL_DEV,
> >> .handled_access_net=
> >> LANDLOCK_ACCESS_NET_BIND_TCP|
> >> LANDLOCK_ACCESS_NET_CONNECT_TCP,
> >> .scoped=
> >> LANDLOCK_SCOPE_ABSTRACT_UNIX_SOCKET|
> >> LANDLOCK_SCOPE_SIGNAL,
> >> .scoped_fs =
> >> LANDLOCK_SCOPE_FS_PATHNAME_UNIX_SOCKET
> >> };
> >>
> >> This more clearly distinguishes between scopes that have exceptions/corresponding fs rights,
> >> and ones that don't. Later we could add scoped_net, if needed. I feel like this would be more
> >> intuitive and better categorize future scoping rights. An obvious con is increasing the size of
> >> the ruleset attributes.
> > I see your point but I don't think it would be worth it to add
> > sub-scoped fields. Each field has a clear semantic, and the scoped one
> > is related to the domain wrt other domains.
> As long as it's documented clearly, and future IPCs have similar behavior
> I agree that a separate field probably isn't needed.
> >
> >> Of course this separation is only worth it if there are other "exempt-able" rights in the future.
> >> I can think of a few potential future rights which COULD be scoped and have corresponding rights
> >> (binder, sysv-ipc, pipes, tcp/udp between two local programs).
> > Yes, it would definitely be useful to add exception for other kind of
> > IPCs. The idea would be to be able to describe the peer, either with a
> > file path, or PID, or cgroups, or a Landlock domain... The inet case
> > is an interesting idea but that might be a challenging task to
> > implement, if even possible.
>
> >
> >>>>>
> >>>>> In conclusion, I think the approach has significant upsides:
> >>>>>
> >>>>> * Simpler UAPI: Users only have one access bit to deal with, in the
> >>>>> near future. Once we do add a scope flag for UNIX connections, it
> >>>>> does not interact in a surprising way with the corresponding FS
> >>>>> access right, because with either of these, scoped access is
> >>>>> allowed.
> >>>>>
> >>>>> If users absolutely need to restrict scoped access, they can
> >>>>> restrict LANDLOCK_ACCESS_FS_MAKE_SOCK. It is a slightly obscure
> >>>>> API, but in line with the "make easy things easy, make hard things
> >>>>> possible" API philosophy. And needing this should be the
> >>>>> exception rather than the norm, after all.
> >>>>>
> >>>>> * Consistent behaviour between scoped flags and regular access
> >>>>> rights, also for speculative access rights affecting the existing
> >>>>> scoped flags for signals and abstract UNIX domain sockets.
> >>>>>
> >>>>> [1] https://lore.kernel.org/all/f07fe41a-96c5-4d3a-9966-35b30b3a71f1@maowtm.org/
> >>> —Günther
^ permalink raw reply
* Re: [PATCH v3 0/3] landlock: documentation improvements
From: Mickaël Salaün @ 2026-02-05 19:23 UTC (permalink / raw)
To: Samasth Norway Ananda; +Cc: gnoack, linux-security-module, linux-kernel
In-Reply-To: <20260128031814.2945394-1-samasth.norway.ananda@oracle.com>
Thanks for these improvements, it helps! I'll send these changes for
Linux 7.0
On Tue, Jan 27, 2026 at 07:18:09PM -0800, Samasth Norway Ananda wrote:
> This patch series improves Landlock documentation by addressing gaps in
> ABI compatibility examples, adding errata documentation, and documenting
> the audit blockers field format.
>
> Changes since v2:
> =================
>
> Patch 1/3:
> - Handle restrict_flags in a separate code block, not in the switch
> - Clear all three ABI v7 logging flags for a generic example
> - Reference sys_landlock_restrict_self() for available flags
> - Use restrict_flags in landlock_restrict_self()
>
> Patch 2/3:
> - Use kernel-doc directives to include errata from header files
> - Move rephrased ABI version text before errata section
>
> Patch 3/3:
> - No changes
>
> Changes since v1:
> =================
>
> Patch 1/3:
> - Add backwards compatibility section for restrict flags
> - Fix /usr rule description
>
> Patch 2/3:
> - Enhance existing DOC sections with Impact descriptions
> - Add errata usage documentation
>
> Patch 3/3:
> - Document audit blocker field format
>
> Samasth Norway Ananda (3):
> landlock: add backwards compatibility for restrict flags
> landlock: add errata documentation section
> landlock: document audit blockers field format
>
> Documentation/admin-guide/LSM/landlock.rst | 20 ++++-
> Documentation/userspace-api/landlock.rst | 97 +++++++++++++++++++---
> security/landlock/errata/abi-1.h | 8 ++
> security/landlock/errata/abi-4.h | 7 ++
> security/landlock/errata/abi-6.h | 10 +++
> security/landlock/syscalls.c | 4 +-
> 6 files changed, 131 insertions(+), 15 deletions(-)
>
> --
> 2.50.1
>
>
^ permalink raw reply
* Re: [PATCH v3 2/3] ima: trim N IMA event log records
From: steven chen @ 2026-02-05 23:53 UTC (permalink / raw)
To: Roberto Sassu, linux-integrity
Cc: zohar, roberto.sassu, dmitry.kasatkin, eric.snowberg, corbet,
serge, paul, jmorris, linux-security-module, anirudhve,
gregorylumen, nramas, sushring, linux-doc, steven chen
In-Reply-To: <74ae5c5c4cb6644537b2643d02b649a2064b718d.camel@huaweicloud.com>
On 1/7/2026 2:06 AM, Roberto Sassu wrote:
> On Mon, 2026-01-05 at 18:07 -0800, steven chen wrote:
>> Trim N entries of the IMA event logs. Clean the hash table if
>> ima_flush_htable is set.
>>
>> Provide a userspace interface ima_trim_log that can be used to input
>> number N to let kernel to trim N entries of IMA event logs. When read
>> this interface, it returns number of entries trimmed last time.
>>
>> Signed-off-by: steven chen <chenste@linux.microsoft.com>
>> ---
>> .../admin-guide/kernel-parameters.txt | 4 +
>> security/integrity/ima/ima.h | 2 +
>> security/integrity/ima/ima_fs.c | 164 +++++++++++++++++-
>> security/integrity/ima/ima_queue.c | 85 +++++++++
>> 4 files changed, 251 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index e92c0056e4e0..cd1a1d0bf0e2 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -2197,6 +2197,10 @@
>> Use the canonical format for the binary runtime
>> measurements, instead of host native format.
>>
>> + ima_flush_htable [IMA]
>> + Flush the measurement list hash table when trim all
>> + or a part of it for deletion.
>> +
>> ima_hash= [IMA]
>> Format: { md5 | sha1 | rmd160 | sha256 | sha384
>> | sha512 | ... }
>> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
>> index e3d71d8d56e3..2102c523dca0 100644
>> --- a/security/integrity/ima/ima.h
>> +++ b/security/integrity/ima/ima.h
>> @@ -246,8 +246,10 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
>>
>> #ifdef CONFIG_IMA_KEXEC
>> void ima_measure_kexec_event(const char *event_name);
>> +long ima_delete_event_log(long req_val);
>> #else
>> static inline void ima_measure_kexec_event(const char *event_name) {}
>> +static inline long ima_delete_event_log(long req_val) { return 0; }
>> #endif
>>
>> /*
>> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
>> index 87045b09f120..67ff0cfc3d3f 100644
>> --- a/security/integrity/ima/ima_fs.c
>> +++ b/security/integrity/ima/ima_fs.c
>> @@ -21,6 +21,9 @@
>> #include <linux/rcupdate.h>
>> #include <linux/parser.h>
>> #include <linux/vmalloc.h>
>> +#include <linux/ktime.h>
>> +#include <linux/timekeeping.h>
>> +#include <linux/ima.h>
>>
>> #include "ima.h"
>>
>> @@ -38,6 +41,17 @@ __setup("ima_canonical_fmt", default_canonical_fmt_setup);
>>
>> static int valid_policy = 1;
>>
>> +#define IMA_LOG_TRIM_REQ_LENGTH 11
>> +#define IMA_LOG_TRIM_EVENT_LEN 256
> Shouldn't this belong to the next patch?
Will update this. Thanks
>> +
>> +static long trimcount;
>> +/* mutex protects atomicity of trimming measurement list
>> + * and also protects atomicity the measurement list read
>> + * write operation.
>> + */
>> +static DEFINE_MUTEX(ima_measure_lock);
>> +static long ima_measure_users;
>> +
>> static ssize_t ima_show_htable_value(char __user *buf, size_t count,
>> loff_t *ppos, atomic_long_t *val)
>> {
>> @@ -202,16 +216,77 @@ static const struct seq_operations ima_measurments_seqops = {
>> .show = ima_measurements_show
>> };
>>
>> +/*
>> + * _ima_measurements_open - open the IMA measurements file
>> + * @inode: inode of the file being opened
>> + * @file: file being opened
>> + * @seq_ops: sequence operations for the file
>> + *
>> + * Returns 0 on success, or negative error code.
>> + * Implements mutual exclusion between readers and writer
>> + * of the measurements file. Multiple readers are allowed,
>> + * but writer get exclusive access only no other readers/writers.
>> + * Readers is not allowed when there is a writer.
>> + */
>> +static int _ima_measurements_open(struct inode *inode, struct file *file,
>> + const struct seq_operations *seq_ops)
>> +{
>> + bool write = !!(file->f_mode & FMODE_WRITE);
>> + int ret;
>> +
>> + if (write && !capable(CAP_SYS_ADMIN))
>> + return -EPERM;
>> +
>> + mutex_lock(&ima_measure_lock);
>> + if ((write && ima_measure_users != 0) ||
>> + (!write && ima_measure_users < 0)) {
>> + mutex_unlock(&ima_measure_lock);
>> + return -EBUSY;
>> + }
>> +
>> + ret = seq_open(file, seq_ops);
>> + if (ret < 0) {
>> + mutex_unlock(&ima_measure_lock);
>> + return ret;
>> + }
>> +
>> + if (write)
>> + ima_measure_users--;
>> + else
>> + ima_measure_users++;
>> +
>> + mutex_unlock(&ima_measure_lock);
>> + return ret;
>> +}
>> +
>> static int ima_measurements_open(struct inode *inode, struct file *file)
>> {
>> - return seq_open(file, &ima_measurments_seqops);
>> + return _ima_measurements_open(inode, file, &ima_measurments_seqops);
>> +}
>> +
>> +static int ima_measurements_release(struct inode *inode, struct file *file)
>> +{
>> + bool write = !!(file->f_mode & FMODE_WRITE);
>> + int ret;
>> +
>> + mutex_lock(&ima_measure_lock);
>> + ret = seq_release(inode, file);
>> + if (!ret) {
>> + if (write)
>> + ima_measure_users++;
>> + else
>> + ima_measure_users--;
>> + }
>> +
>> + mutex_unlock(&ima_measure_lock);
>> + return ret;
>> }
>>
>> static const struct file_operations ima_measurements_ops = {
>> .open = ima_measurements_open,
>> .read = seq_read,
>> .llseek = seq_lseek,
>> - .release = seq_release,
>> + .release = ima_measurements_release,
>> };
>>
>> void ima_print_digest(struct seq_file *m, u8 *digest, u32 size)
>> @@ -279,14 +354,83 @@ static const struct seq_operations ima_ascii_measurements_seqops = {
>>
>> static int ima_ascii_measurements_open(struct inode *inode, struct file *file)
>> {
>> - return seq_open(file, &ima_ascii_measurements_seqops);
>> + return _ima_measurements_open(inode, file, &ima_ascii_measurements_seqops);
>> }
>>
>> static const struct file_operations ima_ascii_measurements_ops = {
>> .open = ima_ascii_measurements_open,
>> .read = seq_read,
>> .llseek = seq_lseek,
>> - .release = seq_release,
>> + .release = ima_measurements_release,
>> +};
>> +
>> +static int ima_log_trim_open(struct inode *inode, struct file *file)
>> +{
>> + bool write = !!(file->f_mode & FMODE_WRITE);
>> +
>> + if (!write && capable(CAP_SYS_ADMIN))
>> + return 0;
>> + else if (!capable(CAP_SYS_ADMIN))
>> + return -EPERM;
>> +
>> + return _ima_measurements_open(inode, file, &ima_measurments_seqops);
>> +}
>> +
>> +static ssize_t ima_log_trim_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
>> +{
>> + char tmpbuf[IMA_LOG_TRIM_REQ_LENGTH]; /* greater than largest 'long' string value */
>> + ssize_t len;
>> +
>> + len = scnprintf(tmpbuf, sizeof(tmpbuf), "%li\n", trimcount);
>> + return simple_read_from_buffer(buf, size, ppos, tmpbuf, len);
>> +}
>> +
>> +static ssize_t ima_log_trim_write(struct file *file,
>> + const char __user *buf, size_t datalen, loff_t *ppos)
>> +{
>> + long count, n, ret;
>> +
>> + if (*ppos > 0 || datalen > IMA_LOG_TRIM_REQ_LENGTH || datalen < 2) {
>> + ret = -EINVAL;
>> + goto out;
>> + }
>> +
>> + n = (int)datalen;
>> +
>> + ret = kstrtol_from_user(buf, n, 10, &count);
>> + if (ret < 0)
>> + goto out;
>> +
>> + ret = ima_delete_event_log(count);
>> +
>> + if (ret < 0)
>> + goto out;
>> +
>> + trimcount = ret;
>> +
>> + ret = datalen;
>> +out:
>> + return ret;
>> +}
>> +
>> +static int ima_log_trim_release(struct inode *inode, struct file *file)
>> +{
>> + bool write = !!(file->f_mode & FMODE_WRITE);
>> +
>> + if (!write && capable(CAP_SYS_ADMIN))
>> + return 0;
>> + else if (!capable(CAP_SYS_ADMIN))
>> + return -EPERM;
>> +
>> + return ima_measurements_release(inode, file);
>> +}
>> +
>> +static const struct file_operations ima_log_trim_ops = {
>> + .open = ima_log_trim_open,
>> + .read = ima_log_trim_read,
>> + .write = ima_log_trim_write,
>> + .llseek = generic_file_llseek,
>> + .release = ima_log_trim_release
>> };
>>
>> static ssize_t ima_read_policy(char *path)
>> @@ -528,6 +672,18 @@ int __init ima_fs_init(void)
>> goto out;
>> }
>>
>> + if (IS_ENABLED(CONFIG_IMA_LOG_TRIMMING)) {
>> + dentry = securityfs_create_file("ima_trim_log",
>> + S_IRUSR | S_IRGRP | S_IWUSR | S_IWGRP,
>> + ima_dir, NULL, &ima_log_trim_ops);
>> + if (IS_ERR(dentry)) {
>> + ret = PTR_ERR(dentry);
>> + goto out;
>> + }
>> + }
>> +
>> + trimcount = 0;
>> +
>> dentry = securityfs_create_file("runtime_measurements_count",
>> S_IRUSR | S_IRGRP, ima_dir, NULL,
>> &ima_measurements_count_ops);
>> diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
>> index 590637e81ad1..33bb5414b8cc 100644
>> --- a/security/integrity/ima/ima_queue.c
>> +++ b/security/integrity/ima/ima_queue.c
>> @@ -22,6 +22,14 @@
>>
>> #define AUDIT_CAUSE_LEN_MAX 32
>>
>> +bool ima_flush_htable;
>> +static int __init ima_flush_htable_setup(char *str)
>> +{
>> + ima_flush_htable = true;
>> + return 1;
>> +}
>> +__setup("ima_flush_htable", ima_flush_htable_setup);
>> +
>> /* pre-allocated array of tpm_digest structures to extend a PCR */
>> static struct tpm_digest *digests;
>>
>> @@ -220,6 +228,83 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
>> return result;
>> }
>>
>> +/**
>> + * ima_delete_event_log - delete IMA event entry
>> + * @num_records: number of records to delete
>> + *
>> + * delete num_records entries off the measurement list.
>> + * Returns the number of entries deleted, or negative error code.
> This is not according to the format stated in the documentation.
Will update this. Thanks
>> + */
>> +long ima_delete_event_log(long num_records)
>> +{
>> + long len, cur = num_records, tmp_len = 0;
>> + struct ima_queue_entry *qe, *qe_tmp;
>> + LIST_HEAD(ima_measurements_staged);
>> + struct list_head *list_ptr;
>> +
>> + if (num_records <= 0)
>> + return num_records;
>> +
>> + if (!IS_ENABLED(CONFIG_IMA_LOG_TRIMMING))
>> + return -EOPNOTSUPP;
>> +
>> + mutex_lock(&ima_extend_list_mutex);
>> + len = atomic_long_read(&ima_htable.len);
>> +
>> + if (num_records > len) {
>> + mutex_unlock(&ima_extend_list_mutex);
>> + return -ENOENT;
>> + }
>> +
>> + list_ptr = &ima_measurements;
>> +
>> + if (cur == len) {
>> + list_replace(&ima_measurements, &ima_measurements_staged);
>> + INIT_LIST_HEAD(&ima_measurements);
>> + atomic_long_set(&ima_htable.len, 0);
>> + list_ptr = &ima_measurements_staged;
>> + if (IS_ENABLED(CONFIG_IMA_KEXEC))
>> + binary_runtime_size = 0;
> Like in my patch, we should have kept the original value of
> binary_runtime_size, to avoid breaking the kexec critical data records.
I think this value is updated when trimming finish.
>> + }
>> +
>> + list_for_each_entry(qe, list_ptr, later) {
>> + if (num_records > 0) {
>> + if (!IS_ENABLED(CONFIG_IMA_DISABLE_HTABLE) && ima_flush_htable)
>> + hlist_del_rcu(&qe->hnext);
>> +
>> + --num_records;
>> + if (num_records == 0)
>> + qe_tmp = qe;
>> + continue;
>> + }
>> + if (len != cur && IS_ENABLED(CONFIG_IMA_KEXEC))
>> + tmp_len += get_binary_runtime_size(qe->entry);
>> + else
>> + break;
>> + }
>> +
>> + if (len != cur) {
>> + __list_cut_position(&ima_measurements_staged, &ima_measurements,
>> + &qe_tmp->later);
>> + atomic_long_sub(cur, &ima_htable.len);
>> + if (IS_ENABLED(CONFIG_IMA_KEXEC))
>> + binary_runtime_size = tmp_len;
>> + }
>> +
>> + mutex_unlock(&ima_extend_list_mutex);
>> +
>> + if (ima_flush_htable)
>> + synchronize_rcu();
>> +
>> + list_for_each_entry_safe(qe, qe_tmp, &ima_measurements_staged, later) {
>> + ima_free_template_entry(qe->entry);
>> + list_del(&qe->later);
>> + kfree(qe);
> If you don't flush the hash table, you cannot delete the entry.
>
> Roberto
Will update this. Thanks
Steven
>> + }
>> +
>> + return cur;
>> +}
>> +
>> int ima_restore_measurement_entry(struct ima_template_entry *entry)
>> {
>> int result = 0;
^ permalink raw reply
* [PATCH v4 0/1] Trim N entries of IMA event logs
From: steven chen @ 2026-02-05 23:58 UTC (permalink / raw)
To: linux-integrity
Cc: zohar, roberto.sassu, dmitry.kasatkin, eric.snowberg, corbet,
serge, paul, jmorris, linux-security-module, anirudhve, chenste,
gregorylumen, nramas, sushring, linux-doc
The Integrity Measurement Architecture (IMA) maintains a measurement list
—a record of system events used for integrity verification. The IMA event
logs are the entries within this measurement list, each representing a
specific event or measurement that contributes to the system's integrity
assessment.
This update introduces the ability to trim, or remove, N entries from the
current measurement list. Trimming involves deleting N entries from the
list and clearing the corresponding entries from the hash table. This
action atomically truncates the measurement list, ensuring that no new
measurements can be added until the operation is complete. Importantly,
only one writer can initiate this trimming process at a time, maintaining
consistency and preventing race conditions.
A userspace interface, ima_trim_log, has been provided for this purpose.
When this interface is read, it returns the total number T of entries
trimmed since system boot up. This value T need to be preserved across
kexec soft reboots. By writing two number T:N to this interface, userspace
can request the kernel to trim N entries from the IMA event logs.
To maintain a complete record, userspace is responsible for concatenating
and storing the logs before initiating trimming. Userspace can then send
the collected data to remote verifiers for validation. After receiving
confirmation from the remote verifiers, userspace may instruct the kernel
to proceed with trimming the IMA event logs accordingly.
The primary benefit of this solution is the ability to free valuable
kernel memory by delegating the task of reconstructing the full
measurement list from log chunks to userspace. Trust is not required in
userspace for the integrity of the measurement list, as its integrity is
cryptographically protected by the Trusted Platform Module (TPM).
Multiple readers are allowed to access the ima_trim_log interface
concurrently, while only one writer can trigger log trimming at any time.
During trimming, readers do not see the list and cannot access it while
deletion is in progress, ensuring atomicity.
Introduce the new kernel option ima_flush_htable to decide whether or not
the digests of measurement entries are flushed from the hash table (from
reference [2]).
The ima_measure_users counter (protected by the ima_measure_lock mutex) has
been introduced to protect access to the measurement list part. The open
method of all the measurement interfaces has been extended to allow only
one writer at a time or, in alternative, multiple readers. The write
permission is used to stage/delete the measurements, the read permission
to read them. Write requires also the CAP_SYS_ADMIN capability (from
reference [2]). This ima_measure_users needs to be preserved across kexec
soft reboots
The total trimmed number T and the ima_measure_users both need to be
preserved across kexec soft reboot and new patch will be added for this
purpose in next version.
New IMA log trim event is added when trimming finish.
The time required for trimming is minimal, and IMA event logs are briefly
on hold during this process, preventing read or add operations. This short
interruption has no impact on the overall functionality of IMA.
A new critical data record "ima_log_trim" is added in this solution. This
record logs the trim event with number of entries deleted total T since
system start and time when this happened. User space can get the total
number T of entries trimmed by checking "ima_log_trim" event in the
measurement list.
The following are how user space to use the measurement list and
ima_log_trim interface
1. get the total numer trimmed T through "ima_log_trim" interface
2. get the PCR quote
3. read the measurement list file, close the file, send for verification
4. wait for response from verifier, until get the good response from
verifier with number N that matched the PCR quote got in step 2
5. get the number N from the above message
6. write the T:N to the ima_log_trim interface when no conflict
when kernel get log trim request T:N
Get the T, compare with the total trimmed number
if equal, then do trim N and change T to T+N
else return error
Using above way to trim the log, the time for user space to hold the list
will be trimming T:N operation itself at the step 6. User space agent
race condition is solved too in this way.
References:
-----------
[1] [PATCH 0/1] Trim N entries of IMA event logs
https://lore.kernel.org/linux-integrity/20251202232857.8211-1-chenste@linux.microsoft.com/T/#t
[2] [RFC][PATCH] ima: Add support for staging measurements for deletion
https://lore.kernel.org/linux-integrity/207fd6d7-53c-57bb-36d8-13a0902052d1@linux.microsoft.com/T/#t
[3] [PATCH v2 0/1] Trim N entries of IMA event logs
https://lore.kernel.org/linux-integrity/20251210235314.3341-1-chenste@linux.microsoft.com/T/#t
[4] [PATCH v3 0/3] Trim N entries of IMA event logs
https://lore.kernel.org/linux-integrity/20260106020713.3994-1-chenste@linux.microsoft.com/T/#t
Change Log v4:
- Incorporated feedback from Roberto on v3 series.
- Update "ima_log_trim" interface definition
When read this interface, return total number of records trimmed T
need to write T:N to this interface to trim N records
- Update user space use case on how to trim IMA event logs
- Updated patch descriptions as necessary.
Change Log v3:
- Incorporated feedback from Mimi on v2 series.
- split patch into multiple patches
- lock time performance improvement
- Updated patch descriptions as necessary.
Change Log v2:
- Incorporated feedback from the Roberto on v1 series.
- Adapted code from Roberto's RFC [Reference 2]
- Add IMA log trim event log to record trim event
- Updated patch descriptions as necessary.
steven chen (3):
ima: make ima event log trimming configurable
ima: trim N IMA event log records
ima: add new critical data record to measure log trim
.../admin-guide/kernel-parameters.txt | 4 +
security/integrity/ima/Kconfig | 12 +
security/integrity/ima/ima.h | 2 +
security/integrity/ima/ima_fs.c | 214 +++++++++++++++++-
security/integrity/ima/ima_queue.c | 100 ++++++++
5 files changed, 328 insertions(+), 4 deletions(-)
--
2.43.0
^ permalink raw reply
* [PATCH v4 1/3] ima: make ima event log trimming configurable
From: steven chen @ 2026-02-05 23:58 UTC (permalink / raw)
To: linux-integrity
Cc: zohar, roberto.sassu, dmitry.kasatkin, eric.snowberg, corbet,
serge, paul, jmorris, linux-security-module, anirudhve, chenste,
gregorylumen, nramas, sushring, linux-doc
In-Reply-To: <20260205235849.7086-1-chenste@linux.microsoft.com>
Make ima event log trimming function configurable.
Signed-off-by: steven chen <chenste@linux.microsoft.com>
---
security/integrity/ima/Kconfig | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 976e75f9b9ba..322964ae4772 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -332,4 +332,16 @@ config IMA_KEXEC_EXTRA_MEMORY_KB
If set to the default value of 0, an extra half page of memory for those
additional measurements will be allocated.
+config IMA_LOG_TRIMMING
+ bool "IMA Event Log Trimming"
+ default n
+ help
+ Say Y here if you want support for IMA Event Log Trimming.
+ This creates the file /sys/kernel/security/integrity/ima/ima_trim_log.
+ Userspace
+ - writes to this file to trigger IMA event log trimming
+ - reads this file to get number of entried trimming last time
+
+ If unsure, say N.
+
endif
--
2.43.0
^ permalink raw reply related
* [PATCH v4 2/3] ima: trim N IMA event log records
From: steven chen @ 2026-02-05 23:58 UTC (permalink / raw)
To: linux-integrity
Cc: zohar, roberto.sassu, dmitry.kasatkin, eric.snowberg, corbet,
serge, paul, jmorris, linux-security-module, anirudhve, chenste,
gregorylumen, nramas, sushring, linux-doc
In-Reply-To: <20260205235849.7086-1-chenste@linux.microsoft.com>
Trim N entries of the IMA event logs. Clean the hash table if
ima_flush_htable is set.
Provide a userspace interface ima_trim_log:
When read this interface, it returns total number T of entries trimmed
since system boot up.
When write to this interface need to provide two numbers T:N to let
kernel to trim N entries of IMA event logs.
when kernel get log trim request T:N
- Get the T, compare with the total trimmed number
- if equal, then do trim N and change T to T+N
- else return error
Signed-off-by: steven chen <chenste@linux.microsoft.com>
---
.../admin-guide/kernel-parameters.txt | 4 +
security/integrity/ima/ima.h | 2 +
security/integrity/ima/ima_fs.c | 195 +++++++++++++++++-
security/integrity/ima/ima_queue.c | 100 +++++++++
4 files changed, 297 insertions(+), 4 deletions(-)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index e92c0056e4e0..cd1a1d0bf0e2 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -2197,6 +2197,10 @@
Use the canonical format for the binary runtime
measurements, instead of host native format.
+ ima_flush_htable [IMA]
+ Flush the measurement list hash table when trim all
+ or a part of it for deletion.
+
ima_hash= [IMA]
Format: { md5 | sha1 | rmd160 | sha256 | sha384
| sha512 | ... }
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index e3d71d8d56e3..2102c523dca0 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -246,8 +246,10 @@ void ima_post_key_create_or_update(struct key *keyring, struct key *key,
#ifdef CONFIG_IMA_KEXEC
void ima_measure_kexec_event(const char *event_name);
+long ima_delete_event_log(long req_val);
#else
static inline void ima_measure_kexec_event(const char *event_name) {}
+static inline long ima_delete_event_log(long req_val) { return 0; }
#endif
/*
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 87045b09f120..7f805ab62f6c 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -21,6 +21,9 @@
#include <linux/rcupdate.h>
#include <linux/parser.h>
#include <linux/vmalloc.h>
+#include <linux/ktime.h>
+#include <linux/timekeeping.h>
+#include <linux/ima.h>
#include "ima.h"
@@ -38,6 +41,17 @@ __setup("ima_canonical_fmt", default_canonical_fmt_setup);
static int valid_policy = 1;
+#define IMA_LOG_TRIM_REQ_NUM_LENGTH 15
+#define IMA_LOG_TRIM_REQ_TOTAL_LENGTH 32
+
+static long trimcount;
+/* mutex protects atomicity of trimming measurement list
+ * and also protects atomicity the measurement list read
+ * write operation.
+ */
+static DEFINE_MUTEX(ima_measure_lock);
+static long ima_measure_users;
+
static ssize_t ima_show_htable_value(char __user *buf, size_t count,
loff_t *ppos, atomic_long_t *val)
{
@@ -202,16 +216,77 @@ static const struct seq_operations ima_measurments_seqops = {
.show = ima_measurements_show
};
+/*
+ * _ima_measurements_open - open the IMA measurements file
+ * @inode: inode of the file being opened
+ * @file: file being opened
+ * @seq_ops: sequence operations for the file
+ *
+ * Returns 0 on success, or negative error code.
+ * Implements mutual exclusion between readers and writer
+ * of the measurements file. Multiple readers are allowed,
+ * but writer get exclusive access only no other readers/writers.
+ * Readers is not allowed when there is a writer.
+ */
+static int _ima_measurements_open(struct inode *inode, struct file *file,
+ const struct seq_operations *seq_ops)
+{
+ bool write = !!(file->f_mode & FMODE_WRITE);
+ int ret;
+
+ if (write && !capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
+ mutex_lock(&ima_measure_lock);
+ if ((write && ima_measure_users != 0) ||
+ (!write && ima_measure_users < 0)) {
+ mutex_unlock(&ima_measure_lock);
+ return -EBUSY;
+ }
+
+ ret = seq_open(file, seq_ops);
+ if (ret < 0) {
+ mutex_unlock(&ima_measure_lock);
+ return ret;
+ }
+
+ if (write)
+ ima_measure_users--;
+ else
+ ima_measure_users++;
+
+ mutex_unlock(&ima_measure_lock);
+ return ret;
+}
+
static int ima_measurements_open(struct inode *inode, struct file *file)
{
- return seq_open(file, &ima_measurments_seqops);
+ return _ima_measurements_open(inode, file, &ima_measurments_seqops);
+}
+
+static int ima_measurements_release(struct inode *inode, struct file *file)
+{
+ bool write = !!(file->f_mode & FMODE_WRITE);
+ int ret;
+
+ mutex_lock(&ima_measure_lock);
+ ret = seq_release(inode, file);
+ if (!ret) {
+ if (!write)
+ ima_measure_users--;
+ else
+ ima_measure_users++;
+ }
+
+ mutex_unlock(&ima_measure_lock);
+ return ret;
}
static const struct file_operations ima_measurements_ops = {
.open = ima_measurements_open,
.read = seq_read,
.llseek = seq_lseek,
- .release = seq_release,
+ .release = ima_measurements_release,
};
void ima_print_digest(struct seq_file *m, u8 *digest, u32 size)
@@ -279,14 +354,114 @@ static const struct seq_operations ima_ascii_measurements_seqops = {
static int ima_ascii_measurements_open(struct inode *inode, struct file *file)
{
- return seq_open(file, &ima_ascii_measurements_seqops);
+ return _ima_measurements_open(inode, file, &ima_ascii_measurements_seqops);
}
static const struct file_operations ima_ascii_measurements_ops = {
.open = ima_ascii_measurements_open,
.read = seq_read,
.llseek = seq_lseek,
- .release = seq_release,
+ .release = ima_measurements_release,
+};
+
+static int ima_log_trim_open(struct inode *inode, struct file *file)
+{
+ bool write = !!(file->f_mode & FMODE_WRITE);
+
+ if (!write && capable(CAP_SYS_ADMIN))
+ return 0;
+ else if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
+ return _ima_measurements_open(inode, file, &ima_measurments_seqops);
+}
+
+static ssize_t ima_log_trim_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
+{
+ char tmpbuf[IMA_LOG_TRIM_REQ_NUM_LENGTH];
+ ssize_t len;
+
+ len = scnprintf(tmpbuf, sizeof(tmpbuf), "%li\n", trimcount);
+ return simple_read_from_buffer(buf, size, ppos, tmpbuf, len);
+}
+
+static ssize_t ima_log_trim_write(struct file *file,
+ const char __user *buf, size_t datalen, loff_t *ppos)
+{
+ char tmpbuf[IMA_LOG_TRIM_REQ_TOTAL_LENGTH];
+ char *p = tmpbuf;
+ long count, ret, val = 0, max = LONG_MAX;
+
+ if (*ppos > 0 || datalen > IMA_LOG_TRIM_REQ_TOTAL_LENGTH || datalen < 2) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ if (copy_from_user(tmpbuf, buf, datalen) != 0) {
+ ret = -EFAULT;
+ goto out;
+ }
+
+ p = tmpbuf;
+
+ while (*p && *p != ':') {
+ if (!isdigit((unsigned char)*p))
+ return -EINVAL;
+
+ /* digit value */
+ int d = *p - '0';
+
+ /* overflow check: val * 10 + d > max -> (val > (max - d) / 10) */
+ if (val > (max - d) / 10)
+ return -ERANGE;
+
+ val = val * 10 + d;
+ p++;
+ }
+
+ if (*p != ':')
+ return -EINVAL;
+
+ /* verify trim count matches */
+ if (val != trimcount)
+ return -EINVAL;
+
+ p++; /* skip ':' */
+ ret = kstrtoul(p, 0, &count);
+
+ if (ret < 0)
+ goto out;
+
+ ret = ima_delete_event_log(count);
+
+ if (ret < 0)
+ goto out;
+
+ trimcount += ret;
+
+ ret = datalen;
+out:
+ return ret;
+}
+
+static int ima_log_trim_release(struct inode *inode, struct file *file)
+{
+ bool write = !!(file->f_mode & FMODE_WRITE);
+
+ if (!write && capable(CAP_SYS_ADMIN))
+ return 0;
+ else if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
+ return ima_measurements_release(inode, file);
+}
+
+static const struct file_operations ima_log_trim_ops = {
+ .open = ima_log_trim_open,
+ .read = ima_log_trim_read,
+ .write = ima_log_trim_write,
+ .llseek = generic_file_llseek,
+ .release = ima_log_trim_release
};
static ssize_t ima_read_policy(char *path)
@@ -528,6 +703,18 @@ int __init ima_fs_init(void)
goto out;
}
+ if (IS_ENABLED(CONFIG_IMA_LOG_TRIMMING)) {
+ dentry = securityfs_create_file("ima_trim_log",
+ S_IRUSR | S_IRGRP | S_IWUSR | S_IWGRP,
+ ima_dir, NULL, &ima_log_trim_ops);
+ if (IS_ERR(dentry)) {
+ ret = PTR_ERR(dentry);
+ goto out;
+ }
+ }
+
+ trimcount = 0;
+
dentry = securityfs_create_file("runtime_measurements_count",
S_IRUSR | S_IRGRP, ima_dir, NULL,
&ima_measurements_count_ops);
diff --git a/security/integrity/ima/ima_queue.c b/security/integrity/ima/ima_queue.c
index 590637e81ad1..5ef722d0fa24 100644
--- a/security/integrity/ima/ima_queue.c
+++ b/security/integrity/ima/ima_queue.c
@@ -22,6 +22,14 @@
#define AUDIT_CAUSE_LEN_MAX 32
+bool ima_flush_htable;
+static int __init ima_flush_htable_setup(char *str)
+{
+ ima_flush_htable = true;
+ return 1;
+}
+__setup("ima_flush_htable", ima_flush_htable_setup);
+
/* pre-allocated array of tpm_digest structures to extend a PCR */
static struct tpm_digest *digests;
@@ -220,6 +228,98 @@ int ima_add_template_entry(struct ima_template_entry *entry, int violation,
return result;
}
+/**
+ * ima_delete_event_log - delete IMA event entry
+ * @num_records: number of records to delete
+ *
+ * delete num_records entries off the measurement list.
+ * Returns num_records, or negative error code.
+ */
+long ima_delete_event_log(long num_records)
+{
+ long len, cur = num_records, tmp_len = 0;
+ struct ima_queue_entry *qe, *qe_tmp;
+ LIST_HEAD(ima_measurements_to_delete);
+ struct list_head *list_ptr;
+
+ if (!IS_ENABLED(CONFIG_IMA_LOG_TRIMMING))
+ return -EOPNOTSUPP;
+
+ if (num_records <= 0)
+ return num_records;
+
+ mutex_lock(&ima_extend_list_mutex);
+ len = atomic_long_read(&ima_htable.len);
+
+ if (num_records > len) {
+ mutex_unlock(&ima_extend_list_mutex);
+ return -ENOENT;
+ }
+
+ list_ptr = &ima_measurements;
+
+ if (num_records == len) {
+ list_replace(&ima_measurements, &ima_measurements_to_delete);
+ INIT_LIST_HEAD(&ima_measurements);
+ atomic_long_set(&ima_htable.len, 0);
+ list_ptr = &ima_measurements_to_delete;
+ if (IS_ENABLED(CONFIG_IMA_KEXEC))
+ binary_runtime_size = 0;
+ }
+
+ list_for_each_entry(qe, list_ptr, later) {
+ if (cur > 0) {
+ if (!IS_ENABLED(CONFIG_IMA_DISABLE_HTABLE) && ima_flush_htable)
+ hlist_del_rcu(&qe->hnext);
+
+ --cur;
+ if (cur == 0)
+ qe_tmp = qe;
+ continue;
+ }
+ if (len != num_records && IS_ENABLED(CONFIG_IMA_KEXEC))
+ tmp_len += get_binary_runtime_size(qe->entry);
+ else
+ break;
+ }
+
+ if (len != num_records) {
+ __list_cut_position(&ima_measurements_to_delete, &ima_measurements,
+ &qe_tmp->later);
+ atomic_long_sub(num_records, &ima_htable.len);
+ if (IS_ENABLED(CONFIG_IMA_KEXEC))
+ binary_runtime_size = tmp_len;
+ }
+
+ mutex_unlock(&ima_extend_list_mutex);
+
+ if (ima_flush_htable)
+ synchronize_rcu();
+
+ list_for_each_entry_safe(qe, qe_tmp, &ima_measurements_to_delete, later) {
+ /*
+ * Ok because after list delete qe is only accessed by
+ * ima_lookup_digest_entry().
+ */
+ for (int i = 0; i < qe->entry->template_desc->num_fields; i++) {
+ kfree(qe->entry->template_data[i].data);
+ qe->entry->template_data[i].data = NULL;
+ qe->entry->template_data[i].len = 0;
+ }
+
+ list_del(&qe->later);
+
+ /* No leak if !ima_flush_htable, referenced by ima_htable. */
+ if (ima_flush_htable) {
+ kfree(qe->entry->digests);
+ kfree(qe->entry);
+ kfree(qe);
+ }
+ }
+
+ return num_records;
+}
+
int ima_restore_measurement_entry(struct ima_template_entry *entry)
{
int result = 0;
--
2.43.0
^ permalink raw reply related
* [PATCH v4 3/3] ima: add new critical data record to measure log trim
From: steven chen @ 2026-02-05 23:58 UTC (permalink / raw)
To: linux-integrity
Cc: zohar, roberto.sassu, dmitry.kasatkin, eric.snowberg, corbet,
serge, paul, jmorris, linux-security-module, anirudhve, chenste,
gregorylumen, nramas, sushring, linux-doc
In-Reply-To: <20260205235849.7086-1-chenste@linux.microsoft.com>
Add a new critical data record to measure the trimming event when
ima event records are deleted since system boot up.
If all IMA event logs are saved in the userspace, use this log to get total
numbers of records deleted since system boot up at that point.
Signed-off-by: steven chen <chenste@linux.microsoft.com>
---
security/integrity/ima/ima_fs.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 7f805ab62f6c..1d6befa51044 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -43,6 +43,7 @@ static int valid_policy = 1;
#define IMA_LOG_TRIM_REQ_NUM_LENGTH 15
#define IMA_LOG_TRIM_REQ_TOTAL_LENGTH 32
+#define IMA_LOG_TRIM_EVENT_LEN 256
static long trimcount;
/* mutex protects atomicity of trimming measurement list
@@ -364,6 +365,22 @@ static const struct file_operations ima_ascii_measurements_ops = {
.release = ima_measurements_release,
};
+static void ima_measure_trim_event(void)
+{
+ char ima_log_trim_event[IMA_LOG_TRIM_EVENT_LEN];
+ struct timespec64 ts;
+ u64 time_ns;
+ int n;
+
+ ktime_get_real_ts64(&ts);
+ time_ns = (u64)ts.tv_sec * 1000000000ULL + ts.tv_nsec;
+ n = scnprintf(ima_log_trim_event, IMA_LOG_TRIM_EVENT_LEN,
+ "time= %llu; number= %lu;", time_ns, trimcount);
+
+ ima_measure_critical_data("ima_log_trim", "trim ima event logs",
+ ima_log_trim_event, n, false, NULL, 0);
+}
+
static int ima_log_trim_open(struct inode *inode, struct file *file)
{
bool write = !!(file->f_mode & FMODE_WRITE);
@@ -438,6 +455,8 @@ static ssize_t ima_log_trim_write(struct file *file,
goto out;
trimcount += ret;
+ if (ret > 0)
+ ima_measure_trim_event();
ret = datalen;
out:
--
2.43.0
^ permalink raw reply related
* Re: [PATCH 04/13] Apparmor: Use simple_start_creating() / simple_done_creating()
From: NeilBrown @ 2026-02-06 0:21 UTC (permalink / raw)
To: Jeff Layton
Cc: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Miklos Szeredi, Amir Goldstein, John Johansen,
Paul Moore, James Morris, Serge E. Hallyn, Stephen Smalley,
linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <7fbfbeb0d57484172304b727bd888d1a1105f96d.camel@kernel.org>
On Thu, 05 Feb 2026, Jeff Layton wrote:
> On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> > From: NeilBrown <neil@brown.name>
> >
> > Instead of explicitly locking the parent and performing a look up in
> > apparmor, use simple_start_creating(), and then simple_done_creating()
> > to unlock and drop the dentry.
> >
> > This removes the need to check for an existing entry (as
> > simple_start_creating() acts like an exclusive create and can return
> > -EEXIST), simplifies error paths, and keeps dir locking code
> > centralised.
> >
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> > security/apparmor/apparmorfs.c | 38 ++++++++--------------------------
> > 1 file changed, 9 insertions(+), 29 deletions(-)
> >
> > diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
> > index 907bd2667e28..7f78c36e6e50 100644
> > --- a/security/apparmor/apparmorfs.c
> > +++ b/security/apparmor/apparmorfs.c
> > @@ -282,32 +282,19 @@ static struct dentry *aafs_create(const char *name, umode_t mode,
> >
> > dir = d_inode(parent);
> >
> > - inode_lock(dir);
> > - dentry = lookup_noperm(&QSTR(name), parent);
> > + dentry = simple_start_creating(parent, name);
> > if (IS_ERR(dentry)) {
> > error = PTR_ERR(dentry);
> > - goto fail_lock;
> > - }
> > -
> > - if (d_really_is_positive(dentry)) {
> > - error = -EEXIST;
> > - goto fail_dentry;
> > + goto fail;
> > }
> >
> > error = __aafs_setup_d_inode(dir, dentry, mode, data, link, fops, iops);
> > + simple_done_creating(dentry);
> > if (error)
> > - goto fail_dentry;
> > - inode_unlock(dir);
> > -
> > - return dentry;
> > -
> > -fail_dentry:
> > - dput(dentry);
> > -
> > -fail_lock:
> > - inode_unlock(dir);
> > + goto fail;
> > + return 0;
>
> As KTR points out, this should be "return NULL;"
Actually it should be "return dentry;" which is what the original code
did.
I've no idea how it became 0...
Callers of aafs_create() will silently treat NULL as failure.
>
> > +fail:
> > simple_release_fs(&aafs_mnt, &aafs_count);
> > -
> > return ERR_PTR(error);
> > }
> >
> > @@ -2572,8 +2559,7 @@ static int aa_mk_null_file(struct dentry *parent)
> > if (error)
> > return error;
> >
> > - inode_lock(d_inode(parent));
> > - dentry = lookup_noperm(&QSTR(NULL_FILE_NAME), parent);
> > + dentry = simple_start_creating(parent, NULL_FILE_NAME);
> > if (IS_ERR(dentry)) {
> > error = PTR_ERR(dentry);
> > goto out;
> > @@ -2581,7 +2567,7 @@ static int aa_mk_null_file(struct dentry *parent)
> > inode = new_inode(parent->d_inode->i_sb);
> > if (!inode) {
> > error = -ENOMEM;
> > - goto out1;
> > + goto out;
> > }
> >
> > inode->i_ino = get_next_ino();
> > @@ -2593,18 +2579,12 @@ static int aa_mk_null_file(struct dentry *parent)
> > aa_null.dentry = dget(dentry);
> > aa_null.mnt = mntget(mount);
> >
> > - error = 0;
> > -
> > -out1:
> > - dput(dentry);
> > out:
> > - inode_unlock(d_inode(parent));
> > + simple_done_creating(dentry);
> > simple_release_fs(&mount, &count);
> > return error;
> > }
> >
> > -
> > -
> > static const char *policy_get_link(struct dentry *dentry,
> > struct inode *inode,
> > struct delayed_call *done)
>
> Assuming you fix the minor problem above.
>
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
>
Thanks,
NeilBrown
^ permalink raw reply
* Re: [PATCH 08/13] ovl: Simplify ovl_lookup_real_one()
From: NeilBrown @ 2026-02-06 0:43 UTC (permalink / raw)
To: Amir Goldstein
Cc: Jeff Layton, Christian Brauner, Alexander Viro, David Howells,
Jan Kara, Chuck Lever, Miklos Szeredi, John Johansen, Paul Moore,
James Morris, Serge E. Hallyn, Stephen Smalley, linux-kernel,
netfs, linux-fsdevel, linux-nfs, linux-unionfs, apparmor,
linux-security-module, selinux
In-Reply-To: <CAOQ4uxh_Ugyy9=Vx_XOzWMTdhqVx6kAu43q+F+afhNF_Zv_9TA@mail.gmail.com>
On Fri, 06 Feb 2026, Amir Goldstein wrote:
> On Thu, Feb 5, 2026 at 1:38 PM Jeff Layton <jlayton@kernel.org> wrote:
> >
> > On Wed, 2026-02-04 at 15:57 +1100, NeilBrown 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:.
> > >
> > > Signed-off-by: NeilBrown <neil@brown.name>
> > > ---
> > > fs/overlayfs/export.c | 61 ++++++++++++++++++-------------------------
> > > 1 file changed, 26 insertions(+), 35 deletions(-)
> > >
> > > diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
> > > index 83f80fdb1567..dcd28ffc4705 100644
> > > --- a/fs/overlayfs/export.c
> > > +++ b/fs/overlayfs/export.c
> > > @@ -359,59 +359,50 @@ 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.
> > > + * @connected is a directory in the overlay and @real is an object
> > > + * on @layer which is expected to be a child of @connected.
> > > + * The goal is to return a dentry from the overlay which corresponds
>
> As the header comment already says:
> "...return a connected overlay dentry whose real dentry is @real"
>
> The wording "corresponds to @real" reduces clarity IMO.
Ok, I'll rephrase.
>
> > > + * to @real. This is done by looking up the name from @real in
> > > + * @connected and checking that the result meets expectations.
> > > + *
> > > + * Return %-ECHILD if the parent of @real no-longer corresponds to
> > > + * @connected, and %-ESTALE if the dentry found by lookup doesn't
> > > + * correspond to @real.
> > > */
>
> I dislike kernel-doc inside code comments.
> I think this is actively discouraged and I haven't found a single example
> of this style in fs code.
>
> If you want to keep this format, please lift the comment to function
> header comment - it is anyway a very generic comment that explains the
> function in general.
OK, I'll remove the formatting or move it - not sure which.
I find that with parameter names like "connected" and "real", some sort
of syntax helps.
>
> > > - 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
> > > - * 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,
> >
> > Reviewed-by: Jeff Layton <jlayton@kernel.org>
>
> Otherwise, it looks fine.
Thanks,
NeilBrown
>
> Thanks,
> Amir.
>
^ permalink raw reply
* Re: [PATCH 10/13] ovl: change ovl_create_real() to get a new lock when re-opening created file.
From: NeilBrown @ 2026-02-06 1:11 UTC (permalink / raw)
To: Amir Goldstein
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,
linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <CAOQ4uxh-MLgwZCstwr6HyPXHVRmtj2F_=xS8pE3FN6Ex-wex4w@mail.gmail.com>
On Thu, 05 Feb 2026, Amir Goldstein wrote:
> On Wed, Feb 4, 2026 at 6:09 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.
> >
> > This removes the only remaining use of ovl_lookup_upper, so it is
> > removed.
> >
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> > fs/overlayfs/dir.c | 24 ++++++++++++++++++------
> > fs/overlayfs/overlayfs.h | 7 -------
> > 2 files changed, 18 insertions(+), 13 deletions(-)
> >
> > diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> > index ff3dbd1ca61f..ec08904d084d 100644
> > --- a/fs/overlayfs/dir.c
> > +++ b/fs/overlayfs/dir.c
> > @@ -219,21 +219,33 @@ struct dentry *ovl_create_real(struct ovl_fs *ofs, struct dentry *parent,
> > err = -EIO;
> > } else if (d_unhashed(newdentry)) {
> > struct dentry *d;
> > + struct name_snapshot name;
> > /*
> > * 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().
>
>
> According to the new locking rules, if the hashed dentry itself is
> the synchronization object, is it going to be allowed to
> filesystem to unhash the dentry while the dentry still in the
> "creating" scope? It is hard for me to wrap my head around this.
It can be confusing....
It will be important for the name the remain locked (and hashed) until
the operation (create, remove, rename) either succeeds or fails. So
leaving a dentry unhashed will be OK providing a subsequent lookup will
also succeed or fail in the same way. The caller must be able to use
the dentry to access the object (i.e. the inode) on success, but they
is nothing in POSIX that requires that the object still has any
particular name.
>
> Or do we need this here because some filesystems (casefold in
> particular) are not going to support parallel creations?
There is no reason that a casefolding filesystem would not support parallel
ops. And it isn't just casefolding that acts like this. At least one of
the special filesystems (tracefs maybe) always unhashes on create. You
only ever get a hashed positive dentry as a result of lookup.
(overlayfs would never see this case of course).
>
> > * In that case, lookup again after making the newdentry
> > * positive, so ovl_create_upper() always returns a hashed
> > * positive dentry.
> > + * 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))
> > + take_dentry_name_snapshot(&name, newdentry);
> > + end_creating_keep(newdentry);
> > + d = ovl_start_creating_upper(ofs, parent, &name.name);
> > + release_dentry_name_snapshot(&name);
>
> OK. not saying no to this (yet) but I have to admit that it is pretty
> ugly that the callers of ovl_create_real() want to create a specific
> stable name, which is could be passed in as const char *name
> and yet we end up doing this weird dance here just to keep the name
> from newdentry.
There are three callers of ovl_create_real()
ovl_lookup_or_create() does have a "const char *name".
ovl_create_upper() has a stable dentry from which it can copy a QSTR
ovl_create_temp() would need some sort of dance to keep hold of the
temporary name that was allocated.
If it weren't for ovl_create_temp() I would agree with you.
Though we could have the three callers of ovl_start_creating_temp() pass a
"char name[OVL_TEMPNAME_SIZE]" in, then ovl_create_temp() would have
easy access.
I could do that if you like.
Thanks,
NeilBrown
>
> Thanks,
> Amir.
>
^ permalink raw reply
* Re: [PATCH 12/13] ovl: remove ovl_lock_rename_workdir()
From: NeilBrown @ 2026-02-06 1:18 UTC (permalink / raw)
To: Amir Goldstein
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,
linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <CAOQ4uxi3bNYq1b4=qL-JLi19hRwurntfLZXhUMVL003NarBdGg@mail.gmail.com>
On Thu, 05 Feb 2026, Amir Goldstein wrote:
> On Wed, Feb 4, 2026 at 6:09 AM NeilBrown <neilb@ownmail.net> wrote:
> >
> > From: NeilBrown <neil@brown.name>
> >
> > This function is unused.
> >
>
> I am confused.
> What was this "fix" fixing an unused function:
>
> e9c70084a64e5 ovl: fail ovl_lock_rename_workdir() if either target is unhashed
>
> What am I missing?
>
Commit 833d2b3a072f ("Add start_renaming_two_dentries()")
removed the last use of ovl_lock_rename_workdir() earlier, but in a
different branch.
e9c was committed upstream Nov 28th v6.18~7
833 was committed upstream Dec 1st v6.19-rc1~240
> Otherwise, feel free to add:
>
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Thanks,
NeilBrown
>
> Thanks,
> Amir.
>
^ permalink raw reply
* Re: [PATCH 03/13] libfs: change simple_done_creating() to use end_creating()
From: NeilBrown @ 2026-02-06 2:13 UTC (permalink / raw)
To: Jeff Layton
Cc: Christian Brauner, Alexander Viro, David Howells, Jan Kara,
Chuck Lever, Miklos Szeredi, Amir Goldstein, John Johansen,
Paul Moore, James Morris, Serge E. Hallyn, Stephen Smalley,
linux-kernel, netfs, linux-fsdevel, linux-nfs, linux-unionfs,
apparmor, linux-security-module, selinux
In-Reply-To: <8d907c67ccab1db0e7bcabe0c34c66722a2970e2.camel@kernel.org>
On Thu, 05 Feb 2026, Jeff Layton wrote:
> On Wed, 2026-02-04 at 15:57 +1100, NeilBrown wrote:
> > From: NeilBrown <neil@brown.name>
> >
> > simple_done_creating() and end_creating() are identical.
> > So change the former to use the latter. This further centralises
> > unlocking of directories.
> >
> > Signed-off-by: NeilBrown <neil@brown.name>
> > ---
> > fs/libfs.c | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/fs/libfs.c b/fs/libfs.c
> > index f1860dff86f2..db18b53fc189 100644
> > --- a/fs/libfs.c
> > +++ b/fs/libfs.c
> > @@ -2318,7 +2318,6 @@ EXPORT_SYMBOL(simple_start_creating);
> > /* parent must have been held exclusive since simple_start_creating() */
> > void simple_done_creating(struct dentry *child)
> > {
> > - inode_unlock(child->d_parent->d_inode);
> > - dput(child);
> > + end_creating(child);
> > }
> > EXPORT_SYMBOL(simple_done_creating);
>
> nit: seems like it would be better to turn this into a static inline
True ... but then it could have been a static inline anyway.
I'd rather not change it without good reason, or knowing what it was
written that way.
Al: do you have an opinion on this?
Thanks,
NeilBrown
^ permalink raw reply
* 黒字の幼児教室 投資オーナー募集
From: 幼児教室／ BBS インターナショナル @ 2026-02-06 2:28 UTC (permalink / raw)
To: linux-security-module
代表者様へ、突然のご連絡失礼いたします。
黒字運営の幼児教室オーナーとなり、
安定した収益と社会貢献を両立できる、投資オーナーを募集しています。
（運営やマネジメントの負担は一切ありません）
◇ 完全業務委託型｜黒字化教室のオーナーになれる ◇
全国200教室／業界最大規模の幼児教室ベビーパーク
◇ 詳細はこちら
https://bbs-i.net/2500a/
投資をする時、どんなポートフォリオを組んでいますか？
短期的には、利回りや回収時期が重要なのは当然ですが
長期的には、その投資が社会に与える影響を考えることが
より大きなリターンにつながる。と考える方も少なくありません。
特に、日本の未来の人材を育てる投資に
関心を持たれる方も増えています。
幼児教育は、日本の未来を支える事業と言えます。
事実、近年の大脳生理学の進歩により、
「人間の脳細胞は3歳で80%、6歳で90%完成する」という事実が明らかになり、
人生の基盤を決めるのは幼児期の教育であることが注目を集めています。
しかし、日本では幼児期の学びに対する意識が
十分に浸透していないという課題があります。
・30年間ゼロ成長という長期低迷
・不安定さを増す国際情勢
・社会保障制度への懸念
このような状況の中で、未来の日本を支えるのは、"優秀な人材"を生み出す教育です。
特に幼児教育の充実は、社会全体の成長につながる最も効率的な投資といえます。
このような未来を創るために、黒字運営の幼児教室オーナーになり、
収益を得ながら社会貢献できる仕組みを提供しています。
◇ 詳細はこちら
https://bbs-i.net/2500a/
◇ オーナー募集モデルの特徴 ◇
・完全業務委託型で、物件・人材・時間リソースなど一切不要
・すでに黒字化の教室への投資のため、投資直後から毎月のキャッシュフローを生み出す
・「売却オプション」付きで、明確な出口戦略を確保
このモデルにより、安定した収益と社会貢献の両立が実現できます。
また、分散投資の選択肢としても優れたリスクヘッジになります。
全国200教室／業界最大規模の幼児教室「ベビーパーク」のオーナーとして、
資産を増やしながら、社会貢献事業を始めませんか？
この機会に、ぜひ詳細資料をご覧ください。
最後までお読みいただき、誠にありがとうございました。
■□■───────────────────────────■□■
BBSインターナショナル株式会社
住所：東京都中央区日本橋小伝馬町2-5 メトロシティ小伝馬町7階
メールご不要の方は、下記よりお手続きをお願いいたします。
https://bbs-i.net/mail/
■□■───────────────────────────■□■
^ permalink raw reply
* [PATCH v2 v2] evm: check return values of crypto_shash functions
From: Daniel Hodges @ 2026-02-06 2:42 UTC (permalink / raw)
To: Roberto Sassu
Cc: zohar, roberto.sassu, dmitry.kasatkin, eric.snowberg, paul,
jmorris, serge, linux-integrity, linux-security-module,
linux-kernel, Daniel Hodges
In-Reply-To: <aYNprpzxppKE0Gf2@fb.com>
The crypto_shash_update() and crypto_shash_final() functions can fail
and return error codes, but their return values were not being checked
in several places in security/integrity/evm/evm_crypto.c:
- hmac_add_misc() ignored returns from crypto_shash_update() and
crypto_shash_final()
- evm_calc_hmac_or_hash() ignored returns from crypto_shash_update()
- evm_init_hmac() ignored returns from crypto_shash_update()
If these hash operations fail silently, the resulting HMAC could be
invalid or incomplete, which could weaken the integrity verification
security that EVM provides.
This patch converts hmac_add_misc() from void to int return type and
adds proper error checking and propagation for all crypto_shash_*
function calls. All callers are updated to handle the new return values.
Additionally, error messages are logged when cryptographic operations
fail to provide visibility into the failure rather than silently
returning error codes.
Fixes: 66dbc325afce ("evm: re-release")
Signed-off-by: Daniel Hodges <git@danielhodges.dev>
---
security/integrity/evm/evm_crypto.c | 55 ++++++++++++++++++++++-------
1 file changed, 42 insertions(+), 13 deletions(-)
diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index a5e730ffda57..402eb1ca64ce 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -139,7 +139,7 @@ static struct shash_desc *init_desc(char type, uint8_t hash_algo)
* (Additional directory/file metadata needs to be added for more complete
* protection.)
*/
-static void hmac_add_misc(struct shash_desc *desc, struct inode *inode,
+static int hmac_add_misc(struct shash_desc *desc, struct inode *inode,
char type, char *digest)
{
struct h_misc {
@@ -149,6 +149,7 @@ static void hmac_add_misc(struct shash_desc *desc, struct inode *inode,
gid_t gid;
umode_t mode;
} hmac_misc;
+ int error;
memset(&hmac_misc, 0, sizeof(hmac_misc));
/* Don't include the inode or generation number in portable
@@ -169,14 +170,28 @@ static void hmac_add_misc(struct shash_desc *desc, struct inode *inode,
hmac_misc.uid = from_kuid(&init_user_ns, inode->i_uid);
hmac_misc.gid = from_kgid(&init_user_ns, inode->i_gid);
hmac_misc.mode = inode->i_mode;
- crypto_shash_update(desc, (const u8 *)&hmac_misc, sizeof(hmac_misc));
+ error = crypto_shash_update(desc, (const u8 *)&hmac_misc, sizeof(hmac_misc));
+ if (error) {
+ pr_err("crypto_shash_update() failed: %d\n", error);
+ return error;
+ }
if ((evm_hmac_attrs & EVM_ATTR_FSUUID) &&
- type != EVM_XATTR_PORTABLE_DIGSIG)
- crypto_shash_update(desc, (u8 *)&inode->i_sb->s_uuid, UUID_SIZE);
- crypto_shash_final(desc, digest);
+ type != EVM_XATTR_PORTABLE_DIGSIG) {
+ error = crypto_shash_update(desc, (u8 *)&inode->i_sb->s_uuid, UUID_SIZE);
+ if (error) {
+ pr_err("crypto_shash_update() failed: %d\n", error);
+ return error;
+ }
+ }
+ error = crypto_shash_final(desc, digest);
+ if (error) {
+ pr_err("crypto_shash_final() failed: %d\n", error);
+ return error;
+ }
pr_debug("hmac_misc: (%zu) [%*phN]\n", sizeof(struct h_misc),
(int)sizeof(struct h_misc), &hmac_misc);
+ return 0;
}
/*
@@ -260,9 +275,12 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
if ((req_xattr_name && req_xattr_value)
&& !strcmp(xattr->name, req_xattr_name)) {
- error = 0;
- crypto_shash_update(desc, (const u8 *)req_xattr_value,
+ error = crypto_shash_update(desc, (const u8 *)req_xattr_value,
req_xattr_value_len);
+ if (error) {
+ pr_err("crypto_shash_update() failed: %d\n", error);
+ goto out;
+ }
if (is_ima)
ima_present = true;
@@ -286,15 +304,20 @@ static int evm_calc_hmac_or_hash(struct dentry *dentry,
pr_debug("file %s: xattr %s size mismatch (kernel: %d, user: %d)\n",
dentry->d_name.name, xattr->name, size,
user_space_size);
- error = 0;
xattr_size = size;
- crypto_shash_update(desc, (const u8 *)xattr_value, xattr_size);
+ error = crypto_shash_update(desc, (const u8 *)xattr_value, xattr_size);
+ if (error) {
+ pr_err("crypto_shash_update() failed: %d\n", error);
+ goto out;
+ }
if (is_ima)
ima_present = true;
dump_security_xattr(xattr->name, xattr_value, xattr_size);
}
- hmac_add_misc(desc, inode, type, data->digest);
+ error = hmac_add_misc(desc, inode, type, data->digest);
+ if (error)
+ goto out;
if (inode != d_backing_inode(dentry) && iint) {
if (IS_I_VERSION(inode))
@@ -401,6 +424,7 @@ int evm_init_hmac(struct inode *inode, const struct xattr *xattrs,
{
struct shash_desc *desc;
const struct xattr *xattr;
+ int error;
desc = init_desc(EVM_XATTR_HMAC, HASH_ALGO_SHA1);
if (IS_ERR(desc)) {
@@ -412,12 +436,17 @@ int evm_init_hmac(struct inode *inode, const struct xattr *xattrs,
if (!evm_protected_xattr(xattr->name))
continue;
- crypto_shash_update(desc, xattr->value, xattr->value_len);
+ error = crypto_shash_update(desc, xattr->value, xattr->value_len);
+ if (error) {
+ pr_err("crypto_shash_update() failed: %d\n", error);
+ goto out;
+ }
}
- hmac_add_misc(desc, inode, EVM_XATTR_HMAC, hmac_val);
+ error = hmac_add_misc(desc, inode, EVM_XATTR_HMAC, hmac_val);
+out:
kfree(desc);
- return 0;
+ return error;
}
/*
--
2.52.0
^ permalink raw reply related
* Re: [PATCH v2 0/3] landlock: Refactor layer masks
From: Günther Noack @ 2026-02-06 7:32 UTC (permalink / raw)
To: Mickaël Salaün
Cc: linux-security-module, Tingmao Wang, Justin Suess,
Samasth Norway Ananda, Matthieu Buffet, Mikhail Ivanov,
konstantin.meskhidze, Randy Dunlap
In-Reply-To: <20260128.jiethoh2Zeem@digikod.net>
Hello!
On Wed, Jan 28, 2026 at 10:31:07PM +0100, Mickaël Salaün wrote:
> On Sun, Jan 25, 2026 at 08:58:50PM +0100, Günther Noack wrote:
> > P.S.: I am open to suggestions on what the "layer masks" variables
> > should be called, because the name "layer masks" might be less
> > appropriate after this change. I have not fixed up the name
> > everywhere because fixing up the code took priority for now.
>
> Could you please clarify your thoughts and explain why this name might
> not be appropriate anymore? Any list of name proposals?
>
> If we rename the variables, this should be done in the same refactoring
> patch.
When this was an array of layer_mask_t, the name layer_masks was a
description of that underlying data type. Now that we have removed
the layer_mask_t datatype, it is not as obviously true any more.
When trying to name these variables after the "role" that they have in
their declaration context, I think of them as "unfulfilled per-layer
access requests", but that strikes me as a bit long.
For the upcoming patch set, I'm leaning towards naming these variables
just "masks", to keep it short.
> > Changes since previous versions:
> >
> > V2: (This patch set)
> >
> > * Remove the refactoring around the deny_mask_t type,
> > it is better to send that as a separate patch (mic review)
>
> Feel free to include the new dedicated patch in this series.
I'm afraid that this one did not get any further than what it already
was, and I'll have to leave it out for now. But I have it on my TODO
list.
–Günther
^ permalink raw reply
* Re: [PATCH v2 0/3] landlock: Refactor layer masks
From: Günther Noack @ 2026-02-06 7:49 UTC (permalink / raw)
To: Mickaël Salaün
Cc: linux-security-module, Tingmao Wang, Justin Suess,
Samasth Norway Ananda, Matthieu Buffet, Mikhail Ivanov,
konstantin.meskhidze, Randy Dunlap
In-Reply-To: <20260206.9509420815f5@gnoack.org>
On Fri, Feb 06, 2026 at 08:32:06AM +0100, Günther Noack wrote:
> On Wed, Jan 28, 2026 at 10:31:07PM +0100, Mickaël Salaün wrote:
> > On Sun, Jan 25, 2026 at 08:58:50PM +0100, Günther Noack wrote:
> > > P.S.: I am open to suggestions on what the "layer masks" variables
> > > should be called, because the name "layer masks" might be less
> > > appropriate after this change. I have not fixed up the name
> > > everywhere because fixing up the code took priority for now.
> >
> > Could you please clarify your thoughts and explain why this name might
> > not be appropriate anymore? Any list of name proposals?
> >
> > If we rename the variables, this should be done in the same refactoring
> > patch.
>
> When this was an array of layer_mask_t, the name layer_masks was a
> description of that underlying data type. Now that we have removed
> the layer_mask_t datatype, it is not as obviously true any more.
>
> When trying to name these variables after the "role" that they have in
> their declaration context, I think of them as "unfulfilled per-layer
> access requests", but that strikes me as a bit long.
>
> For the upcoming patch set, I'm leaning towards naming these variables
> just "masks", to keep it short.
OK, staring at the code a bit longer, I realize that since the type is
now named "struct layer_access_masks", "layer_masks" is actually still
a reasonable shorthand. I have abbreviated that to "masks" in some
places where it is anyway clear from the context that those are the
layer access masks, but left it as "layer_masks" in places where we
also use other access masks, for disambiguation.
–Günther
^ permalink raw reply
* Re: [PATCH v2 3/3] landlock: transpose the layer masks data structure
From: Günther Noack @ 2026-02-06 8:02 UTC (permalink / raw)
To: Mickaël Salaün
Cc: linux-security-module, Tingmao Wang, Justin Suess,
Samasth Norway Ananda, Matthieu Buffet, Mikhail Ivanov,
konstantin.meskhidze, Randy Dunlap
In-Reply-To: <20260129.xahm6Ue7raL3@digikod.net>
On Thu, Jan 29, 2026 at 05:54:01PM +0100, Mickaël Salaün wrote:
> On Thu, Jan 29, 2026 at 08:56:37AM +0100, Günther Noack wrote:
> > On Wed, Jan 28, 2026 at 10:34:02PM +0100, Mickaël Salaün wrote:
> > > > + for (int i = ARRAY_SIZE(masks->access) - 1; i >= 0; i--) {
> > >
> > > size_t i
> >
> > This is one of the two places where this didn't work.
> >
> > The loop goes from top to bottom here, and the "i >= 0" check would
> > always be true for a size_t.
> >
> > If there is a more idiomatic way to write that loop, I can switch to
> > it, but would otherwise lean towards keeping it as it is?
>
> Indeed. We can use ssize_t as in get_hierarchy().
Good point, done.
> > > > -static bool
> > > > -scope_to_request(const access_mask_t access_request,
> > > > - layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS])
> > > > +static bool scope_to_request(const access_mask_t access_request,
> > > > + struct layer_access_masks *masks)
> > > > {
> > > > - const unsigned long access_req = access_request;
> > > > - unsigned long access_bit;
> > > > + bool saw_unfulfilled_access = false;
> > > >
> > > > - if (WARN_ON_ONCE(!layer_masks))
> > > > + if (WARN_ON_ONCE(!masks))
> > > > return true;
> > > >
> > > > - for_each_clear_bit(access_bit, &access_req, ARRAY_SIZE(*layer_masks))
> > > > - (*layer_masks)[access_bit] = 0;
> > > > -
> > > > - return is_layer_masks_allowed(layer_masks);
> > > > + for (size_t i = 0; i < ARRAY_SIZE(masks->access); i++) {
> > > > + masks->access[i] &= access_request;
> > > > + if (masks->access[i])
> > >
> > > {
> > >
> > > > + saw_unfulfilled_access = true;
> > >
> > > break;
> > > }
> >
> > Two lines above, this loop mutates masks->access[...]:
> >
> > masks->access[i] &= access_request
> >
> > If we break the loop early, we would not actually scope it down to the
> > request entirely? Is this safe?
>
> You're right, don't add this break. BTW, would a test catch it?
Yes, the existing tests already catch that; this happens when we break early:
[08:53:12] ================= landlock_fs (7 subtests) =================
[08:53:12] [PASSED] test_no_more_access
[08:53:12] [PASSED] test_scope_to_request_with_exec_none
[08:53:12] # test_scope_to_request_with_exec_some: EXPECTATION FAILED at security/landlock/fs.c:616
[08:53:12] Expected 0 == masks.access[1], but
[08:53:12] masks.access[1] == 2 (0x2)
[08:53:12] [FAILED] test_scope_to_request_with_exec_some
[08:53:12] [PASSED] test_scope_to_request_without_access
[08:53:12] [PASSED] test_is_eacces_with_none
[08:53:12] [PASSED] test_is_eacces_with_refer
[08:53:12] [PASSED] test_is_eacces_with_write
[08:53:12] # module: landlock
[08:53:12] # landlock_fs: pass:6 fail:1 skip:0 total:7
[08:53:12] # Totals: pass:6 fail:1 skip:0 total:7
Good coverage!
–Günther
^ permalink raw reply
* Re: [PATCH v4 05/17] module: Switch load_info::len to size_t
From: David Howells @ 2026-02-06 8:18 UTC (permalink / raw)
To: =?utf-8?q?Thomas_Wei=C3=9Fschuh?=
Cc: dhowells, 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, Nicolas Schier,
Daniel Gomez, Aaron Tomlin, Christophe Leroy (CS GROUP),
Nicolas Schier, 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: <20260113-module-hashes-v4-5-0b932db9b56b@weissschuh.net>
Thomas Weißschuh <linux@weissschuh.net> wrote:
> As both 'size_t' and 'unsigned int' are always the same size, this
> should be risk-free.
Did you mean 'unsigned long'?
David
^ permalink raw reply
* Re: [PATCH v4 02/17] powerpc/ima: Drop unnecessary check for CONFIG_MODULE_SIG
From: Nicolas Schier @ 2026-02-06 8:25 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: <20260113-module-hashes-v4-2-0b932db9b56b@weissschuh.net>
On Tue, Jan 13, 2026 at 01:28:46PM +0100, Thomas Weißschuh wrote:
> When CONFIG_MODULE_SIG is disabled set_module_sig_enforced() is defined
> as an empty stub, so the check is unnecessary.
> The specific configuration option for set_module_sig_enforced() is
> about to change and removing the check avoids some later churn.
>
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
> arch/powerpc/kernel/ima_arch.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
Reviewed-by: Nicolas Schier <nsc@kernel.org>
--
Nicolas
^ permalink raw reply
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