[PATCH v2] mm: Fix use_mm() vs TLB invalidate

[PATCH v2] mm: Fix use_mm() vs TLB invalidate

[Peter Zijlstra]

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

Cc: stable@kernel.org
Reported-by: Andy Lutomirski <luto@amacapital.net>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
v2 now with WARN_ON_ONCE

 mmu_context.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Index: linux-2.6/mm/mmu_context.c
===================================================================
--- linux-2.6.orig/mm/mmu_context.c
+++ linux-2.6/mm/mmu_context.c
@@ -24,14 +24,19 @@ void use_mm(struct mm_struct *mm)
 	struct mm_struct *active_mm;
 	struct task_struct *tsk = current;
 
+	WARN_ON(!(tsk->flags & PF_KTHREAD));
+	WARN_ON(tsk->mm != NULL);
+
 	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();
@@ -54,11 +59,15 @@ void unuse_mm(struct mm_struct *mm)
 {
 	struct task_struct *tsk = current;
 
+	WARN_ON(!(tsk->flags & PF_KTHREAD));
+
 	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(unuse_mm);

Re: [PATCH v2] mm: Fix use_mm() vs TLB invalidate

[Jens Axboe]

On 2/26/20 6:21 AM, Peter Zijlstra wrote:
> 
> 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. ]

Not that I'm worried about it breaking anything, but ran it through
the usual testing and might as well report:

Tested-by: Jens Axboe <axboe@kernel.dk>

-- 
Jens Axboe

Re: [PATCH v2] mm: Fix use_mm() vs TLB invalidate

[Kees Cook]

On Wed, Feb 26, 2020 at 02:21:33PM +0100, Peter Zijlstra wrote:
> v2 now with WARN_ON_ONCE
> [...]
> +	WARN_ON(!(tsk->flags & PF_KTHREAD));
> +	WARN_ON(tsk->mm != NULL);
> [...]
> +	WARN_ON(!(tsk->flags & PF_KTHREAD));

Version mismatch?

-- 
Kees Cook

Re: [PATCH v2] mm: Fix use_mm() vs TLB invalidate

[Peter Zijlstra]

On Wed, Feb 26, 2020 at 01:36:53PM -0800, Kees Cook wrote:
> On Wed, Feb 26, 2020 at 02:21:33PM +0100, Peter Zijlstra wrote:
> > v2 now with WARN_ON_ONCE
> > [...]
> > +	WARN_ON(!(tsk->flags & PF_KTHREAD));
> > +	WARN_ON(tsk->mm != NULL);
> > [...]
> > +	WARN_ON(!(tsk->flags & PF_KTHREAD));
> 
> Version mismatch?

Dammit; no, that's just me being an idiot for doing too many things at
once :/

I suppose I'll go send v3..