fix possible mm_context wrap-around race

fix possible mm_context wrap-around race

[david mosberger]

Tony, since I can't test the patch on anything but a UP machine at
this time (which is uninteresting, since the race doesn't trigger
there), I'd suggest to put this into the "testing" tree.  Hopefully,
somebody could run some tests which involve multi-threaded apps doing
a lot of fork/exec'ing.

Thanks,

  --david
-- 
Mosberger Consulting LLC, voice/fax: 510-744-9372,
http://www.mosberger-consulting.com/
35706 Runckel Lane, Fremont, CA 94536

Re: fix possible mm_context wrap-around race

[david mosberger]

[-- Attachment #1: Type: text/plain, Size: 770 bytes --]

Argh, some day I'll hopefully remember to attach the actual patch...

  --david

On 7/25/05, david mosberger <dmosberger@gmail.com> wrote:
> Tony, since I can't test the patch on anything but a UP machine at
> this time (which is uninteresting, since the race doesn't trigger
> there), I'd suggest to put this into the "testing" tree.  Hopefully,
> somebody could run some tests which involve multi-threaded apps doing
> a lot of fork/exec'ing.
> 
> Thanks,
> 
>   --david
> --
> Mosberger Consulting LLC, voice/fax: 510-744-9372,
> http://www.mosberger-consulting.com/
> 35706 Runckel Lane, Fremont, CA 94536
> 


-- 
Mosberger Consulting LLC, voice/fax: 510-744-9372,
http://www.mosberger-consulting.com/
35706 Runckel Lane, Fremont, CA 94536

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: mm-context-fix.diff --]
[-- Type: text/x-patch; name="mm-context-fix.diff", Size: 3920 bytes --]

[IA64] Fix race in mm-context wrap-around logic.

The patch below should fix a race which could cause stale TLB entries.
Specifically, when 2 CPUs ended up racing for entrance to
wrap_mmu_context().  The losing CPU would find that by the time it
acquired ctx.lock, mm->context already had a valid value, but then it
failed to (re-)check the delayed TLB flushing logic and hence could
end up using a context number when there were still stale entries in
its TLB.  The fix is to check for delayed TLB flushes only after
mm->context is valid (non-zero).  The patch also makes GCC v4.x
happier by defining a non-volatile variant of mm_context_t called
nv_mm_context_t.

Signed-off-by: David Mosberger-Tang <David.Mosberger@acm.org>

diff --git a/include/asm-ia64/mmu.h b/include/asm-ia64/mmu.h
--- a/include/asm-ia64/mmu.h
+++ b/include/asm-ia64/mmu.h
@@ -2,10 +2,12 @@
 #define __MMU_H
 
 /*
- * Type for a context number.  We declare it volatile to ensure proper ordering when it's
- * accessed outside of spinlock'd critical sections (e.g., as done in activate_mm() and
- * init_new_context()).
+ * Type for a context number.  We declare it volatile to ensure proper
+ * ordering when it's accessed outside of spinlock'd critical sections
+ * (e.g., as done in activate_mm() and init_new_context()).
  */
 typedef volatile unsigned long mm_context_t;
 
+typedef unsigned long nv_mm_context_t;
+
 #endif
diff --git a/include/asm-ia64/mmu_context.h b/include/asm-ia64/mmu_context.h
--- a/include/asm-ia64/mmu_context.h
+++ b/include/asm-ia64/mmu_context.h
@@ -55,34 +55,46 @@ static inline void
 delayed_tlb_flush (void)
 {
 	extern void local_flush_tlb_all (void);
+	unsigned long flags;
 
 	if (unlikely(__ia64_per_cpu_var(ia64_need_tlb_flush))) {
-		local_flush_tlb_all();
-		__ia64_per_cpu_var(ia64_need_tlb_flush) = 0;
+		spin_lock_irqsave(&ia64_ctx.lock, flags);
+		{
+			if (__ia64_per_cpu_var(ia64_need_tlb_flush)) {
+				local_flush_tlb_all();
+				__ia64_per_cpu_var(ia64_need_tlb_flush) = 0;
+			}
+		}
+		spin_unlock_irqrestore(&ia64_ctx.lock, flags);
 	}
 }
 
