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

[Heiko Carstens <hca@linux.ibm.com>]

On Mon, Oct 26, 2026 at 02:04:02PM +0100, Ilya Leoshkevich wrote:
> 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?

Initially I thought that wouldn't be necessary, since "I know what I'm doing"
:) But Sashiko already complained that this might not be safe. So I'm going to
had the mutex to all callers of s390_kernel_write(), and add __must_hold too,
like you propose.

> > -	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?

Maybe I don't understand your point, but I added the preempt_disable() /
preempt_enable() pair exactly for this reason: make sure that the writer syncs
first - only calling text_poke_sync() afterward does not guarantee anything
due to preemption (well, that's not true since there are many implicit
synchronizations before the next CPU runs, but the point of this was to make
everything explicit, and not "hope" that things work).

> 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?

Initial alternatives patching might indeed patch multiple instructions in a
row, and that's a valid use case, since the code is guaranteed to be not
executed anywhere...
So a WARN_ONCE() would at least find one false positive :)

I don't think we should go this route (when is it valid to patch more than one
instruction, when not). As of now no code is doing that.