ppc32: fix destroy_context() race condition

PATCH In review Guillaume Autran 3 participants 5 messages Jul 12, 2005 – Jul 13, 2005

Share & tools

Guillaume Autran

author

2005-07-12 14:22 UTC #0 lore raw

PATCH 1/1 ppc32: fix destroy_context() race condition

Fix for a race condition when a task gets preempted by another task while executing the destroy_context(...) in a FEW_CONTEXTS environment. mm->context == NO_CONTEXT but the context_map may indicate all contexts are in use.
The solution to this problem is to disable kernel preemption while destroying a MMU context.

Signed-off-by Guillaume Autran <[email protected]>

---

include/asm-ppc/mmu_context.h +0 -0

diff -Nru a/include/asm-ppc/mmu_context.h b/include/asm-ppc/mmu_context.h
@@ -149,6 +149,7 @@

*/
static inline void destroy_context(struct mm_struct *mm) {

+	preempt_disable();
	if (mm->context != NO_CONTEXT) {
		clear_bit(mm->context, context_map);
		mm->context = NO_CONTEXT;
@@ -156,6 +157,7 @@
		atomic_inc(&nr_free_contexts);
#endif
	}

+ preempt_enable();
}

static inline void switch_mm(struct mm_struct *prev, struct mm_struct *next,

Eugene Surovegin

2005-07-12 16:39 UTC #1 lore raw

Quoted 24 lines · Guillaume Autran

Fix for a race condition when a task gets preempted by another task while executing the destroy_context(...) in a FEW_CONTEXTS environment. mm->context == NO_CONTEXT but the context_map may indicate all contexts are in use.
The solution to this problem is to disable kernel preemption while destroying a MMU context.
Signed-off-by Guillaume Autran <[email protected]>
---
diff -Nru a/include/asm-ppc/mmu_context.h b/include/asm-ppc/mmu_context.h
include/asm-ppc/mmu_context.h +1 -0
@@ -149,6 +149,7 @@
 */
 static inline void destroy_context(struct mm_struct *mm)
 {
+	preempt_disable();
 	if (mm->context != NO_CONTEXT) {
 		clear_bit(mm->context, context_map);
 		mm->context = NO_CONTEXT;

include/asm-ppc/mmu_context.h line 163

Quoted 7 lines

include/asm-ppc/mmu_context.h +1 -0

@@ -156,6 +157,7 @@
 		atomic_inc(&nr_free_contexts);
 #endif
 	}
+	preempt_enable();
 }
 
 static inline void switch_mm(struct mm_struct *prev, struct mm_struct *next,

Could you explain, why this patch is needed?

AFAIK, destroy_context() is only called from switch_mm(), which in turn is called from schedule() with preemption already disabled. If not, IMHO we have bigger problems, and "fixing" destroy_context() will only hide such problems.

Signature · 1 line

Eugene

Guillaume Autran

author

2005-07-12 18:27 UTC #2 lore raw

Eugene, destroy_context(..) can be called from other place outside of the scheduler and switch_mm(...). See mmdrop(...) for example. In many of those cases, the task will run with preemption enable.

Guillaume.

Quoted 43 lines · Eugene Surovegin

Quoted 26 lines · Guillaume Autran
Fix for a race condition when a task gets preempted by another task while executing the destroy_context(...) in a FEW_CONTEXTS environment. mm->context == NO_CONTEXT but the context_map may indicate all contexts are in use.
The solution to this problem is to disable kernel preemption while destroying a MMU context.
Signed-off-by Guillaume Autran <[email protected]>
---
diff -Nru a/include/asm-ppc/mmu_context.h b/include/asm-ppc/mmu_context.h
include/asm-ppc/mmu_context.h +1 -0
@@ -149,6 +149,7 @@
 */
 static inline void destroy_context(struct mm_struct *mm)
 {
+	preempt_disable();
 	if (mm->context != NO_CONTEXT) {
 		clear_bit(mm->context, context_map);
 		mm->context = NO_CONTEXT;
include/asm-ppc/mmu_context.h line 163
Quoted 7 lines
include/asm-ppc/mmu_context.h +1 -0
@@ -156,6 +157,7 @@
 		atomic_inc(&nr_free_contexts);
 #endif
 	}
+	preempt_enable();
 }
 
 static inline void switch_mm(struct mm_struct *prev, struct mm_struct *next,
Could you explain, why this patch is needed?
AFAIK, destroy_context() is only called from switch_mm(), which in turn is called from schedule() with preemption already disabled. If not, IMHO we have bigger problems, and "fixing" destroy_context() will only hide such problems.

Signature · 7 lines

=======================================
Guillaume Autran
Senior Software Engineer
MRV Communications, Inc.
Tel: (978) 952-4932 office
E-mail: [email protected]
=======================================

Eugene Surovegin

2005-07-12 18:45 UTC #3 lore raw

Quoted 3 lines · Guillaume Autran

Eugene, destroy_context(..) can be called from other place outside of the scheduler and switch_mm(...). See mmdrop(...) for example. In many of those cases, the task will run with preemption enable.

Ahh, OK, you're right. Maybe it's worth mentioning in the patch description :).

Signature · 1 line

Eugene

Benjamin Herrenschmidt

Acked-by on v1 1/1

2005-07-13 00:11 UTC #4 lore raw

In reply to Guillaume Autran

Quoted 8 lines · Guillaume Autran

Fix for a race condition when a task gets preempted by another task while executing the destroy_context(...) in a FEW_CONTEXTS environment. mm->context == NO_CONTEXT but the context_map may indicate all contexts are in use.
The solution to this problem is to disable kernel preemption while destroying a MMU context.
Signed-off-by Guillaume Autran <[email protected]>

Acked-by Benjamin Herrenschmidt <[email protected]>

Quoted 27 lines

---

diff -Nru a/include/asm-ppc/mmu_context.h b/include/asm-ppc/mmu_context.h

include/asm-ppc/mmu_context.h +2 -0

@@ -149,6 +149,7 @@
 */
 static inline void destroy_context(struct mm_struct *mm)
 {
+	preempt_disable();
 	if (mm->context != NO_CONTEXT) {
 		clear_bit(mm->context, context_map);
 		mm->context = NO_CONTEXT;

@@ -156,6 +157,7 @@
 		atomic_inc(&nr_free_contexts);
 #endif
 	}
+	preempt_enable();
 }
 
 static inline void switch_mm(struct mm_struct *prev, struct mm_struct *next,