| Message ID | 74ace142f48db7d0e71b05b5ace72bfe8e0a2652.1623813516.git.luto@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | membarrier cleanups | expand |
Excerpts from Andy Lutomirski's message of June 16, 2021 1:21 pm: > membarrier reads cpu_rq(remote cpu)->curr->mm without locking. Use > READ_ONCE() and WRITE_ONCE() to remove the data races. > > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > Cc: Nicholas Piggin <npiggin@gmail.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Signed-off-by: Andy Lutomirski <luto@kernel.org> > --- > fs/exec.c | 2 +- > kernel/kthread.c | 4 ++-- > kernel/sched/membarrier.c | 6 +++--- > 3 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/fs/exec.c b/fs/exec.c > index 18594f11c31f..2e63dea83411 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -1007,7 +1007,7 @@ static int exec_mmap(struct mm_struct *mm) > local_irq_disable(); > active_mm = tsk->active_mm; > tsk->active_mm = mm; > - tsk->mm = mm; > + WRITE_ONCE(tsk->mm, mm); /* membarrier reads this without locks */ > /* > * This prevents preemption while active_mm is being loaded and > * it and mm are being updated, which could cause problems for > diff --git a/kernel/kthread.c b/kernel/kthread.c > index 8275b415acec..4962794e02d5 100644 > --- a/kernel/kthread.c > +++ b/kernel/kthread.c > @@ -1322,7 +1322,7 @@ void kthread_use_mm(struct mm_struct *mm) > mmgrab(mm); > tsk->active_mm = mm; > } > - tsk->mm = mm; > + WRITE_ONCE(tsk->mm, mm); /* membarrier reads this without locks */ > membarrier_update_current_mm(mm); > switch_mm_irqs_off(active_mm, mm, tsk); > membarrier_finish_switch_mm(atomic_read(&mm->membarrier_state)); > @@ -1363,7 +1363,7 @@ void kthread_unuse_mm(struct mm_struct *mm) > smp_mb__after_spinlock(); > sync_mm_rss(mm); > local_irq_disable(); > - tsk->mm = NULL; > + WRITE_ONCE(tsk->mm, NULL); /* membarrier reads this without locks */ > membarrier_update_current_mm(NULL); > /* active_mm is still 'mm' */ > enter_lazy_tlb(mm, tsk); > diff --git a/kernel/sched/membarrier.c b/kernel/sched/membarrier.c > index 3173b063d358..c32c32a2441e 100644 > --- a/kernel/sched/membarrier.c > +++ b/kernel/sched/membarrier.c > @@ -410,7 +410,7 @@ static int membarrier_private_expedited(int flags, int cpu_id) > goto out; > rcu_read_lock(); > p = rcu_dereference(cpu_rq(cpu_id)->curr); > - if (!p || p->mm != mm) { > + if (!p || READ_ONCE(p->mm) != mm) { > rcu_read_unlock(); > goto out; > } > @@ -423,7 +423,7 @@ static int membarrier_private_expedited(int flags, int cpu_id) > struct task_struct *p; > > p = rcu_dereference(cpu_rq(cpu)->curr); > - if (p && p->mm == mm) > + if (p && READ_ONCE(p->mm) == mm) /* exec, kthread_use_mm write this without locks */ ? Seems good to me. Acked-by: Nicholas Piggin <npiggin@gmail.com> > __cpumask_set_cpu(cpu, tmpmask); > } > rcu_read_unlock(); > @@ -521,7 +521,7 @@ static int sync_runqueues_membarrier_state(struct mm_struct *mm) > struct task_struct *p; > > p = rcu_dereference(rq->curr); > - if (p && p->mm == mm) > + if (p && READ_ONCE(p->mm) == mm) > __cpumask_set_cpu(cpu, tmpmask); > } > rcu_read_unlock(); > -- > 2.31.1 > >
----- On Jun 15, 2021, at 11:21 PM, Andy Lutomirski luto@kernel.org wrote: > membarrier reads cpu_rq(remote cpu)->curr->mm without locking. Use > READ_ONCE() and WRITE_ONCE() to remove the data races. I notice that kernel/exit.c:exit_mm() also has: current->mm = NULL; I suspect you may want to add a WRITE_ONCE() there as well ? Thanks, Mathieu
On 6/16/21 11:08 AM, Mathieu Desnoyers wrote: > ----- On Jun 15, 2021, at 11:21 PM, Andy Lutomirski luto@kernel.org wrote: > >> membarrier reads cpu_rq(remote cpu)->curr->mm without locking. Use >> READ_ONCE() and WRITE_ONCE() to remove the data races. > > I notice that kernel/exit.c:exit_mm() also has: > > current->mm = NULL; > > I suspect you may want to add a WRITE_ONCE() there as well ? Good catch. I was thinking that exit_mm() couldn't execute concurrently with membarrier(), but that's wrong. --Andy
diff --git a/fs/exec.c b/fs/exec.c index 18594f11c31f..2e63dea83411 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1007,7 +1007,7 @@ static int exec_mmap(struct mm_struct *mm) local_irq_disable(); active_mm = tsk->active_mm; tsk->active_mm = mm; - tsk->mm = mm; + WRITE_ONCE(tsk->mm, mm); /* membarrier reads this without locks */ /* * This prevents preemption while active_mm is being loaded and * it and mm are being updated, which could cause problems for diff --git a/kernel/kthread.c b/kernel/kthread.c index 8275b415acec..4962794e02d5 100644 --- a/kernel/kthread.c +++ b/kernel/kthread.c @@ -1322,7 +1322,7 @@ void kthread_use_mm(struct mm_struct *mm) mmgrab(mm); tsk->active_mm = mm; } - tsk->mm = mm; + WRITE_ONCE(tsk->mm, mm); /* membarrier reads this without locks */ membarrier_update_current_mm(mm); switch_mm_irqs_off(active_mm, mm, tsk); membarrier_finish_switch_mm(atomic_read(&mm->membarrier_state)); @@ -1363,7 +1363,7 @@ void kthread_unuse_mm(struct mm_struct *mm) smp_mb__after_spinlock(); sync_mm_rss(mm); local_irq_disable(); - tsk->mm = NULL; + WRITE_ONCE(tsk->mm, NULL); /* membarrier reads this without locks */ membarrier_update_current_mm(NULL); /* active_mm is still 'mm' */ enter_lazy_tlb(mm, tsk); diff --git a/kernel/sched/membarrier.c b/kernel/sched/membarrier.c index 3173b063d358..c32c32a2441e 100644 --- a/kernel/sched/membarrier.c +++ b/kernel/sched/membarrier.c @@ -410,7 +410,7 @@ static int membarrier_private_expedited(int flags, int cpu_id) goto out; rcu_read_lock(); p = rcu_dereference(cpu_rq(cpu_id)->curr); - if (!p || p->mm != mm) { + if (!p || READ_ONCE(p->mm) != mm) { rcu_read_unlock(); goto out; } @@ -423,7 +423,7 @@ static int membarrier_private_expedited(int flags, int cpu_id) struct task_struct *p; p = rcu_dereference(cpu_rq(cpu)->curr); - if (p && p->mm == mm) + if (p && READ_ONCE(p->mm) == mm) __cpumask_set_cpu(cpu, tmpmask); } rcu_read_unlock(); @@ -521,7 +521,7 @@ static int sync_runqueues_membarrier_state(struct mm_struct *mm) struct task_struct *p; p = rcu_dereference(rq->curr); - if (p && p->mm == mm) + if (p && READ_ONCE(p->mm) == mm) __cpumask_set_cpu(cpu, tmpmask); } rcu_read_unlock();
membarrier reads cpu_rq(remote cpu)->curr->mm without locking. Use READ_ONCE() and WRITE_ONCE() to remove the data races. Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Cc: Nicholas Piggin <npiggin@gmail.com> Cc: Peter Zijlstra <peterz@infradead.org> Signed-off-by: Andy Lutomirski <luto@kernel.org> --- fs/exec.c | 2 +- kernel/kthread.c | 4 ++-- kernel/sched/membarrier.c | 6 +++--- 3 files changed, 6 insertions(+), 6 deletions(-)