Messages in this thread Patch in this message | | | From | "Aneesh Kumar K.V" <> | Subject | Re: [PATCH v3] mm: Fix kthread_use_mm() vs TLB invalidate | Date | Fri, 21 Aug 2020 11:09:51 +0530 |
| |
Peter Zijlstra <peterz@infradead.org> writes:
> For SMP systems using IPI based TLB invalidation, looking at > current->active_mm is entirely reasonable. This then presents the > following race condition: > > > CPU0 CPU1 > > flush_tlb_mm(mm) use_mm(mm) > <send-IPI> > tsk->active_mm = mm; > <IPI> > if (tsk->active_mm == mm) > // flush TLBs > </IPI> > switch_mm(old_mm,mm,tsk); > > > Where it is possible the IPI flushed the TLBs for @old_mm, not @mm, > because the IPI lands before we actually switched. > > Avoid this by disabling IRQs across changing ->active_mm and > switch_mm(). > > [ There are all sorts of reasons this might be harmless for various > architecture specific reasons, but best not leave the door open at > all. ]
Do we have similar race with exec_mmap()? I am looking at exec_mmap() runnning parallel to do_exit_flush_lazy_tlb(). We can get
if (current->active_mm == mm) {
true and if we don't disable irq around updating tsk->mm/active_mm we can end up doing mmdrop on wrong mm?
> > Cc: stable@kernel.org > Reported-by: Andy Lutomirski <luto@amacapital.net> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> > --- > > Sorry, I dropped the ball on this and only found it because I was > looking at the whole membarrier things vs use_mm(). > > > kernel/kthread.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/kernel/kthread.c b/kernel/kthread.c > index 1d9e2fdfd67a..7221dcbffef3 100644 > --- a/kernel/kthread.c > +++ b/kernel/kthread.c > @@ -1241,13 +1241,15 @@ void kthread_use_mm(struct mm_struct *mm) > WARN_ON_ONCE(tsk->mm); > > task_lock(tsk); > + local_irq_disable(); > active_mm = tsk->active_mm; > if (active_mm != mm) { > mmgrab(mm); > tsk->active_mm = mm; > } > tsk->mm = mm; > - switch_mm(active_mm, mm, tsk); > + switch_mm_irqs_off(active_mm, mm, tsk); > + local_irq_enable(); > task_unlock(tsk); > #ifdef finish_arch_post_lock_switch > finish_arch_post_lock_switch(); > @@ -1276,9 +1278,11 @@ void kthread_unuse_mm(struct mm_struct *mm) > > task_lock(tsk); > sync_mm_rss(mm); > + local_irq_disable(); > tsk->mm = NULL; > /* active_mm is still 'mm' */ > enter_lazy_tlb(mm, tsk); > + local_irq_enable(); > task_unlock(tsk); > } > EXPORT_SYMBOL_GPL(kthread_unuse_mm);
| |