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

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

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

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