linux-kernel - Re: [PATCH v2] rust: Fix build error

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <Zyv6unk_tRyv_v7m@boqun-archlinux>
Date: Wed, 6 Nov 2024 15:24:42 -0800
From: Boqun Feng <boqun.feng@...il.com>
To: Eder Zulian <ezulian@...hat.com>
Cc: rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
	miguel.ojeda.sandonis@...il.com, tglx@...utronix.de,
	williams@...hat.com, ojeda@...nel.org, alex.gaynor@...il.com,
	gary@...yguo.net, bjorn3_gh@...tonmail.com, benno.lossin@...ton.me,
	a.hindborg@...nel.org, aliceryhl@...gle.com, tmgross@...ch.edu,
	jlelli@...hat.com
Subject: Re: [PATCH v2] rust: Fix build error

Hi Eder,

Seems I forgot to reply you on your reply to v1, sorry about that.

For the commit title, I think it better be:

	rust: helpers: Avoid raw_spin_lock initialization for PREEMPT_RT

, because in general, title of the commit should be as specific as
possible (otherwise, half year later there could be 10 commits titled
"rust: Fix build error").

On Wed, Nov 06, 2024 at 10:12:15PM +0100, Eder Zulian wrote:
> On a PREEMPT_RT build, spin locks have been mapped to rt_mutex types, so
> avoid the raw_spinlock_init call for RT.
> 
> When CONFIG_DEBUG_SPINLOCK=y and CONFIG_PREEMPT_RT=y the following build
> error occurs:
> 
> https://lore.kernel.org/oe-kbuild-all/202409251238.vetlgXE9-lkp@intel.com/
> 

Since you already use the "Closes" tag to refer the bug report, let's
avoid links showing twice, how rephrase the above three paragraphs as:

"""
When PREEMPT_RT=y, spin locks are mapped to rt_mutex types, so using
spinlock_check() + __raw_spin_lock_init() to initialize spin locks is
incorrect, and would cause build errors.

Introduce __spin_lock_init() to initialize a spin lock with lockdep
rquired information for PREEMPT_RT builds, and use it in the Rust
helper.
"""

Thoughts?

> Fixes: 876346536c1b ("rust: kbuild: split up helpers.c")

I'm not sure this is the correct "Fixes" tag, that commit is a code
move, so it's unlikely introducing issue itself. Moreover, we really
need PREEMPT_RT being able to trigger the issue, so I think the correct
"Fixes" tag is:

Fixes: d2d6422f8bd1 ("x86: Allow to enable PREEMPT_RT.")

(yes, I know PREEMPT_RT is a long existing project, but it was until
that commit, you can build a kernel with PREEMPT_RT=y IIUC)

This will help stable maintainers for backport decisions.


The rest of patch looks good to me (we could maybe provide a
__spin_lock_init() for !RT build as well, but that's more of a
cleanup)

Regards,
Boqun

> Reported-by: kernel test robot <lkp@...el.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202409251238.vetlgXE9-lkp@intel.com/
> Signed-off-by: Eder Zulian <ezulian@...hat.com>
> ---
> V1 -> V2: Cleaned up style and addressed review comments
>  include/linux/spinlock_rt.h | 15 +++++++--------
>  rust/helpers/spinlock.c     |  8 ++++++--
>  2 files changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/spinlock_rt.h b/include/linux/spinlock_rt.h
> index f9f14e135be7..f6499c37157d 100644
> --- a/include/linux/spinlock_rt.h
> +++ b/include/linux/spinlock_rt.h
> @@ -16,22 +16,21 @@ static inline void __rt_spin_lock_init(spinlock_t *lock, const char *name,
>  }
>  #endif
>  
> -#define spin_lock_init(slock)					\
> +#define __spin_lock_init(slock, name, key, percpu)		\
>  do {								\
> -	static struct lock_class_key __key;			\
> -								\
>  	rt_mutex_base_init(&(slock)->lock);			\
> -	__rt_spin_lock_init(slock, #slock, &__key, false);	\
> +	__rt_spin_lock_init(slock, name, key, percpu);		\
>  } while (0)
>  
> -#define local_spin_lock_init(slock)				\
> +#define _spin_lock_init(slock, percpu)				\
>  do {								\
>  	static struct lock_class_key __key;			\
> -								\
> -	rt_mutex_base_init(&(slock)->lock);			\
> -	__rt_spin_lock_init(slock, #slock, &__key, true);	\
> +	__spin_lock_init(slock, #slock, &__key, percpu);	\
>  } while (0)
>  
> +#define spin_lock_init(slock)		_spin_lock_init(slock, false)
> +#define local_spin_lock_init(slock)	_spin_lock_init(slock, true)
> +
>  extern void rt_spin_lock(spinlock_t *lock) __acquires(lock);
>  extern void rt_spin_lock_nested(spinlock_t *lock, int subclass)	__acquires(lock);
>  extern void rt_spin_lock_nest_lock(spinlock_t *lock, struct lockdep_map *nest_lock) __acquires(lock);
> diff --git a/rust/helpers/spinlock.c b/rust/helpers/spinlock.c
> index b7b0945e8b3c..5971fdf6f755 100644
> --- a/rust/helpers/spinlock.c
> +++ b/rust/helpers/spinlock.c
> @@ -6,10 +6,14 @@ void rust_helper___spin_lock_init(spinlock_t *lock, const char *name,
>  				  struct lock_class_key *key)
>  {
>  #ifdef CONFIG_DEBUG_SPINLOCK
> +# if defined(CONFIG_PREEMPT_RT)
> +	__spin_lock_init(lock, name, key, false);
> +# else /*!CONFIG_PREEMPT_RT */
>  	__raw_spin_lock_init(spinlock_check(lock), name, key, LD_WAIT_CONFIG);
> -#else
> +# endif /* CONFIG_PREEMPT_RT */
> +#else /* !CONFIG_DEBUG_SPINLOCK */
>  	spin_lock_init(lock);
> -#endif
> +#endif /* CONFIG_DEBUG_SPINLOCK */
>  }
>  
>  void rust_helper_spin_lock(spinlock_t *lock)
> -- 
> 2.47.0
> 
>