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

[Ilya Leoshkevich <iii@linux.ibm.com>]

On 10/26/26 13:03, Heiko Carstens wrote:
> The spinlock in __s390_kernel_write() was introduced to serialize the
> read-modify-write sequence against concurrent callers. Now that all
> relevant runtime callers of s390_kernel_write() hold the text_mutex, the
> spinlock is not required anymore.

Do we want to enforce this with __must_hold?

> 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.
> 
> Other CPUs need to perform CPU serialization, e.g. with text_poke_sync(),
> to discard already prefetched instructions.
> 
> Signed-off-by: Heiko Carstens <hca@linux.ibm.com>
> ---
>   arch/s390/mm/maccess.c | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/s390/mm/maccess.c b/arch/s390/mm/maccess.c
> index f39968dd8063..cda3f81dfbc2 100644
> --- a/arch/s390/mm/maccess.c
> +++ b/arch/s390/mm/maccess.c
> @@ -6,6 +6,7 @@
>    *
>    */
>   
> +#include <linux/text-patching.h>
>   #include <linux/uaccess.h>
>   #include <linux/kernel.h>
>   #include <linux/types.h>
> @@ -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();
>   
>   	return dst;
>   }

I've been looking at sync_core() and started thinking about the safety
of this interface w.r.t. CPUs concurrently executing the code being
patched, since locking protects only against concurrent readers and
writers, but not executors.

If there is only one active CPU, for example, during ftrace
initialization, then everything is fine.

If there are other active CPUs and we make modifications within a single
aligned u64, then sync_core() is required, because even though
text_poke_sync() nominally takes care of this, it may sync our core
last. However, I wonder, what would it take to make it always sync the
current core first?

Finally, and most importantly, if there are other active CPUs and we
patch multiple u64s, then we are broken, since the other CPUs may see
partial instructions. I'm not sure if it's worth bothering with, but
maybe consider adding a WARN_ONCE() to guard against this?