----- Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx> wrote:
> ----- On Jan 8, 2022, at 11:43 AM, Andy Lutomirski luto@xxxxxxxxxx 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@xxxxxxxxxx>
> > ---
> > 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
