* Re: fix possible mm_context wrap-around race
2005-07-26 5:20 fix possible mm_context wrap-around race david mosberger
@ 2005-07-26 5:22 ` david mosberger
2005-07-28 21:34 ` Smarduch Mario-CMS063
1 sibling, 0 replies; 3+ messages in thread
From: david mosberger @ 2005-07-26 5:22 UTC (permalink / raw)
To: linux-ia64
[-- 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.
^ permalink raw reply [flat|nested] 3+ messages in thread* RE: fix possible mm_context wrap-around race
2005-07-26 5:20 fix possible mm_context wrap-around race david mosberger
2005-07-26 5:22 ` david mosberger
@ 2005-07-28 21:34 ` Smarduch Mario-CMS063
1 sibling, 0 replies; 3+ messages in thread
From: Smarduch Mario-CMS063 @ 2005-07-28 21:34 UTC (permalink / raw)
To: linux-ia64
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
^ permalink raw reply [flat|nested] 3+ messages in thread