Re: [PATCH v2 10/10] s390/maccess: Remove spinlock from __s390_kernel_write()

[Ilya Leoshkevich]

On 10/27/26 15:30, Heiko Carstens wrote:
> The spinlock in __s390_kernel_write() was introduced to serialize the
> read-modify-write sequence against concurrent callers. Now that all
> runtime callers of s390_kernel_write() hold the text_mutex, the
> spinlock is not required anymore.
>
> Replace it with preempt_disable() / preempt_enable() and sync_core().
> Previously, CPU serialization was implicitly done by other
> instructions. Make this explicit with sync_core() instead of
> relying on that.
>
> Preemption is disabled so that sync_core() issues the CPU serialization
> instruction on the same CPU that executed the stores. This completes all
> previous stores and makes them visible to other CPUs.

Doesn't preemption happen only via interrupts, which always serialize?
> Other CPUs need to perform CPU serialization, e.g. with text_poke_sync(),
> to discard already prefetched instructions.
>
> To verify that text_mutex is held instead of the spinlock, add a
> lockdep_assert_held() call to s390_kernel_write(). To make that work
> a change to the kprobes code is required:
>
> swap_instruction(), which calls s390_kernel_write(), may be called in
> stop_machine() context.
> For that case the task which calls s390_kernel_write() is not the task
> which called stop_machine() - lockdep_assert_held() will report that
> text_mutex is not held by the current task, even though text_mutex is
> held by the stop_machine() caller. Exclusive text write access is
> guaranteed.
> Work around that by calling the non-checking __s390_kernel_write(),
> and add lockdep_assert_held() checks to arch_arm_kprobe() and
> arch_disarm_kprobe().
>
> Signed-off-by: Heiko Carstens <hca@xxxxxxxxxxxxx>
> ---
>   arch/s390/include/asm/text-patching.h | 7 +++++--
>   arch/s390/kernel/kprobes.c            | 4 +++-
>   arch/s390/mm/maccess.c                | 8 +++-----
>   3 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/arch/s390/include/asm/text-patching.h b/arch/s390/include/asm/text-patching.h
> index c2f87f20ff97..a382a3e7c2f8 100644
> --- a/arch/s390/include/asm/text-patching.h
> +++ b/arch/s390/include/asm/text-patching.h
> @@ -19,9 +19,12 @@ void *__s390_kernel_write(void *dst, const void *src, size_t size);
>   
>   static inline void *s390_kernel_write(void *dst, const void *src, size_t size)
>   {
> -	if (__is_defined(__DECOMPRESSOR))
> -		return memcpy(dst, src, size);
> +#ifdef __DECOMPRESSOR
> +	return memcpy(dst, src, size);
> +#else
> +	lockdep_assert_held(&text_mutex);
>   	return __s390_kernel_write(dst, src, size);
> +#endif
>   }

I assume #ifdef is required because lockdep_assert_held() is not
available in decompressor and there are no stubs?


Anyhow:

Reviewed-by: Ilya Leoshkevich <iii@xxxxxxxxxxxxx>

[...]