* Re: [PATCH v5 13/36] bit_spinlock: Support Clang's context analysis
From: Marco Elver @ 2025-12-19 21:09 UTC (permalink / raw)
To: Bart Van Assche
Cc: Peter Zijlstra, Boqun Feng, Ingo Molnar, Will Deacon,
David S. Miller, Luc Van Oostenryck, Chris Li, Paul E. McKenney,
Alexander Potapenko, Arnd Bergmann, Christoph Hellwig,
Dmitry Vyukov, Eric Dumazet, Frederic Weisbecker,
Greg Kroah-Hartman, Herbert Xu, Ian Rogers, Jann Horn,
Joel Fernandes, Johannes Berg, Jonathan Corbet, Josh Triplett,
Justin Stitt, Kees Cook, Kentaro Takeda, Lukas Bulwahn,
Mark Rutland, Mathieu Desnoyers, Miguel Ojeda, Nathan Chancellor,
Neeraj Upadhyay, Nick Desaulniers, Steven Rostedt, Tetsuo Handa,
Thomas Gleixner, Thomas Graf, Uladzislau Rezki, Waiman Long,
kasan-dev, linux-crypto, linux-doc, linux-kbuild, linux-kernel,
linux-mm, linux-security-module, linux-sparse, linux-wireless,
llvm, rcu
In-Reply-To: <3b070057-5fda-410e-a047-d9061d56a82f@acm.org>
On Fri, 19 Dec 2025 at 21:48, 'Bart Van Assche' via kasan-dev
<kasan-dev@googlegroups.com> wrote:
>
> On 12/19/25 7:40 AM, Marco Elver wrote:
> > +/*
> > + * For static context analysis, we need a unique token for each possible bit
> > + * that can be used as a bit_spinlock. The easiest way to do that is to create a
> > + * fake context that we can cast to with the __bitlock(bitnum, addr) macro
> > + * below, which will give us unique instances for each (bit, addr) pair that the
> > + * static analysis can use.
> > + */
> > +context_lock_struct(__context_bitlock) { };
> > +#define __bitlock(bitnum, addr) (struct __context_bitlock *)(bitnum + (addr))
>
> Will this cause static analyzers to complain about out-of-bounds
> accesses for (bitnum + (addr)), which is equivalent to &(addr)[bitnum]?
Only if they decide to interpret never-executed code (i think the
kernel has various dead code that's optimized out that might trigger
static analyzers if they analyzed it).
But this could probably be improved by using a different idiom, and
using an empty inline function that takes bitnum, addr as args, and
Clang simply takes the call to that function as the context lock
identity.
^ permalink raw reply
* Re: [PATCH v5 22/36] um: Fix incorrect __acquires/__releases annotations
From: Bart Van Assche @ 2025-12-19 21:05 UTC (permalink / raw)
To: Marco Elver, Peter Zijlstra, Boqun Feng, Ingo Molnar, Will Deacon
Cc: David S. Miller, Luc Van Oostenryck, Chris Li, Paul E. McKenney,
Alexander Potapenko, Arnd Bergmann, Christoph Hellwig,
Dmitry Vyukov, Eric Dumazet, Frederic Weisbecker,
Greg Kroah-Hartman, Herbert Xu, Ian Rogers, Jann Horn,
Joel Fernandes, Johannes Berg, Jonathan Corbet, Josh Triplett,
Justin Stitt, Kees Cook, Kentaro Takeda, Lukas Bulwahn,
Mark Rutland, Mathieu Desnoyers, Miguel Ojeda, Nathan Chancellor,
Neeraj Upadhyay, Nick Desaulniers, Steven Rostedt, Tetsuo Handa,
Thomas Gleixner, Thomas Graf, Uladzislau Rezki, Waiman Long,
kasan-dev, linux-crypto, linux-doc, linux-kbuild, linux-kernel,
linux-mm, linux-security-module, linux-sparse, linux-wireless,
llvm, rcu, kernel test robot, Johannes Berg, Tiwei Bie
In-Reply-To: <20251219154418.3592607-23-elver@google.com>
On 12/19/25 7:40 AM, Marco Elver wrote:
> -void enter_turnstile(struct mm_id *mm_id) __acquires(turnstile)
> +struct mutex *__get_turnstile(struct mm_id *mm_id)
> {
> struct mm_context *ctx = container_of(mm_id, struct mm_context, id);
>
> - mutex_lock(&ctx->turnstile);
> + return &ctx->turnstile;
> }
Many "container_of()" wrappers have "to" in their name. Please follow
that convention and rename this function into e.g. mm_id_to_turnstile().
Thanks,
Bart.
^ permalink raw reply
* Re: [PATCH v5 08/36] locking/rwlock, spinlock: Support Clang's context analysis
From: Marco Elver @ 2025-12-19 21:02 UTC (permalink / raw)
To: Bart Van Assche
Cc: Peter Zijlstra, Boqun Feng, Ingo Molnar, Will Deacon,
David S. Miller, Luc Van Oostenryck, Chris Li, Paul E. McKenney,
Alexander Potapenko, Arnd Bergmann, Christoph Hellwig,
Dmitry Vyukov, Eric Dumazet, Frederic Weisbecker,
Greg Kroah-Hartman, Herbert Xu, Ian Rogers, Jann Horn,
Joel Fernandes, Johannes Berg, Jonathan Corbet, Josh Triplett,
Justin Stitt, Kees Cook, Kentaro Takeda, Lukas Bulwahn,
Mark Rutland, Mathieu Desnoyers, Miguel Ojeda, Nathan Chancellor,
Neeraj Upadhyay, Nick Desaulniers, Steven Rostedt, Tetsuo Handa,
Thomas Gleixner, Thomas Graf, Uladzislau Rezki, Waiman Long,
kasan-dev, linux-crypto, linux-doc, linux-kbuild, linux-kernel,
linux-mm, linux-security-module, linux-sparse, linux-wireless,
llvm, rcu
In-Reply-To: <17723ae6-9611-4731-905c-60dab9fb7102@acm.org>
On Fri, 19 Dec 2025 at 21:26, Bart Van Assche <bvanassche@acm.org> wrote:
> On 12/19/25 7:39 AM, Marco Elver wrote:
> > - extern void do_raw_read_lock(rwlock_t *lock) __acquires(lock);
> > + extern void do_raw_read_lock(rwlock_t *lock) __acquires_shared(lock);
>
> Given the "one change per patch" rule, shouldn't the annotation fixes
> for rwlock operations be moved into a separate patch?
>
> > -typedef struct {
> > +context_lock_struct(rwlock) {
> > arch_rwlock_t raw_lock;
> > #ifdef CONFIG_DEBUG_SPINLOCK
> > unsigned int magic, owner_cpu;
> > @@ -31,7 +31,8 @@ typedef struct {
> > #ifdef CONFIG_DEBUG_LOCK_ALLOC
> > struct lockdep_map dep_map;
> > #endif
> > -} rwlock_t;
> > +};
> > +typedef struct rwlock rwlock_t;
>
> This change introduces a new globally visible "struct rwlock". Although
> I haven't found any existing "struct rwlock" definitions, maybe it's a
> good idea to use a more unique name instead.
This doesn't actually introduce a new globally visible "struct
rwlock", it's already the case before.
An inlined struct definition in a typedef is available by its struct
name, so this is not introducing a new name
(https://godbolt.org/z/Y1jf66e1M).
> > diff --git a/include/linux/spinlock_api_up.h b/include/linux/spinlock_api_up.h
> > index 819aeba1c87e..018f5aabc1be 100644
> > --- a/include/linux/spinlock_api_up.h
> > +++ b/include/linux/spinlock_api_up.h
> > @@ -24,68 +24,77 @@
> > * flags straight, to suppress compiler warnings of unused lock
> > * variables, and to add the proper checker annotations:
> > */
> > -#define ___LOCK(lock) \
> > - do { __acquire(lock); (void)(lock); } while (0)
> > +#define ___LOCK_void(lock) \
> > + do { (void)(lock); } while (0)
>
> Instead of introducing a new macro ___LOCK_void(), please expand this
> macro where it is used ((void)(lock)). I think this will make the code
> in this header file easier to read.
If I recall right, we needed this to generalize __LOCK(),
__LOCK_IRQ(), etc. which do preempt_disable(), local_irq_disable() in
the right way, but then need to make sure we call the right
acquire/release helper, which require different cases depending on the
lock kind. Obviously we could just expand all the macros below, but
the current pattern tried to not rewrite this altogether.
There's probably a way this can all be simplified for UP, but maybe a
separate patch. I'd leave it to the locking maintainers which way they
prefer to go.
> > -#define __LOCK(lock) \
> > - do { preempt_disable(); ___LOCK(lock); } while (0)
> > +#define ___LOCK_(lock) \
> > + do { __acquire(lock); ___LOCK_void(lock); } while (0)
>
> Is the macro ___LOCK_() used anywhere? If not, can it be left out?
Yes, it's the default case if __VA_ARGS__ is empty.
> > -#define __LOCK_BH(lock) \
> > - do { __local_bh_disable_ip(_THIS_IP_, SOFTIRQ_LOCK_OFFSET); ___LOCK(lock); } while (0)
> > +#define ___LOCK_shared(lock) \
> > + do { __acquire_shared(lock); ___LOCK_void(lock); } while (0)
>
> The introduction of the new macros in this header file make the changes
> hard to follow. Please consider splitting the changes for this header
> file as follows:
> * A first patch that splits ___LOCK() into ___LOCK_exclusive() and
> ___LOCK_shared().
> * A second patch with the thread-safety annotation changes
> (__acquire() -> __acquire_shared()).
I've wrangled with this maze of interdependent macros and definitions
for days (though that was earlier in the year), believe me when I say
I tried to split it up. I think the commit message hints at this:
> Add support for Clang's context analysis for raw_spinlock_t,
> spinlock_t, and rwlock. This wholesale conversion is required because
> all three of them are interdependent.
It's like a carefully crafted house of cards: you take one away, the
whole thing breaks apart. If I recall correctly, the main problem was
that as soon as you make one of these a context lock type, and because
they are all interdependent, the compiler will just complain endlessly
about either wrong attributes or incorrectly acquired/released locks
until they are all precisely in the way you see them here.
> > /* Non PREEMPT_RT kernels map spinlock to raw_spinlock */
> > -typedef struct spinlock {
> > +context_lock_struct(spinlock) {
> > union {
> > struct raw_spinlock rlock;
> >
> > @@ -26,7 +26,8 @@ typedef struct spinlock {
> > };
> > #endif
> > };
> > -} spinlock_t;
> > +};
> > +typedef struct spinlock spinlock_t;
>
> Also here, a new global struct name is introduced (spinlock). Maybe the
> name of this new struct should be made more unique?
As above.
^ permalink raw reply
* Re: [PATCH v5 21/36] debugfs: Make debugfs_cancellation a context lock struct
From: Bart Van Assche @ 2025-12-19 21:01 UTC (permalink / raw)
To: Marco Elver, Peter Zijlstra, Boqun Feng, Ingo Molnar, Will Deacon
Cc: David S. Miller, Luc Van Oostenryck, Chris Li, Paul E. McKenney,
Alexander Potapenko, Arnd Bergmann, Christoph Hellwig,
Dmitry Vyukov, Eric Dumazet, Frederic Weisbecker,
Greg Kroah-Hartman, Herbert Xu, Ian Rogers, Jann Horn,
Joel Fernandes, Johannes Berg, Jonathan Corbet, Josh Triplett,
Justin Stitt, Kees Cook, Kentaro Takeda, Lukas Bulwahn,
Mark Rutland, Mathieu Desnoyers, Miguel Ojeda, Nathan Chancellor,
Neeraj Upadhyay, Nick Desaulniers, Steven Rostedt, Tetsuo Handa,
Thomas Gleixner, Thomas Graf, Uladzislau Rezki, Waiman Long,
kasan-dev, linux-crypto, linux-doc, linux-kbuild, linux-kernel,
linux-mm, linux-security-module, linux-sparse, linux-wireless,
llvm, rcu
In-Reply-To: <20251219154418.3592607-22-elver@google.com>
On 12/19/25 7:40 AM, Marco Elver wrote:
> When compiling include/linux/debugfs.h with CONTEXT_ANALYSIS enabled, we
> can see this error:
>
> ./include/linux/debugfs.h:239:17: error: use of undeclared identifier 'cancellation'
> 239 | void __acquires(cancellation)
>
> Move the __acquires(..) attribute after the declaration, so that the
> compiler can see the cancellation function argument, as well as making
> struct debugfs_cancellation a real context lock to benefit from Clang's
> context analysis.
>
> This change is a preparatory change to allow enabling context analysis
> in subsystems that include the above header.
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
^ permalink raw reply
* Re: [PATCH v5 18/36] locking/local_lock: Include missing headers
From: Bart Van Assche @ 2025-12-19 20:56 UTC (permalink / raw)
To: Marco Elver, Peter Zijlstra, Boqun Feng, Ingo Molnar, Will Deacon
Cc: David S. Miller, Luc Van Oostenryck, Chris Li, Paul E. McKenney,
Alexander Potapenko, Arnd Bergmann, Christoph Hellwig,
Dmitry Vyukov, Eric Dumazet, Frederic Weisbecker,
Greg Kroah-Hartman, Herbert Xu, Ian Rogers, Jann Horn,
Joel Fernandes, Johannes Berg, Jonathan Corbet, Josh Triplett,
Justin Stitt, Kees Cook, Kentaro Takeda, Lukas Bulwahn,
Mark Rutland, Mathieu Desnoyers, Miguel Ojeda, Nathan Chancellor,
Neeraj Upadhyay, Nick Desaulniers, Steven Rostedt, Tetsuo Handa,
Thomas Gleixner, Thomas Graf, Uladzislau Rezki, Waiman Long,
kasan-dev, linux-crypto, linux-doc, linux-kbuild, linux-kernel,
linux-mm, linux-security-module, linux-sparse, linux-wireless,
llvm, rcu
In-Reply-To: <20251219154418.3592607-19-elver@google.com>
On 12/19/25 7:40 AM, Marco Elver wrote:
> Including <linux/local_lock.h> into an empty TU will result in the
> compiler complaining:
>
> ./include/linux/local_lock.h: In function ‘class_local_lock_irqsave_constructor’:
> ./include/linux/local_lock_internal.h:95:17: error: implicit declaration of function ‘local_irq_save’; <...>
> 95 | local_irq_save(flags); \
> | ^~~~~~~~~~~~~~
>
> As well as (some architectures only, such as 'sh'):
>
> ./include/linux/local_lock_internal.h: In function ‘local_lock_acquire’:
> ./include/linux/local_lock_internal.h:33:20: error: ‘current’ undeclared (first use in this function)
> 33 | l->owner = current;
>
> Include missing headers to allow including local_lock.h where the
> required headers are not otherwise included.
>
> Signed-off-by: Marco Elver <elver@google.com>
> ---
> include/linux/local_lock_internal.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/include/linux/local_lock_internal.h b/include/linux/local_lock_internal.h
> index 8f82b4eb542f..1a1ea1232add 100644
> --- a/include/linux/local_lock_internal.h
> +++ b/include/linux/local_lock_internal.h
> @@ -4,7 +4,9 @@
> #endif
>
> #include <linux/percpu-defs.h>
> +#include <linux/irqflags.h>
> #include <linux/lockdep.h>
> +#include <asm/current.h>
>
> #ifndef CONFIG_PREEMPT_RT
>
The abbreviation "TU" is uncommon. Hence, please expand that
abbreviation. Anyway:
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
^ permalink raw reply
* Re: [PATCH v5 17/36] locking/rwsem: Support Clang's context analysis
From: Bart Van Assche @ 2025-12-19 20:55 UTC (permalink / raw)
To: Marco Elver, Peter Zijlstra, Boqun Feng, Ingo Molnar, Will Deacon
Cc: David S. Miller, Luc Van Oostenryck, Chris Li, Paul E. McKenney,
Alexander Potapenko, Arnd Bergmann, Christoph Hellwig,
Dmitry Vyukov, Eric Dumazet, Frederic Weisbecker,
Greg Kroah-Hartman, Herbert Xu, Ian Rogers, Jann Horn,
Joel Fernandes, Johannes Berg, Jonathan Corbet, Josh Triplett,
Justin Stitt, Kees Cook, Kentaro Takeda, Lukas Bulwahn,
Mark Rutland, Mathieu Desnoyers, Miguel Ojeda, Nathan Chancellor,
Neeraj Upadhyay, Nick Desaulniers, Steven Rostedt, Tetsuo Handa,
Thomas Gleixner, Thomas Graf, Uladzislau Rezki, Waiman Long,
kasan-dev, linux-crypto, linux-doc, linux-kbuild, linux-kernel,
linux-mm, linux-security-module, linux-sparse, linux-wireless,
llvm, rcu
In-Reply-To: <20251219154418.3592607-18-elver@google.com>
On 12/19/25 7:40 AM, Marco Elver wrote:
> static inline void rwsem_assert_held_nolockdep(const struct rw_semaphore *sem)
> + __assumes_ctx_lock(sem)
> {
> WARN_ON(atomic_long_read(&sem->count) == RWSEM_UNLOCKED_VALUE);
> }
>
> static inline void rwsem_assert_held_write_nolockdep(const struct rw_semaphore *sem)
> + __assumes_ctx_lock(sem)
> {
> WARN_ON(!(atomic_long_read(&sem->count) & RWSEM_WRITER_LOCKED));
> }
> @@ -119,6 +121,7 @@ do { \
> static struct lock_class_key __key; \
> \
> __init_rwsem((sem), #sem, &__key); \
> + __assume_ctx_lock(sem); \
> } while (0)
Just like as for lockdep.h, I think that the above annotations should be
changed into __must_hold().
Thanks,
Bart.
^ permalink raw reply
* Re: [PATCH v8 07/12] KEYS: trusted: Remove dead branch from tpm2_unseal_cmd
From: James Bottomley @ 2025-12-19 20:54 UTC (permalink / raw)
To: Jarkko Sakkinen, linux-integrity
Cc: David Howells, Paul Moore, James Morris, Serge E. Hallyn,
Mimi Zohar, open list:KEYS/KEYRINGS, open list:SECURITY SUBSYSTEM,
open list
In-Reply-To: <20251216092147.2326606-8-jarkko@kernel.org>
On Tue, 2025-12-16 at 11:21 +0200, Jarkko Sakkinen wrote:
> TPM2_Unseal requires TPM2_ST_SESSIONS, and tpm2_unseal_cmd() always
> does set up either password or HMAC session.
>
> Remove the branch in tpm2_unseal_cmd() conditionally setting
> TPM2_ST_NO_SESSIONS. It is faulty but luckily it is never exercised
> at run-time, and thus does not cause regressions.
Shouldn't that also be
Fixes: b7960b904861 ("tpm2-sessions: Open code tpm_buf_append_hmac_session()")
> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org>
> ---
> security/keys/trusted-keys/trusted_tpm2.c | 10 +---------
> 1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/security/keys/trusted-keys/trusted_tpm2.c
> b/security/keys/trusted-keys/trusted_tpm2.c
> index d3a5c5f2b926..3666e3e48eab 100644
> --- a/security/keys/trusted-keys/trusted_tpm2.c
> +++ b/security/keys/trusted-keys/trusted_tpm2.c
> @@ -451,10 +451,8 @@ static int tpm2_unseal_cmd(struct tpm_chip
> *chip,
> struct trusted_key_options *options,
> u32 blob_handle)
> {
> - struct tpm_header *head;
> struct tpm_buf buf;
> u16 data_len;
> - int offset;
> u8 *data;
> int rc;
>
> @@ -495,14 +493,8 @@ static int tpm2_unseal_cmd(struct tpm_chip
> *chip,
> tpm_buf_append_u16(&buf, options->blobauth_len);
> tpm_buf_append(&buf, options->blobauth, options-
> >blobauth_len);
>
> - if (tpm2_chip_auth(chip)) {
> + if (tpm2_chip_auth(chip))
Since the statement above is that the if is always true, why do you
still have it here?
Regards,
James
^ permalink raw reply
* Re: [PATCH v5 07/36] lockdep: Annotate lockdep assertions for context analysis
From: Bart Van Assche @ 2025-12-19 20:53 UTC (permalink / raw)
To: Marco Elver, Peter Zijlstra, Boqun Feng, Ingo Molnar, Will Deacon
Cc: David S. Miller, Luc Van Oostenryck, Chris Li, Paul E. McKenney,
Alexander Potapenko, Arnd Bergmann, Christoph Hellwig,
Dmitry Vyukov, Eric Dumazet, Frederic Weisbecker,
Greg Kroah-Hartman, Herbert Xu, Ian Rogers, Jann Horn,
Joel Fernandes, Johannes Berg, Jonathan Corbet, Josh Triplett,
Justin Stitt, Kees Cook, Kentaro Takeda, Lukas Bulwahn,
Mark Rutland, Mathieu Desnoyers, Miguel Ojeda, Nathan Chancellor,
Neeraj Upadhyay, Nick Desaulniers, Steven Rostedt, Tetsuo Handa,
Thomas Gleixner, Thomas Graf, Uladzislau Rezki, Waiman Long,
kasan-dev, linux-crypto, linux-doc, linux-kbuild, linux-kernel,
linux-mm, linux-security-module, linux-sparse, linux-wireless,
llvm, rcu
In-Reply-To: <20251219154418.3592607-8-elver@google.com>
On 12/19/25 7:39 AM, Marco Elver wrote:
> diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
> index dd634103b014..621566345406 100644
> --- a/include/linux/lockdep.h
> +++ b/include/linux/lockdep.h
> @@ -282,16 +282,16 @@ extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie);
> do { WARN_ON_ONCE(debug_locks && !(cond)); } while (0)
>
> #define lockdep_assert_held(l) \
> - lockdep_assert(lockdep_is_held(l) != LOCK_STATE_NOT_HELD)
> + do { lockdep_assert(lockdep_is_held(l) != LOCK_STATE_NOT_HELD); __assume_ctx_lock(l); } while (0)
>
> #define lockdep_assert_not_held(l) \
> lockdep_assert(lockdep_is_held(l) != LOCK_STATE_HELD)
>
> #define lockdep_assert_held_write(l) \
> - lockdep_assert(lockdep_is_held_type(l, 0))
> + do { lockdep_assert(lockdep_is_held_type(l, 0)); __assume_ctx_lock(l); } while (0)
>
> #define lockdep_assert_held_read(l) \
> - lockdep_assert(lockdep_is_held_type(l, 1))
> + do { lockdep_assert(lockdep_is_held_type(l, 1)); __assume_shared_ctx_lock(l); } while (0)
>
> #define lockdep_assert_held_once(l) \
> lockdep_assert_once(lockdep_is_held(l) != LOCK_STATE_NOT_HELD)
> @@ -389,10 +389,10 @@ extern int lockdep_is_held(const void *);
> #define lockdep_assert(c) do { } while (0)
> #define lockdep_assert_once(c) do { } while (0)
>
> -#define lockdep_assert_held(l) do { (void)(l); } while (0)
> +#define lockdep_assert_held(l) __assume_ctx_lock(l)
> #define lockdep_assert_not_held(l) do { (void)(l); } while (0)
> -#define lockdep_assert_held_write(l) do { (void)(l); } while (0)
> -#define lockdep_assert_held_read(l) do { (void)(l); } while (0)
> +#define lockdep_assert_held_write(l) __assume_ctx_lock(l)
> +#define lockdep_assert_held_read(l) __assume_shared_ctx_lock(l)
> #define lockdep_assert_held_once(l) do { (void)(l); } while (0)
> #define lockdep_assert_none_held_once() do { } while (0)
I think these macros should use __must_hold() instead of __assume...().
lockdep_assert_held() emits a runtime warning if 'l' is not held. Hence,
I think that code where lockdep_assert_held() is used should not compile
if it cannot be verified at compile time that 'l' is held.
Thanks,
Bart.
^ permalink raw reply
* Re: [PATCH v5 16/36] kref: Add context-analysis annotations
From: Bart Van Assche @ 2025-12-19 20:49 UTC (permalink / raw)
To: Marco Elver, Peter Zijlstra, Boqun Feng, Ingo Molnar, Will Deacon
Cc: David S. Miller, Luc Van Oostenryck, Chris Li, Paul E. McKenney,
Alexander Potapenko, Arnd Bergmann, Christoph Hellwig,
Dmitry Vyukov, Eric Dumazet, Frederic Weisbecker,
Greg Kroah-Hartman, Herbert Xu, Ian Rogers, Jann Horn,
Joel Fernandes, Johannes Berg, Jonathan Corbet, Josh Triplett,
Justin Stitt, Kees Cook, Kentaro Takeda, Lukas Bulwahn,
Mark Rutland, Mathieu Desnoyers, Miguel Ojeda, Nathan Chancellor,
Neeraj Upadhyay, Nick Desaulniers, Steven Rostedt, Tetsuo Handa,
Thomas Gleixner, Thomas Graf, Uladzislau Rezki, Waiman Long,
kasan-dev, linux-crypto, linux-doc, linux-kbuild, linux-kernel,
linux-mm, linux-security-module, linux-sparse, linux-wireless,
llvm, rcu
In-Reply-To: <20251219154418.3592607-17-elver@google.com>
On 12/19/25 7:40 AM, Marco Elver wrote:
> Mark functions that conditionally acquire the passed lock.
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
^ permalink raw reply
* Re: [PATCH v5 13/36] bit_spinlock: Support Clang's context analysis
From: Bart Van Assche @ 2025-12-19 20:47 UTC (permalink / raw)
To: Marco Elver, Peter Zijlstra, Boqun Feng, Ingo Molnar, Will Deacon
Cc: David S. Miller, Luc Van Oostenryck, Chris Li, Paul E. McKenney,
Alexander Potapenko, Arnd Bergmann, Christoph Hellwig,
Dmitry Vyukov, Eric Dumazet, Frederic Weisbecker,
Greg Kroah-Hartman, Herbert Xu, Ian Rogers, Jann Horn,
Joel Fernandes, Johannes Berg, Jonathan Corbet, Josh Triplett,
Justin Stitt, Kees Cook, Kentaro Takeda, Lukas Bulwahn,
Mark Rutland, Mathieu Desnoyers, Miguel Ojeda, Nathan Chancellor,
Neeraj Upadhyay, Nick Desaulniers, Steven Rostedt, Tetsuo Handa,
Thomas Gleixner, Thomas Graf, Uladzislau Rezki, Waiman Long,
kasan-dev, linux-crypto, linux-doc, linux-kbuild, linux-kernel,
linux-mm, linux-security-module, linux-sparse, linux-wireless,
llvm, rcu
In-Reply-To: <20251219154418.3592607-14-elver@google.com>
On 12/19/25 7:40 AM, Marco Elver wrote:
> +/*
> + * For static context analysis, we need a unique token for each possible bit
> + * that can be used as a bit_spinlock. The easiest way to do that is to create a
> + * fake context that we can cast to with the __bitlock(bitnum, addr) macro
> + * below, which will give us unique instances for each (bit, addr) pair that the
> + * static analysis can use.
> + */
> +context_lock_struct(__context_bitlock) { };
> +#define __bitlock(bitnum, addr) (struct __context_bitlock *)(bitnum + (addr))
Will this cause static analyzers to complain about out-of-bounds
accesses for (bitnum + (addr)), which is equivalent to &(addr)[bitnum]?
Thanks,
Bart.
^ permalink raw reply
* Re: [PATCH v5 12/36] bit_spinlock: Include missing <asm/processor.h>
From: Bart Van Assche @ 2025-12-19 20:38 UTC (permalink / raw)
To: Marco Elver, Peter Zijlstra, Boqun Feng, Ingo Molnar, Will Deacon
Cc: David S. Miller, Luc Van Oostenryck, Chris Li, Paul E. McKenney,
Alexander Potapenko, Arnd Bergmann, Christoph Hellwig,
Dmitry Vyukov, Eric Dumazet, Frederic Weisbecker,
Greg Kroah-Hartman, Herbert Xu, Ian Rogers, Jann Horn,
Joel Fernandes, Johannes Berg, Jonathan Corbet, Josh Triplett,
Justin Stitt, Kees Cook, Kentaro Takeda, Lukas Bulwahn,
Mark Rutland, Mathieu Desnoyers, Miguel Ojeda, Nathan Chancellor,
Neeraj Upadhyay, Nick Desaulniers, Steven Rostedt, Tetsuo Handa,
Thomas Gleixner, Thomas Graf, Uladzislau Rezki, Waiman Long,
kasan-dev, linux-crypto, linux-doc, linux-kbuild, linux-kernel,
linux-mm, linux-security-module, linux-sparse, linux-wireless,
llvm, rcu
In-Reply-To: <20251219154418.3592607-13-elver@google.com>
On 12/19/25 7:40 AM, Marco Elver wrote:
> Including <linux/bit_spinlock.h> into an empty TU will result in the
> compiler complaining:
>
> ./include/linux/bit_spinlock.h:34:4: error: call to undeclared function 'cpu_relax'; <...>
> 34 | cpu_relax();
> | ^
> 1 error generated.
>
> Include <asm/processor.h> to allow including bit_spinlock.h where
> <asm/processor.h> is not otherwise included.
>
> Signed-off-by: Marco Elver <elver@google.com>
> ---
> include/linux/bit_spinlock.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/include/linux/bit_spinlock.h b/include/linux/bit_spinlock.h
> index c0989b5b0407..59e345f74b0e 100644
> --- a/include/linux/bit_spinlock.h
> +++ b/include/linux/bit_spinlock.h
> @@ -7,6 +7,8 @@
> #include <linux/atomic.h>
> #include <linux/bug.h>
>
> +#include <asm/processor.h> /* for cpu_relax() */
> +
> /*
> * bit-based spin_lock()
> *
The abbreviation "TU" is uncommon so it's probably a good idea to expand
it. Anyway:
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
^ permalink raw reply
* Re: [PATCH] selftests/landlock: NULL-terminate unix pathname addresses
From: Mickaël Salaün @ 2025-12-19 20:26 UTC (permalink / raw)
To: Günther Noack; +Cc: Matthieu Buffet, linux-security-module
In-Reply-To: <aTAHfes90YxsXWt0@google.com>
Thanks for both patches! Applied to my -next branch.
On Wed, Dec 03, 2025 at 10:48:45AM +0100, Günther Noack wrote:
> On Tue, Dec 02, 2025 at 10:51:41PM +0100, Matthieu Buffet wrote:
> > The size of Unix pathname addresses is computed in selftests using
> > offsetof(struct sockaddr_un, sun_path) + strlen(xxx). It should have
> > been that +1, which makes addresses passed to the libc and kernel
> > non-NULL-terminated. unix_mkname_bsd() fixes that in Linux so there is
> > no harm, but just using sizeof(the address struct) should improve
> > readability.
> >
> > Signed-off-by: Matthieu Buffet <matthieu@buffet.re>
> > ---
> > [...]
>
> Thank you very much, this looks good!
>
> Reviewed-by: Günther Noack <gnoack@google.com>
>
> —Günther
>
^ permalink raw reply
* Re: [PATCH v5 08/36] locking/rwlock, spinlock: Support Clang's context analysis
From: Bart Van Assche @ 2025-12-19 20:26 UTC (permalink / raw)
To: Marco Elver, Peter Zijlstra, Boqun Feng, Ingo Molnar, Will Deacon
Cc: David S. Miller, Luc Van Oostenryck, Chris Li, Paul E. McKenney,
Alexander Potapenko, Arnd Bergmann, Christoph Hellwig,
Dmitry Vyukov, Eric Dumazet, Frederic Weisbecker,
Greg Kroah-Hartman, Herbert Xu, Ian Rogers, Jann Horn,
Joel Fernandes, Johannes Berg, Jonathan Corbet, Josh Triplett,
Justin Stitt, Kees Cook, Kentaro Takeda, Lukas Bulwahn,
Mark Rutland, Mathieu Desnoyers, Miguel Ojeda, Nathan Chancellor,
Neeraj Upadhyay, Nick Desaulniers, Steven Rostedt, Tetsuo Handa,
Thomas Gleixner, Thomas Graf, Uladzislau Rezki, Waiman Long,
kasan-dev, linux-crypto, linux-doc, linux-kbuild, linux-kernel,
linux-mm, linux-security-module, linux-sparse, linux-wireless,
llvm, rcu
In-Reply-To: <20251219154418.3592607-9-elver@google.com>
On 12/19/25 7:39 AM, Marco Elver wrote:
> - extern void do_raw_read_lock(rwlock_t *lock) __acquires(lock);
> + extern void do_raw_read_lock(rwlock_t *lock) __acquires_shared(lock);
Given the "one change per patch" rule, shouldn't the annotation fixes
for rwlock operations be moved into a separate patch?
> -typedef struct {
> +context_lock_struct(rwlock) {
> arch_rwlock_t raw_lock;
> #ifdef CONFIG_DEBUG_SPINLOCK
> unsigned int magic, owner_cpu;
> @@ -31,7 +31,8 @@ typedef struct {
> #ifdef CONFIG_DEBUG_LOCK_ALLOC
> struct lockdep_map dep_map;
> #endif
> -} rwlock_t;
> +};
> +typedef struct rwlock rwlock_t;
This change introduces a new globally visible "struct rwlock". Although
I haven't found any existing "struct rwlock" definitions, maybe it's a
good idea to use a more unique name instead.
> diff --git a/include/linux/spinlock_api_up.h b/include/linux/spinlock_api_up.h
> index 819aeba1c87e..018f5aabc1be 100644
> --- a/include/linux/spinlock_api_up.h
> +++ b/include/linux/spinlock_api_up.h
> @@ -24,68 +24,77 @@
> * flags straight, to suppress compiler warnings of unused lock
> * variables, and to add the proper checker annotations:
> */
> -#define ___LOCK(lock) \
> - do { __acquire(lock); (void)(lock); } while (0)
> +#define ___LOCK_void(lock) \
> + do { (void)(lock); } while (0)
Instead of introducing a new macro ___LOCK_void(), please expand this
macro where it is used ((void)(lock)). I think this will make the code
in this header file easier to read.
> -#define __LOCK(lock) \
> - do { preempt_disable(); ___LOCK(lock); } while (0)
> +#define ___LOCK_(lock) \
> + do { __acquire(lock); ___LOCK_void(lock); } while (0)
Is the macro ___LOCK_() used anywhere? If not, can it be left out?
> -#define __LOCK_BH(lock) \
> - do { __local_bh_disable_ip(_THIS_IP_, SOFTIRQ_LOCK_OFFSET); ___LOCK(lock); } while (0)
> +#define ___LOCK_shared(lock) \
> + do { __acquire_shared(lock); ___LOCK_void(lock); } while (0)
The introduction of the new macros in this header file make the changes
hard to follow. Please consider splitting the changes for this header
file as follows:
* A first patch that splits ___LOCK() into ___LOCK_exclusive() and
___LOCK_shared().
* A second patch with the thread-safety annotation changes
(__acquire() -> __acquire_shared()).
> /* Non PREEMPT_RT kernels map spinlock to raw_spinlock */
> -typedef struct spinlock {
> +context_lock_struct(spinlock) {
> union {
> struct raw_spinlock rlock;
>
> @@ -26,7 +26,8 @@ typedef struct spinlock {
> };
> #endif
> };
> -} spinlock_t;
> +};
> +typedef struct spinlock spinlock_t;
Also here, a new global struct name is introduced (spinlock). Maybe the
name of this new struct should be made more unique?
Thanks,
Bart.
^ permalink raw reply
* [PATCH] ima: Fix stack-out-of-bounds in is_bprm_creds_for_exec()
From: Chris J Arges @ 2025-12-19 19:54 UTC (permalink / raw)
To: zohar, roberto.sassu
Cc: kernel-team, Chris J Arges, Dmitry Kasatkin, Eric Snowberg,
Paul Moore, James Morris, Serge E. Hallyn,
Mickaël Salaün, Kees Cook, linux-integrity,
linux-security-module, linux-kernel
KASAN reported a stack-out-of-bounds access in ima_appraise_measurement
from is_bprm_creds_for_exec:
BUG: KASAN: stack-out-of-bounds in ima_appraise_measurement+0x12dc/0x16a0
Read of size 1 at addr ffffc9000160f940 by task sudo/550
The buggy address belongs to stack of task sudo/550
and is located at offset 24 in frame:
ima_appraise_measurement+0x0/0x16a0
This frame has 2 objects:
[48, 56) 'file'
[80, 148) 'hash'
This is caused by using container_of on the *file pointer which by the time
this function is called is actually a stack variable.
In order to fix this pass in a bprm_is_check boolean which can be set
depending on how process_measurement is called. If the caller has a
linux_binprm pointer we can determine is_check and set it then. Otherwise
set it to false.
Fixes: 95b3cdafd7cb7 ("ima: instantiate the bprm_creds_for_exec() hook")
Signed-off-by: Chris J Arges <carges@cloudflare.com>
---
security/integrity/ima/ima.h | 2 +-
security/integrity/ima/ima_appraise.c | 15 ++-------------
security/integrity/ima/ima_main.c | 18 +++++++++---------
3 files changed, 12 insertions(+), 23 deletions(-)
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index e3d71d8d56e3..5c9f244ed1d6 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -441,7 +441,7 @@ int ima_check_blacklist(struct ima_iint_cache *iint,
int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint,
struct file *file, const unsigned char *filename,
struct evm_ima_xattr_data *xattr_value,
- int xattr_len, const struct modsig *modsig);
+ int xattr_len, const struct modsig *modsig, bool bprm_is_check);
int ima_must_appraise(struct mnt_idmap *idmap, struct inode *inode,
int mask, enum ima_hooks func);
void ima_update_xattr(struct ima_iint_cache *iint, struct file *file);
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 5149ff4fd50d..ea2079417318 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -470,17 +470,6 @@ int ima_check_blacklist(struct ima_iint_cache *iint,
return rc;
}
-static bool is_bprm_creds_for_exec(enum ima_hooks func, struct file *file)
-{
- struct linux_binprm *bprm;
-
- if (func == BPRM_CHECK) {
- bprm = container_of(&file, struct linux_binprm, file);
- return bprm->is_check;
- }
- return false;
-}
-
/*
* ima_appraise_measurement - appraise file measurement
*
@@ -492,7 +481,7 @@ static bool is_bprm_creds_for_exec(enum ima_hooks func, struct file *file)
int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint,
struct file *file, const unsigned char *filename,
struct evm_ima_xattr_data *xattr_value,
- int xattr_len, const struct modsig *modsig)
+ int xattr_len, const struct modsig *modsig, bool bprm_is_check)
{
static const char op[] = "appraise_data";
int audit_msgno = AUDIT_INTEGRITY_DATA;
@@ -514,7 +503,7 @@ int ima_appraise_measurement(enum ima_hooks func, struct ima_iint_cache *iint,
* of the script interpreter(userspace). Differentiate kernel and
* userspace enforced integrity audit messages.
*/
- if (is_bprm_creds_for_exec(func, file))
+ if (bprm_is_check)
audit_msgno = AUDIT_INTEGRITY_USERSPACE;
/* If reading the xattr failed and there's no modsig, error out. */
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 5770cf691912..955dbaa42f4a 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -236,7 +236,7 @@ static void ima_file_free(struct file *file)
static int process_measurement(struct file *file, const struct cred *cred,
struct lsm_prop *prop, char *buf, loff_t size,
int mask, enum ima_hooks func,
- enum kernel_read_file_id read_id)
+ enum kernel_read_file_id read_id, bool bprm_is_check)
{
struct inode *real_inode, *inode = file_inode(file);
struct ima_iint_cache *iint = NULL;
@@ -426,7 +426,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
inode_lock(inode);
rc = ima_appraise_measurement(func, iint, file,
pathname, xattr_value,
- xattr_len, modsig);
+ xattr_len, modsig, bprm_is_check);
inode_unlock(inode);
}
if (!rc)
@@ -493,14 +493,14 @@ static int ima_file_mmap(struct file *file, unsigned long reqprot,
if (reqprot & PROT_EXEC) {
ret = process_measurement(file, current_cred(), &prop, NULL,
- 0, MAY_EXEC, MMAP_CHECK_REQPROT, 0);
+ 0, MAY_EXEC, MMAP_CHECK_REQPROT, 0, false);
if (ret)
return ret;
}
if (prot & PROT_EXEC)
return process_measurement(file, current_cred(), &prop, NULL,
- 0, MAY_EXEC, MMAP_CHECK, 0);
+ 0, MAY_EXEC, MMAP_CHECK, 0, false);
return 0;
}
@@ -584,7 +584,7 @@ static int ima_bprm_check(struct linux_binprm *bprm)
security_current_getlsmprop_subj(&prop);
return process_measurement(bprm->file, current_cred(),
- &prop, NULL, 0, MAY_EXEC, BPRM_CHECK, 0);
+ &prop, NULL, 0, MAY_EXEC, BPRM_CHECK, 0, bprm->is_check);
}
/**
@@ -614,7 +614,7 @@ static int ima_creds_check(struct linux_binprm *bprm, const struct file *file)
security_current_getlsmprop_subj(&prop);
return process_measurement((struct file *)file, bprm->cred, &prop, NULL,
- 0, MAY_EXEC, CREDS_CHECK, 0);
+ 0, MAY_EXEC, CREDS_CHECK, 0, bprm->is_check);
}
/**
@@ -662,7 +662,7 @@ static int ima_file_check(struct file *file, int mask)
security_current_getlsmprop_subj(&prop);
return process_measurement(file, current_cred(), &prop, NULL, 0,
mask & (MAY_READ | MAY_WRITE | MAY_EXEC |
- MAY_APPEND), FILE_CHECK, 0);
+ MAY_APPEND), FILE_CHECK, 0, false);
}
static int __ima_inode_hash(struct inode *inode, struct file *file, char *buf,
@@ -881,7 +881,7 @@ static int ima_read_file(struct file *file, enum kernel_read_file_id read_id,
func = read_idmap[read_id] ?: FILE_CHECK;
security_current_getlsmprop_subj(&prop);
return process_measurement(file, current_cred(), &prop, NULL, 0,
- MAY_READ, func, 0);
+ MAY_READ, func, 0, false);
}
const int read_idmap[READING_MAX_ID] = {
@@ -925,7 +925,7 @@ static int ima_post_read_file(struct file *file, char *buf, loff_t size,
func = read_idmap[read_id] ?: FILE_CHECK;
security_current_getlsmprop_subj(&prop);
return process_measurement(file, current_cred(), &prop, buf, size,
- MAY_READ, func, read_id);
+ MAY_READ, func, read_id, false);
}
/**
--
2.43.0
^ permalink raw reply related
* [PATCH v1 5/5] landlock: Fix formatting
From: Mickaël Salaün @ 2025-12-19 19:38 UTC (permalink / raw)
Cc: Mickaël Salaün, linux-security-module,
Christian Brauner, Günther Noack, Mateusz Guzik
In-Reply-To: <20251219193855.825889-1-mic@digikod.net>
Format with clang-format -i security/landlock/*.[ch]
Cc: Christian Brauner <brauner@kernel.org>
Cc: Günther Noack <gnoack3000@gmail.com>
Cc: Mateusz Guzik <mjguzik@gmail.com>
Fixes: b4dbfd8653b3 ("Coccinelle-based conversion to use ->i_state accessors")
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
security/landlock/fs.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index fe794875ad46..e3c3a8a9ac27 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -1314,7 +1314,8 @@ static void hook_sb_delete(struct super_block *const sb)
* second call to iput() for the same Landlock object. Also
* checks I_NEW because such inode cannot be tied to an object.
*/
- if (inode_state_read(inode) & (I_FREEING | I_WILL_FREE | I_NEW)) {
+ if (inode_state_read(inode) &
+ (I_FREEING | I_WILL_FREE | I_NEW)) {
spin_unlock(&inode->i_lock);
continue;
}
--
2.52.0
^ permalink raw reply related
* [PATCH v1 4/5] landlock: Fix spelling
From: Mickaël Salaün @ 2025-12-19 19:38 UTC (permalink / raw)
Cc: Mickaël Salaün, linux-security-module,
Günther Noack
In-Reply-To: <20251219193855.825889-1-mic@digikod.net>
Cc: Günther Noack <gnoack3000@gmail.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
security/landlock/domain.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/security/landlock/domain.h b/security/landlock/domain.h
index 7fb70b25f85a..621f054c9a2b 100644
--- a/security/landlock/domain.h
+++ b/security/landlock/domain.h
@@ -97,7 +97,7 @@ struct landlock_hierarchy {
*/
atomic64_t num_denials;
/**
- * @id: Landlock domain ID, sets once at domain creation time.
+ * @id: Landlock domain ID, set once at domain creation time.
*/
u64 id;
/**
--
2.52.0
^ permalink raw reply related
* [PATCH v1 3/5] landlock: Clean up hook_ptrace_access_check()
From: Mickaël Salaün @ 2025-12-19 19:38 UTC (permalink / raw)
Cc: Mickaël Salaün, linux-security-module,
Günther Noack
In-Reply-To: <20251219193855.825889-1-mic@digikod.net>
Make variable's scope minimal in hook_ptrace_access_check().
Cc: Günther Noack <gnoack3000@gmail.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
security/landlock/task.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/security/landlock/task.c b/security/landlock/task.c
index 2385017418ca..bf4ed15a7f01 100644
--- a/security/landlock/task.c
+++ b/security/landlock/task.c
@@ -86,7 +86,6 @@ static int hook_ptrace_access_check(struct task_struct *const child,
const unsigned int mode)
{
const struct landlock_cred_security *parent_subject;
- const struct landlock_ruleset *child_dom;
int err;
/* Quick return for non-landlocked tasks. */
@@ -96,7 +95,8 @@ static int hook_ptrace_access_check(struct task_struct *const child,
scoped_guard(rcu)
{
- child_dom = landlock_get_task_domain(child);
+ const struct landlock_ruleset *const child_dom =
+ landlock_get_task_domain(child);
err = domain_ptrace(parent_subject->domain, child_dom);
}
--
2.52.0
^ permalink raw reply related
* [PATCH v1 2/5] landlock: Improve erratum documentation
From: Mickaël Salaün @ 2025-12-19 19:38 UTC (permalink / raw)
Cc: Mickaël Salaün, linux-security-module,
Günther Noack
In-Reply-To: <20251219193855.825889-1-mic@digikod.net>
Improve description about scoped signal handling.
Reported-by: Günther Noack <gnoack3000@gmail.com>
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
security/landlock/errata/abi-6.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/security/landlock/errata/abi-6.h b/security/landlock/errata/abi-6.h
index df7bc0e1fdf4..5113a829f87e 100644
--- a/security/landlock/errata/abi-6.h
+++ b/security/landlock/errata/abi-6.h
@@ -9,7 +9,7 @@
* This fix addresses an issue where signal scoping was overly restrictive,
* preventing sandboxed threads from signaling other threads within the same
* process if they belonged to different domains. Because threads are not
- * security boundaries, user space might assume that any thread within the same
+ * security boundaries, user space might assume that all threads within the same
* process can send signals between themselves (see :manpage:`nptl(7)` and
* :manpage:`libpsx(3)`). Consistent with :manpage:`ptrace(2)` behavior, direct
* interaction between threads of the same process should always be allowed.
--
2.52.0
^ permalink raw reply related
* [PATCH v1 1/5] landlock: Remove useless include
From: Mickaël Salaün @ 2025-12-19 19:38 UTC (permalink / raw)
Cc: Mickaël Salaün, linux-security-module,
Günther Noack
Remove useless audit.h include.
Cc: Günther Noack <gnoack@google.com>
Fixes: 33e65b0d3add ("landlock: Add AUDIT_LANDLOCK_ACCESS and log ptrace denials")
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
security/landlock/ruleset.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
index dfcdc19ea268..0a5b0c76b3f7 100644
--- a/security/landlock/ruleset.c
+++ b/security/landlock/ruleset.c
@@ -23,7 +23,6 @@
#include <linux/workqueue.h>
#include "access.h"
-#include "audit.h"
#include "domain.h"
#include "limits.h"
#include "object.h"
--
2.52.0
^ permalink raw reply related
* Re: [PATCH v5 06/36] cleanup: Basic compatibility with context analysis
From: Bart Van Assche @ 2025-12-19 19:16 UTC (permalink / raw)
To: Marco Elver, Peter Zijlstra, Boqun Feng, Ingo Molnar, Will Deacon
Cc: David S. Miller, Luc Van Oostenryck, Chris Li, Paul E. McKenney,
Alexander Potapenko, Arnd Bergmann, Christoph Hellwig,
Dmitry Vyukov, Eric Dumazet, Frederic Weisbecker,
Greg Kroah-Hartman, Herbert Xu, Ian Rogers, Jann Horn,
Joel Fernandes, Johannes Berg, Jonathan Corbet, Josh Triplett,
Justin Stitt, Kees Cook, Kentaro Takeda, Lukas Bulwahn,
Mark Rutland, Mathieu Desnoyers, Miguel Ojeda, Nathan Chancellor,
Neeraj Upadhyay, Nick Desaulniers, Steven Rostedt, Tetsuo Handa,
Thomas Gleixner, Thomas Graf, Uladzislau Rezki, Waiman Long,
kasan-dev, linux-crypto, linux-doc, linux-kbuild, linux-kernel,
linux-mm, linux-security-module, linux-sparse, linux-wireless,
llvm, rcu
In-Reply-To: <20251219154418.3592607-7-elver@google.com>
On 12/19/25 7:39 AM, Marco Elver wrote:
> +#define DECLARE_LOCK_GUARD_1_ATTRS(_name, _lock, _unlock) \
> +static inline class_##_name##_t class_##_name##_constructor(lock_##_name##_t *_T) _lock;\
> +static __always_inline void __class_##_name##_cleanup_ctx(class_##_name##_t **_T) \
> + __no_context_analysis _unlock { }
Elsewhere in the cleanup.h header arguments with the names "_lock" and
"_unlock" hold executable code that perform "lock" and "unlock"
operations respectively, e.g. mutex_lock() and mutex_unlock(). The
DECLARE_LOCK_GUARD_1_ATTRS() "_lock" and "_unlock" arguments however are
function annotations. Please prevent confusion and use other names for
the _lock and _unlock arguments, e.g. _acquire_attr and _release_attr.
Thanks,
Bart.
^ permalink raw reply
* Re: [PATCH v5 02/36] compiler-context-analysis: Add infrastructure for Context Analysis with Clang
From: Marco Elver @ 2025-12-19 19:11 UTC (permalink / raw)
To: Bart Van Assche
Cc: Peter Zijlstra, Boqun Feng, Ingo Molnar, Will Deacon,
David S. Miller, Luc Van Oostenryck, Chris Li, Paul E. McKenney,
Alexander Potapenko, Arnd Bergmann, Christoph Hellwig,
Dmitry Vyukov, Eric Dumazet, Frederic Weisbecker,
Greg Kroah-Hartman, Herbert Xu, Ian Rogers, Jann Horn,
Joel Fernandes, Johannes Berg, Jonathan Corbet, Josh Triplett,
Justin Stitt, Kees Cook, Kentaro Takeda, Lukas Bulwahn,
Mark Rutland, Mathieu Desnoyers, Miguel Ojeda, Nathan Chancellor,
Neeraj Upadhyay, Nick Desaulniers, Steven Rostedt, Tetsuo Handa,
Thomas Gleixner, Thomas Graf, Uladzislau Rezki, Waiman Long,
kasan-dev, linux-crypto, linux-doc, linux-kbuild, linux-kernel,
linux-mm, linux-security-module, linux-sparse, linux-wireless,
llvm, rcu
In-Reply-To: <2f0c27eb-eca5-4a7f-8035-71c6b0c84e30@acm.org>
On Fri, Dec 19, 2025 at 11:04AM -0800, 'Bart Van Assche' via kasan-dev wrote:
> On 12/19/25 10:59 AM, Marco Elver wrote:
> > On Fri, 19 Dec 2025 at 19:39, 'Bart Van Assche' via kasan-dev
> > <kasan-dev@googlegroups.com> wrote:
> > > I'm concerned that the context_lock_struct() macro will make code harder
> > > to read. Anyone who encounters the context_lock_struct() macro will have
> > > to look up its definition to learn what it does. I propose to split this
> > > macro into two macros:
> > > * One macro that expands into "__ctx_lock_type(name)".
> > > * A second macro that expands into the rest of the above macro.
> > >
> > > In other words, instead of having to write
> > > context_lock_struct(struct_name, { ... }); developers will have to write
> > >
> > > struct context_lock_type struct_name {
> > > ...;
> > > };
> > > context_struct_helper_functions(struct_name);
> >
> > This doesn't necessarily help with not having to look up its
> > definition to learn what it does.
> >
> > If this is the common pattern, it will blindly be repeated, and this
> > adds 1 more line and makes this a bit more verbose. Maybe the helper
> > functions aren't always needed, but I also think that context lock
> > types should remain relatively few. For all synchronization
> > primitives that were enabled in this series, the helpers are required.
> >
> > The current usage is simply:
> >
> > context_lock_struct(name) {
> > ... struct goes here ...
> > }; // note no awkward ) brace
> >
> > I don't know which way the current kernel style is leaning towards,
> > but if we take <linux/cleanup.h> as an example, a simple programming
> > model / API is actually preferred.
> Many kernel developers are used to look up the definition of a data
> structure either by using ctags, etags or a similar tool or by using
> grep and a pattern like "${struct_name} {\$". Breaking the tools kernel
> developer use today to look up data structure definitions might cause
> considerable frustration and hence shouldn't be done lightly.
Fair point. In fact, it's as simple as e.g. (just tested with mutex) as
this:
diff --git a/include/linux/mutex_types.h b/include/linux/mutex_types.h
index 80975935ec48..63ab9e65bb48 100644
--- a/include/linux/mutex_types.h
+++ b/include/linux/mutex_types.h
@@ -38,7 +38,8 @@
* - detects multi-task circular deadlocks and prints out all affected
* locks and tasks (and only those tasks)
*/
-context_lock_struct(mutex) {
+context_lock_struct(mutex);
+struct mutex {
atomic_long_t owner;
raw_spinlock_t wait_lock;
#ifdef CONFIG_MUTEX_SPIN_ON_OWNER
@@ -59,7 +60,8 @@ context_lock_struct(mutex) {
*/
#include <linux/rtmutex.h>
-context_lock_struct(mutex) {
+context_lock_struct(mutex);
+struct mutex {
struct rt_mutex_base rtmutex;
#ifdef CONFIG_DEBUG_LOCK_ALLOC
struct lockdep_map dep_map;
So the existing macro does support both use-cases as-is. I suppose we
could force the above use pattern.
The reason it works, is because it forward-declares the struct anyway to
define the helper functions.
^ permalink raw reply related
* Re: [PATCH v5 02/36] compiler-context-analysis: Add infrastructure for Context Analysis with Clang
From: Bart Van Assche @ 2025-12-19 19:04 UTC (permalink / raw)
To: Marco Elver
Cc: Peter Zijlstra, Boqun Feng, Ingo Molnar, Will Deacon,
David S. Miller, Luc Van Oostenryck, Chris Li, Paul E. McKenney,
Alexander Potapenko, Arnd Bergmann, Christoph Hellwig,
Dmitry Vyukov, Eric Dumazet, Frederic Weisbecker,
Greg Kroah-Hartman, Herbert Xu, Ian Rogers, Jann Horn,
Joel Fernandes, Johannes Berg, Jonathan Corbet, Josh Triplett,
Justin Stitt, Kees Cook, Kentaro Takeda, Lukas Bulwahn,
Mark Rutland, Mathieu Desnoyers, Miguel Ojeda, Nathan Chancellor,
Neeraj Upadhyay, Nick Desaulniers, Steven Rostedt, Tetsuo Handa,
Thomas Gleixner, Thomas Graf, Uladzislau Rezki, Waiman Long,
kasan-dev, linux-crypto, linux-doc, linux-kbuild, linux-kernel,
linux-mm, linux-security-module, linux-sparse, linux-wireless,
llvm, rcu
In-Reply-To: <CANpmjNM=4baTiSWGOiSWLfQV2YqMt6qkdV__uj+QtD4zAY8Weg@mail.gmail.com>
On 12/19/25 10:59 AM, Marco Elver wrote:
> On Fri, 19 Dec 2025 at 19:39, 'Bart Van Assche' via kasan-dev
> <kasan-dev@googlegroups.com> wrote:
>> I'm concerned that the context_lock_struct() macro will make code harder
>> to read. Anyone who encounters the context_lock_struct() macro will have
>> to look up its definition to learn what it does. I propose to split this
>> macro into two macros:
>> * One macro that expands into "__ctx_lock_type(name)".
>> * A second macro that expands into the rest of the above macro.
>>
>> In other words, instead of having to write
>> context_lock_struct(struct_name, { ... }); developers will have to write
>>
>> struct context_lock_type struct_name {
>> ...;
>> };
>> context_struct_helper_functions(struct_name);
>
> This doesn't necessarily help with not having to look up its
> definition to learn what it does.
>
> If this is the common pattern, it will blindly be repeated, and this
> adds 1 more line and makes this a bit more verbose. Maybe the helper
> functions aren't always needed, but I also think that context lock
> types should remain relatively few. For all synchronization
> primitives that were enabled in this series, the helpers are required.
>
> The current usage is simply:
>
> context_lock_struct(name) {
> ... struct goes here ...
> }; // note no awkward ) brace
>
> I don't know which way the current kernel style is leaning towards,
> but if we take <linux/cleanup.h> as an example, a simple programming
> model / API is actually preferred.
Many kernel developers are used to look up the definition of a data
structure either by using ctags, etags or a similar tool or by using
grep and a pattern like "${struct_name} {\$". Breaking the tools kernel
developer use today to look up data structure definitions might cause
considerable frustration and hence shouldn't be done lightly.
Thanks,
Bart.
^ permalink raw reply
* Re: [PATCH v4 06/35] cleanup: Basic compatibility with context analysis
From: Bart Van Assche @ 2025-12-19 18:59 UTC (permalink / raw)
To: Marco Elver, Peter Zijlstra
Cc: Boqun Feng, Ingo Molnar, Will Deacon, David S. Miller,
Luc Van Oostenryck, Chris Li, Paul E. McKenney,
Alexander Potapenko, Arnd Bergmann, Christoph Hellwig,
Dmitry Vyukov, Eric Dumazet, Frederic Weisbecker,
Greg Kroah-Hartman, Herbert Xu, Ian Rogers, Jann Horn,
Joel Fernandes, Johannes Berg, Jonathan Corbet, Josh Triplett,
Justin Stitt, Kees Cook, Kentaro Takeda, Lukas Bulwahn,
Mark Rutland, Mathieu Desnoyers, Miguel Ojeda, Nathan Chancellor,
Neeraj Upadhyay, Nick Desaulniers, Steven Rostedt, Tetsuo Handa,
Thomas Gleixner, Thomas Graf, Uladzislau Rezki, Waiman Long,
kasan-dev, linux-crypto, linux-doc, linux-kbuild, linux-kernel,
linux-mm, linux-security-module, linux-sparse, linux-wireless,
llvm, rcu
In-Reply-To: <CANpmjNNm-kbTw46Wh1BJudynHOeLn-Oxew8VuAnCppvV_WtyBw@mail.gmail.com>
On 12/15/25 7:53 AM, Marco Elver wrote:
> For cleanup.h, the problem is that to instantiate we use
> "guard(class)(args..)". If it had been designed as "guard(class,
> args...)", i.e. just use __VA_ARGS__ explicitly instead of the
> implicit 'args...', it might have been possible to add a second
> cleanup variable to do the same (with some additional magic to extract
> the first arg if one exists). Unfortunately, the use of the current
> guard()() idiom has become so pervasive that this is a bigger
> refactor. I'm going to leave cleanup.h as-is for now, if we think we
> want to give this a go in the current state.
Peter, has it already been considered to make the guard() and
scoped_guard() macros more consistent? If there would be agreement that
guard(class)(args..) should be changed into guard(class, args..), I can
help with realizing this conversion.
Thanks,
Bart.
^ permalink raw reply
* Re: [PATCH v5 02/36] compiler-context-analysis: Add infrastructure for Context Analysis with Clang
From: Marco Elver @ 2025-12-19 18:59 UTC (permalink / raw)
To: Bart Van Assche
Cc: Peter Zijlstra, Boqun Feng, Ingo Molnar, Will Deacon,
David S. Miller, Luc Van Oostenryck, Chris Li, Paul E. McKenney,
Alexander Potapenko, Arnd Bergmann, Christoph Hellwig,
Dmitry Vyukov, Eric Dumazet, Frederic Weisbecker,
Greg Kroah-Hartman, Herbert Xu, Ian Rogers, Jann Horn,
Joel Fernandes, Johannes Berg, Jonathan Corbet, Josh Triplett,
Justin Stitt, Kees Cook, Kentaro Takeda, Lukas Bulwahn,
Mark Rutland, Mathieu Desnoyers, Miguel Ojeda, Nathan Chancellor,
Neeraj Upadhyay, Nick Desaulniers, Steven Rostedt, Tetsuo Handa,
Thomas Gleixner, Thomas Graf, Uladzislau Rezki, Waiman Long,
kasan-dev, linux-crypto, linux-doc, linux-kbuild, linux-kernel,
linux-mm, linux-security-module, linux-sparse, linux-wireless,
llvm, rcu
In-Reply-To: <97e832b7-04a9-49cb-973a-bf9870c21c2f@acm.org>
On Fri, 19 Dec 2025 at 19:39, 'Bart Van Assche' via kasan-dev
<kasan-dev@googlegroups.com> wrote:
> On 12/19/25 7:39 AM, Marco Elver wrote:
> > +#if defined(WARN_CONTEXT_ANALYSIS)
> > +
> > +/*
> > + * These attributes define new context lock (Clang: capability) types.
> > + * Internal only.
> > + */
>
> How can macros be "internal only" that are defined in a header file that
> will be included by almost all kernel code? Please consider changing
> "internal only" into something that is more clear, e.g. "should only be
> used in the macro definitions in this header file".
Sure, comment could be improved.
Let's say they aren't for general use by normal code that just enables
the analysis for checking; for that we define the shorter (retaining
previous names already in use) ones below. But some of these
attributes can and are used by implementing support for some of the
synchronization primitives.
> > +/*
> > + * The below are used to annotate code being checked. Internal only.
> > + */
>
> Same comment here about "internal only".
Sure, can be clarified.
> > +/**
> > + * context_lock_struct() - declare or define a context lock struct
> > + * @name: struct name
> > + *
> > + * Helper to declare or define a struct type that is also a context lock.
> > + *
> > + * .. code-block:: c
> > + *
> > + * context_lock_struct(my_handle) {
> > + * int foo;
> > + * long bar;
> > + * };
> > + *
> > + * struct some_state {
> > + * ...
> > + * };
> > + * // ... declared elsewhere ...
> > + * context_lock_struct(some_state);
> > + *
> > + * Note: The implementation defines several helper functions that can acquire
> > + * and release the context lock.
> > + */
> > +# define context_lock_struct(name, ...) \
> > + struct __ctx_lock_type(name) __VA_ARGS__ name; \
> > + static __always_inline void __acquire_ctx_lock(const struct name *var) \
> > + __attribute__((overloadable)) __no_context_analysis __acquires_ctx_lock(var) { } \
> > + static __always_inline void __acquire_shared_ctx_lock(const struct name *var) \
> > + __attribute__((overloadable)) __no_context_analysis __acquires_shared_ctx_lock(var) { } \
> > + static __always_inline bool __try_acquire_ctx_lock(const struct name *var, bool ret) \
> > + __attribute__((overloadable)) __no_context_analysis __try_acquires_ctx_lock(1, var) \
> > + { return ret; } \
> > + static __always_inline bool __try_acquire_shared_ctx_lock(const struct name *var, bool ret) \
> > + __attribute__((overloadable)) __no_context_analysis __try_acquires_shared_ctx_lock(1, var) \
> > + { return ret; } \
> > + static __always_inline void __release_ctx_lock(const struct name *var) \
> > + __attribute__((overloadable)) __no_context_analysis __releases_ctx_lock(var) { } \
> > + static __always_inline void __release_shared_ctx_lock(const struct name *var) \
> > + __attribute__((overloadable)) __no_context_analysis __releases_shared_ctx_lock(var) { } \
> > + static __always_inline void __assume_ctx_lock(const struct name *var) \
> > + __attribute__((overloadable)) __assumes_ctx_lock(var) { } \
> > + static __always_inline void __assume_shared_ctx_lock(const struct name *var) \
> > + __attribute__((overloadable)) __assumes_shared_ctx_lock(var) { } \
> > + struct name
>
> I'm concerned that the context_lock_struct() macro will make code harder
> to read. Anyone who encounters the context_lock_struct() macro will have
> to look up its definition to learn what it does. I propose to split this
> macro into two macros:
> * One macro that expands into "__ctx_lock_type(name)".
> * A second macro that expands into the rest of the above macro.
>
> In other words, instead of having to write
> context_lock_struct(struct_name, { ... }); developers will have to write
>
> struct context_lock_type struct_name {
> ...;
> };
> context_struct_helper_functions(struct_name);
This doesn't necessarily help with not having to look up its
definition to learn what it does.
If this is the common pattern, it will blindly be repeated, and this
adds 1 more line and makes this a bit more verbose. Maybe the helper
functions aren't always needed, but I also think that context lock
types should remain relatively few. For all synchronization
primitives that were enabled in this series, the helpers are required.
The current usage is simply:
context_lock_struct(name) {
... struct goes here ...
}; // note no awkward ) brace
I don't know which way the current kernel style is leaning towards,
but if we take <linux/cleanup.h> as an example, a simple programming
model / API is actually preferred.
> My opinion is that the alternative that I'm proposing is easier to read.
> Additionally, it doesn't break existing tools that support jumping from
> the name of a struct to its definition, e.g. ctags and etags.
>
> > +config WARN_CONTEXT_ANALYSIS_ALL
> > + bool "Enable context analysis for all source files"
> > + depends on WARN_CONTEXT_ANALYSIS
> > + depends on EXPERT && !COMPILE_TEST
> > + help
> > + Enable tree-wide context analysis. This is likely to produce a
> > + large number of false positives - enable at your own risk.
> > +
> > + If unsure, say N.
>
> Why !COMPILE_TEST?
That's the idiomatic way to prevent this being enabled in allyesconfig
builds, and other compile-only random configs enabling this and then
stumbling over 1000s of warnings.
Thanks,
-- Marco
^ permalink raw reply
* Re: [PATCH v5 04/36] Documentation: Add documentation for Compiler-Based Context Analysis
From: Bart Van Assche @ 2025-12-19 18:51 UTC (permalink / raw)
To: Marco Elver, Peter Zijlstra, Boqun Feng, Ingo Molnar, Will Deacon
Cc: David S. Miller, Luc Van Oostenryck, Chris Li, Paul E. McKenney,
Alexander Potapenko, Arnd Bergmann, Christoph Hellwig,
Dmitry Vyukov, Eric Dumazet, Frederic Weisbecker,
Greg Kroah-Hartman, Herbert Xu, Ian Rogers, Jann Horn,
Joel Fernandes, Johannes Berg, Jonathan Corbet, Josh Triplett,
Justin Stitt, Kees Cook, Kentaro Takeda, Lukas Bulwahn,
Mark Rutland, Mathieu Desnoyers, Miguel Ojeda, Nathan Chancellor,
Neeraj Upadhyay, Nick Desaulniers, Steven Rostedt, Tetsuo Handa,
Thomas Gleixner, Thomas Graf, Uladzislau Rezki, Waiman Long,
kasan-dev, linux-crypto, linux-doc, linux-kbuild, linux-kernel,
linux-mm, linux-security-module, linux-sparse, linux-wireless,
llvm, rcu
In-Reply-To: <20251219154418.3592607-5-elver@google.com>
On 12/19/25 7:39 AM, Marco Elver wrote:
> +Context Analysis is a language extension, which enables statically checking
^
My grammar checker tells me that there shouldn't be a comma here.
However, I'm not sure whether the software that I'm using got this
right.
> +that required contexts are active (or inactive) by acquiring and releasing
> +user-definable "context locks". An obvious application is lock-safety checking
Please improve clarity of this text by adding a definition for "context
lock", e.g. the following: "Each context lock has a name. A state is
associated with each context lock. Supported states are locked,
unlocked, shared locked and exclusive locked. Functions can be annotated
to declare what lock state is expected upon entry and what the lock
state will be upon return. Members of data structures can be annotated
to indicate what context locks should be held upon access."
Thanks,
Bart.
^ 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