Re: [PATCH 09/23] membarrier: Fix incorrect barrier positions during exec and kthread_use_mm()

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

----- Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> ----- On Jan 8, 2022, at 11:43 AM, Andy Lutomirski luto@kernel.org wrote:
> 
> > membarrier() requires a barrier before changes to rq->curr->mm, not just
> > before writes to rq->membarrier_state.  Move the barrier in exec_mmap() to
> > the right place.
> 
> I don't see anything that was technically wrong with membarrier_exec_mmap
> before this patchset. membarrier_exec-mmap issued a smp_mb just after
> the task_lock(), and proceeded to clear the mm->membarrier_state and
> runqueue membarrier state. And then the tsk->mm is set *after* the smp_mb().
> 
> So from this commit message we could be led to think there was something
> wrong before, but I do not think it's true. This first part of the proposed
> change is merely a performance optimization that removes a useless memory
> barrier on architectures where smp_mb__after_spinlock() is a no-op, and
> removes a useless store to mm->membarrier_state because it is already
> zero-initialized. This is all very nice, but does not belong in a "Fix" patch.
> 
> > Add the barrier in kthread_use_mm() -- it was entirely
> > missing before.
> 
> This is correct. This second part of the patch is indeed a relevant fix.

However this adds a useless barrier for CONFIG_MEMBARRIER=n.

Thanks,

Mathieu


> 
> Thanks,
> 
> Mathieu
> 
> > 
> > This patch makes exec_mmap() and kthread_use_mm() use the same membarrier
> > hooks, which results in some code deletion.
> > 
> > As an added bonus, this will eliminate a redundant barrier in execve() on
> > arches for which spinlock acquisition is a barrier.
> > 
> > Signed-off-by: Andy Lutomirski <luto@kernel.org>
> > ---
> > fs/exec.c                 |  6 +++++-
> > include/linux/sched/mm.h  |  2 --
> > kernel/kthread.c          |  5 +++++
> > kernel/sched/membarrier.c | 15 ---------------
> > 4 files changed, 10 insertions(+), 18 deletions(-)
> > 
> > diff --git a/fs/exec.c b/fs/exec.c
> > index 38b05e01c5bd..325dab98bc51 100644
> > --- a/fs/exec.c
> > +++ b/fs/exec.c
> > @@ -1001,12 +1001,16 @@ static int exec_mmap(struct mm_struct *mm)
> > 	}
> > 
> > 	task_lock(tsk);
> > -	membarrier_exec_mmap(mm);
> > +	/*
> > +	 * membarrier() requires a full barrier before switching mm.
> > +	 */
> > +	smp_mb__after_spinlock();
> > 
> > 	local_irq_disable();
> > 	active_mm = tsk->active_mm;
> > 	tsk->active_mm = mm;
> > 	WRITE_ONCE(tsk->mm, mm);  /* membarrier reads this without locks */
> > +	membarrier_update_current_mm(mm);
> > 	/*
> > 	 * This prevents preemption while active_mm is being loaded and
> > 	 * it and mm are being updated, which could cause problems for
> > diff --git a/include/linux/sched/mm.h b/include/linux/sched/mm.h
> > index e107f292fc42..f1d2beac464c 100644
> > --- a/include/linux/sched/mm.h
> > +++ b/include/linux/sched/mm.h
> > @@ -344,8 +344,6 @@ enum {
> > #include <asm/membarrier.h>
> > #endif
> > 
> > -extern void membarrier_exec_mmap(struct mm_struct *mm);
> > -
> > extern void membarrier_update_current_mm(struct mm_struct *next_mm);
> > 
> > /*
> > diff --git a/kernel/kthread.c b/kernel/kthread.c
> > index 3b18329f885c..18b0a2e0e3b2 100644
> > --- a/kernel/kthread.c
> > +++ b/kernel/kthread.c
> > @@ -1351,6 +1351,11 @@ void kthread_use_mm(struct mm_struct *mm)
> > 	WARN_ON_ONCE(tsk->mm);
> > 
> > 	task_lock(tsk);
> > +	/*
> > +	 * membarrier() requires a full barrier before switching mm.
> > +	 */
> > +	smp_mb__after_spinlock();
> > +
> > 	/* Hold off tlb flush IPIs while switching mm's */
> > 	local_irq_disable();
> > 	active_mm = tsk->active_mm;
> > diff --git a/kernel/sched/membarrier.c b/kernel/sched/membarrier.c
> > index c38014c2ed66..44fafa6e1efd 100644
> > --- a/kernel/sched/membarrier.c
> > +++ b/kernel/sched/membarrier.c
> > @@ -277,21 +277,6 @@ static void ipi_sync_rq_state(void *info)
> > 	smp_mb();
> > }
> > 
> > -void membarrier_exec_mmap(struct mm_struct *mm)
> > -{
> > -	/*
> > -	 * Issue a memory barrier before clearing membarrier_state to
> > -	 * guarantee that no memory access prior to exec is reordered after
> > -	 * clearing this state.
> > -	 */
> > -	smp_mb();
> > -	/*
> > -	 * Keep the runqueue membarrier_state in sync with this mm
> > -	 * membarrier_state.
> > -	 */
> > -	this_cpu_write(runqueues.membarrier_state, 0);
> > -}
> > -
> > void membarrier_update_current_mm(struct mm_struct *next_mm)
> > {
> > 	struct rq *rq = this_rq();
> > --
> > 2.33.1
> 
> -- 
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com