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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] s390/maccess: RMW race in __s390_kernel_write() via apply_relocate_add()
--

commit d0c62af80b12a6f9e8f6c9ffe62431d5b78a99bf
Author: Heiko Carstens <hca@linux.ibm.com>

s390/maccess: Remove spinlock from __s390_kernel_write()

This commit removes s390_kernel_write_lock from __s390_kernel_write() under
the assumption that all runtime callers hold text_mutex, and replaces the
spinlock with preempt_disable(), sync_core(), and preempt_enable().

> diff --git a/arch/s390/mm/maccess.c b/arch/s390/mm/maccess.c
> index f39968dd80638..cda3f81dfbc2f 100644
> --- a/arch/s390/mm/maccess.c
> +++ b/arch/s390/mm/maccess.c
[ ... ]
> @@ -60,22 +61,20 @@ static notrace long s390_kernel_write_odd(void *dst, const void *src, size_t siz
>   * bytes from destination at an eight byte boundary, modifies the bytes
>   * requested and writes the result back in a loop.
>   */
> -static DEFINE_SPINLOCK(s390_kernel_write_lock);
> -
>  notrace void *__s390_kernel_write(void *dst, const void *src, size_t size)
>  {
>  	void *tmp = dst;
> -	unsigned long flags;
>  	long copied;
>  
> -	spin_lock_irqsave(&s390_kernel_write_lock, flags);
> +	preempt_disable();
>  	while (size) {
>  		copied = s390_kernel_write_odd(tmp, src, size);
>  		tmp += copied;
>  		src += copied;
>  		size -= copied;
>  	}
> -	spin_unlock_irqrestore(&s390_kernel_write_lock, flags);
> +	sync_core();
> +	preempt_enable();

[Severity: High]
Can removing s390_kernel_write_lock cause a read-modify-write race in
s390_kernel_write_odd() when apply_relocate_add() is called by livepatching?

In arch/s390/kernel/module.c:apply_relocate_add(), s390_kernel_write() is
used whenever the module is already formed, without taking text_mutex:

    int apply_relocate_add(Elf_Shdr *sechdrs, const char *strtab,
                           unsigned int symindex, unsigned int relsec,
                           struct module *me)
    {
        bool early = me->state == MODULE_STATE_UNFORMED;
        void *(*write)(void *, const void *, size_t) = memcpy;

        if (!early)
            write = s390_kernel_write;

        return __apply_relocate_add(sechdrs, strtab, symindex, relsec, me,
                                    write);
    }

At runtime, livepatching reaches this path via:

    klp_enable_patch() / klp_module_coming()
      -> klp_init_object_loaded()
        -> klp_apply_object_relocs()
          -> klp_write_object_relocs()
            -> klp_write_section_relocs()
              -> apply_relocate_add()

Commit 5b384f933590 ("x86/module: Use text_mutex in apply_relocate_add()")
removed mutex_lock(&text_mutex) from klp_init_object_loaded() in
kernel/livepatch/core.c and added text_mutex only to x86, relying on
s390_kernel_write_lock to serialize s390's apply_relocate_add().

Because s390_kernel_write_odd() performs a non-atomic 8-byte
read-modify-write cycle on (dst & ~7UL) using mvc, ex, and sturg, could a
concurrent caller holding text_mutex (such as ftrace, jump_label, kprobes, or
bpf_arch_text_poke) modifying an instruction in the same 8-byte doubleword of
the livepatch module have its write clobbered?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026120329.1792436-1-hca@linux.ibm.com?part=4