-static inline mm_context_t
+static inline nv_mm_context_t
 get_mmu_context (struct mm_struct *mm)
 {
 	unsigned long flags;
-	mm_context_t context = mm->context;
-
-	if (context)
-		return context;
+	nv_mm_context_t context = mm->context;
 
-	spin_lock_irqsave(&ia64_ctx.lock, flags);
-	{
-		/* re-check, now that we've got the lock: */
-		context = mm->context;
-		if (context == 0) {
-			cpus_clear(mm->cpu_vm_mask);
-			if (ia64_ctx.next >= ia64_ctx.limit)
-				wrap_mmu_context(mm);
-			mm->context = context = ia64_ctx.next++;
+	if (unlikely(!context)) {
+		spin_lock_irqsave(&ia64_ctx.lock, flags);
+		{
+			/* re-check, now that we've got the lock: */
+			context = mm->context;
+			if (context == 0) {
+				cpus_clear(mm->cpu_vm_mask);
+				if (ia64_ctx.next >= ia64_ctx.limit)
+					wrap_mmu_context(mm);
+				mm->context = context = ia64_ctx.next++;
+			}
 		}
+		spin_unlock_irqrestore(&ia64_ctx.lock, flags);
 	}
-	spin_unlock_irqrestore(&ia64_ctx.lock, flags);
+	/*
+	 * Ensure we're not starting to use "context" before any old
+	 * uses of it are gone from our TLB.
+	 */
+	delayed_tlb_flush();
+
 	return context;
 }
 
@@ -104,7 +116,7 @@ destroy_context (struct mm_struct *mm)
 }
 
 static inline void
-reload_context (mm_context_t context)
+reload_context (nv_mm_context_t context)
 {
 	unsigned long rid;
 	unsigned long rid_incr = 0;
@@ -138,7 +150,7 @@ reload_context (mm_context_t context)
 static inline void
 activate_context (struct mm_struct *mm)
 {
-	mm_context_t context;
+	nv_mm_context_t context;
 
 	do {
 		context = get_mmu_context(mm);
@@ -157,8 +169,6 @@ activate_context (struct mm_struct *mm)
 static inline void
 activate_mm (struct mm_struct *prev, struct mm_struct *next)
 {
-	delayed_tlb_flush();
-
 	/*
 	 * We may get interrupts here, but that's OK because interrupt handlers cannot
 	 * touch user-space.

RE: fix possible mm_context wrap-around race

[Smarduch Mario-CMS063]

Here is a program that exposes the issue on a 2.4
and 2.6 kernel. It pretty easy to reproduce on an 8-way -
takes about 1 or 2 minutes on a 2-way it has not been
reproduced or tried on a 4-way (i.e. no available
system).

- mario
----------------------------

gcc -g -pthread foo.cpp -o foo -lpthread

#include <stdio.h>
#include <errno.h>
#include <pthread.h>
#include <string.h>
#include <unistd.h>
#include <stdlib.h>
#include <sys/wait.h>

void * worker(void *self) {
  int x = -1;
  FILE *pp = 0;
  long junk[8192];
  char buf[1024];

    x = -1;
    errno = 0;
    pp = popen("ls", "r");
    x = errno;
    if(pp) {
      memset(buf, 0, sizeof(buf));
      while(fgets(buf, sizeof(buf)-1, pp) != 0) {
        ;
        }
      pclose(pp);

    }
    else printf("popen failed: %s\n", strerror(errno));
    pthread_exit(NULL);
}

int main(void) {
  pthread_t pt[128];
  pthread_t pthr;
  int x = 0;

  while(1) {
        for (x = 0; x < 5; x++)
                pthread_create(&(pt[x]), 0, worker, (void *)&x);
        for (x = 0; x < 5; x++)
                pthread_join( pt[x], NULL);
        usleep(5000);
        }
  return 0;
}



-----Original Message-----
From: linux-ia64-owner@vger.kernel.org [mailto:linux-ia64-owner@vger.kernel.org] On Behalf Of david mosberger
Sent: Tuesday, July 26, 2005 12:23 AM
To: Luck, Tony
Cc: linux-ia64@vger.kernel.org
Subject: Re: fix possible mm_context wrap-around race


Argh, some day I'll hopefully remember to attach the actual patch...

  --david

On 7/25/05, david mosberger <dmosberger@gmail.com> wrote:
> Tony, since I can't test the patch on anything but a UP machine at 
> this time (which is uninteresting, since the race doesn't trigger 
> there), I'd suggest to put this into the "testing" tree.  Hopefully, 
> somebody could run some tests which involve multi-threaded apps doing 
> a lot of fork/exec'ing.
> 
> Thanks,
> 
>   --david
> --
> Mosberger Consulting LLC, voice/fax: 510-744-9372, 
> http://www.mosberger-consulting.com/
> 35706 Runckel Lane, Fremont, CA 94536
> 


-- 
Mosberger Consulting LLC, voice/fax: 510-744-9372, http://www.mosberger-consulting.com/
35706 Runckel Lane, Fremont, CA 94536