Fix preempt unsafe paths accessing per_cpu variables

Hi,
This patch fixes various paths in the -rt kernel on powerpc64 where per_cpu variables are accessed in a preempt unsafe way.
When a power box with -rt kernel is booted, multiple BUG messages are generated "BUG: init:1 task might have lost a preemption check!". After booting a kernel with these patches applied, these messages don't appear.

Also I ran the realtime tests from ltp to ensure the stability.

Signed-off-by Chirag <[email protected]>

arch/powerpc/mm/tlb_64.c               |   31 ++++++++++++++++---------------
arch/powerpc/platforms/pseries/iommu.c |   14 ++++++++++----
include/asm-powerpc/tlb.h              |    5 ++---
3 files changed, 28 insertions(+), 22 deletions(-)

Index: linux-2.6.25.8-rt7/arch/powerpc/mm/tlb_64.c
===================================================================

arch/powerpc/mm/tlb_64.c +16 -15

@@ -38,7 +38,6 @@
  * include/asm-powerpc/tlb.h file -- tgall
  */
 DEFINE_PER_CPU_LOCKED(struct mmu_gather, mmu_gathers);
-DEFINE_PER_CPU(struct pte_freelist_batch *, pte_freelist_cur);
 unsigned long pte_freelist_forced_free;
 
 struct pte_freelist_batch

@@ -48,7 +47,7 @@
 	pgtable_free_t	tables[0];
 };
 
-DEFINE_PER_CPU(struct pte_freelist_batch *, pte_freelist_cur);
+DEFINE_PER_CPU_LOCKED(struct pte_freelist_batch *, pte_freelist_cur);
 unsigned long pte_freelist_forced_free;
 
 #define PTE_FREELIST_SIZE \

@@ -92,24 +91,21 @@
 
 void pgtable_free_tlb(struct mmu_gather *tlb, pgtable_free_t pgf)
 {
-	/*
-	 * This is safe since tlb_gather_mmu has disabled preemption.
-	 * tlb->cpu is set by tlb_gather_mmu as well.
-	 */
+	int cpu;
         cpumask_t local_cpumask = cpumask_of_cpu(tlb->cpu);
-	struct pte_freelist_batch **batchp = &__get_cpu_var(pte_freelist_cur);
+	struct pte_freelist_batch **batchp = &get_cpu_var_locked(pte_freelist_cur, &cpu);
 
 	if (atomic_read(&tlb->mm->mm_users) < 2 ||
 	    cpus_equal(tlb->mm->cpu_vm_mask, local_cpumask)) {
 		pgtable_free(pgf);
-		return;
+		goto cleanup;
 	}
 
 	if (*batchp == NULL) {
 		*batchp = (struct pte_freelist_batch *)__get_free_page(GFP_ATOMIC);
 		if (*batchp == NULL) {
 			pgtable_free_now(pgf);
-			return;
+			goto cleanup;
 		}
 		(*batchp)->index = 0;
 	}

@@ -118,6 +114,9 @@
 		pte_free_submit(*batchp);
 		*batchp = NULL;
 	}
+
+ cleanup:
+	put_cpu_var_locked(pte_freelist_cur, cpu);
 }
 
 /*

@@ -253,13 +252,15 @@
 
 void pte_free_finish(void)
 {
-	/* This is safe since tlb_gather_mmu has disabled preemption */
-	struct pte_freelist_batch **batchp = &__get_cpu_var(pte_freelist_cur);
+	int cpu;
+	struct pte_freelist_batch **batchp = &get_cpu_var_locked(pte_freelist_cur, &cpu);
 
-	if (*batchp == NULL)
-		return;
-	pte_free_submit(*batchp);
-	*batchp = NULL;
+	if (*batchp) {
+		pte_free_submit(*batchp);
+		*batchp = NULL;
+	}
+
+	put_cpu_var_locked(pte_freelist_cur, cpu);
 }
 
 /**

Index: linux-2.6.25.8-rt7/include/asm-powerpc/tlb.h
===================================================================

include/asm-powerpc/tlb.h +2 -3

@@ -40,18 +40,17 @@
 
 static inline void tlb_flush(struct mmu_gather *tlb)
 {
-	struct ppc64_tlb_batch *tlbbatch = &__get_cpu_var(ppc64_tlb_batch);
+	struct ppc64_tlb_batch *tlbbatch = &get_cpu_var(ppc64_tlb_batch);
 
 	/* If there's a TLB batch pending, then we must flush it because the
 	 * pages are going to be freed and we really don't want to have a CPU
 	 * access a freed page because it has a stale TLB
 	 */
 	if (tlbbatch->index) {
-		preempt_disable();
 		__flush_tlb_pending(tlbbatch);
-		preempt_enable();
 	}
 
+	put_cpu_var(ppc64_tlb_batch);
 	pte_free_finish();
 }
 

Index: linux-2.6.25.8-rt7/arch/powerpc/platforms/pseries/iommu.c
===================================================================

arch/powerpc/platforms/pseries/iommu.c +10 -5

@@ -124,7 +124,7 @@
 	}
 }
 
-static DEFINE_PER_CPU(u64 *, tce_page) = NULL;
+static DEFINE_PER_CPU_LOCKED(u64 *, tce_page) = NULL;
 
 static void tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
 				     long npages, unsigned long uaddr,

@@ -135,12 +135,13 @@
 	u64 *tcep;
 	u64 rpn;
 	long l, limit;
+	int cpu;
 
 	if (npages == 1)
 		return tce_build_pSeriesLP(tbl, tcenum, npages, uaddr,
 					   direction);
 
-	tcep = __get_cpu_var(tce_page);
+	tcep = get_cpu_var_locked(tce_page, &cpu);
 
 	/* This is safe to do since interrupts are off when we're called
 	 * from iommu_alloc{,_sg}()

@@ -148,10 +149,13 @@
 	if (!tcep) {
 		tcep = (u64 *)__get_free_page(GFP_ATOMIC);
 		/* If allocation fails, fall back to the loop implementation */
-		if (!tcep)
+		if (!tcep) {
+			put_cpu_var_locked(tce_page, cpu);
 			return tce_build_pSeriesLP(tbl, tcenum, npages,
 						   uaddr, direction);
-		__get_cpu_var(tce_page) = tcep;
+		}
+
+		per_cpu_var_locked(tce_page, cpu) = tcep;
 	}
 
 	rpn = (virt_to_abs(uaddr)) >> TCE_SHIFT;

@@ -188,6 +192,8 @@
 		printk("\ttce[0] val = 0x%lx\n", tcep[0]);
 		show_stack(current, (unsigned long *)__get_SP());
 	}
+
+	put_cpu_var_locked(tce_page, cpu);
 }
 
 static void tce_free_pSeriesLP(struct iommu_table *tbl, long tcenum, long npages)
--

To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Hi,
This patch fixes various paths in the -rt kernel on powerpc64 where per_cpu variables are accessed in a preempt unsafe way.
When a power box with -rt kernel is booted, multiple BUG messages are generated "BUG: init:1 task might have lost a preemption check!". After booting a kernel with these patches applied, these messages don't appear.

Also I ran the realtime tests from ltp to ensure the stability.

Signed-off-by Chirag <[email protected]>

arch/powerpc/mm/tlb_64.c               |   31 ++++++++++++++++---------------
arch/powerpc/platforms/pseries/iommu.c |   14 ++++++++++----
include/asm-powerpc/tlb.h              |    5 ++---
3 files changed, 28 insertions(+), 22 deletions(-)

Index: linux-2.6.25.8-rt7/arch/powerpc/mm/tlb_64.c
===================================================================

arch/powerpc/mm/tlb_64.c +16 -15

@@ -38,7 +38,6 @@
  * include/asm-powerpc/tlb.h file -- tgall
  */
 DEFINE_PER_CPU_LOCKED(struct mmu_gather, mmu_gathers);
-DEFINE_PER_CPU(struct pte_freelist_batch *, pte_freelist_cur);
 unsigned long pte_freelist_forced_free;
 
 struct pte_freelist_batch

@@ -48,7 +47,7 @@
 	pgtable_free_t	tables[0];
 };
 
-DEFINE_PER_CPU(struct pte_freelist_batch *, pte_freelist_cur);
+DEFINE_PER_CPU_LOCKED(struct pte_freelist_batch *, pte_freelist_cur);
 unsigned long pte_freelist_forced_free;
 
 #define PTE_FREELIST_SIZE \

@@ -92,24 +91,21 @@
 
 void pgtable_free_tlb(struct mmu_gather *tlb, pgtable_free_t pgf)
 {
-	/*
-	 * This is safe since tlb_gather_mmu has disabled preemption.
-	 * tlb->cpu is set by tlb_gather_mmu as well.
-	 */
+	int cpu;
         cpumask_t local_cpumask = cpumask_of_cpu(tlb->cpu);
-	struct pte_freelist_batch **batchp = &__get_cpu_var(pte_freelist_cur);
+	struct pte_freelist_batch **batchp = &get_cpu_var_locked(pte_freelist_cur, &cpu);
 
 	if (atomic_read(&tlb->mm->mm_users) < 2 ||
 	    cpus_equal(tlb->mm->cpu_vm_mask, local_cpumask)) {
 		pgtable_free(pgf);
-		return;
+		goto cleanup;
 	}
 
 	if (*batchp == NULL) {
 		*batchp = (struct pte_freelist_batch *)__get_free_page(GFP_ATOMIC);
 		if (*batchp == NULL) {
 			pgtable_free_now(pgf);
-			return;
+			goto cleanup;
 		}
 		(*batchp)->index = 0;
 	}

@@ -118,6 +114,9 @@
 		pte_free_submit(*batchp);
 		*batchp = NULL;
 	}
+
+ cleanup:
+	put_cpu_var_locked(pte_freelist_cur, cpu);
 }
 
 /*

@@ -253,13 +252,15 @@
 
 void pte_free_finish(void)
 {
-	/* This is safe since tlb_gather_mmu has disabled preemption */
-	struct pte_freelist_batch **batchp = &__get_cpu_var(pte_freelist_cur);
+	int cpu;
+	struct pte_freelist_batch **batchp = &get_cpu_var_locked(pte_freelist_cur, &cpu);
 
-	if (*batchp == NULL)
-		return;
-	pte_free_submit(*batchp);
-	*batchp = NULL;
+	if (*batchp) {
+		pte_free_submit(*batchp);
+		*batchp = NULL;
+	}
+
+	put_cpu_var_locked(pte_freelist_cur, cpu);
 }
 
 /**

Index: linux-2.6.25.8-rt7/include/asm-powerpc/tlb.h
===================================================================

include/asm-powerpc/tlb.h +2 -3

@@ -40,18 +40,17 @@
 
 static inline void tlb_flush(struct mmu_gather *tlb)
 {
-	struct ppc64_tlb_batch *tlbbatch = &__get_cpu_var(ppc64_tlb_batch);
+	struct ppc64_tlb_batch *tlbbatch = &get_cpu_var(ppc64_tlb_batch);
 
 	/* If there's a TLB batch pending, then we must flush it because the
 	 * pages are going to be freed and we really don't want to have a CPU
 	 * access a freed page because it has a stale TLB
 	 */
 	if (tlbbatch->index) {
-		preempt_disable();
 		__flush_tlb_pending(tlbbatch);
-		preempt_enable();
 	}
 
+	put_cpu_var(ppc64_tlb_batch);
 	pte_free_finish();
 }
 

Index: linux-2.6.25.8-rt7/arch/powerpc/platforms/pseries/iommu.c
===================================================================

arch/powerpc/platforms/pseries/iommu.c +10 -4

@@ -124,7 +124,7 @@
 	}
 }
 
-static DEFINE_PER_CPU(u64 *, tce_page) = NULL;
+static DEFINE_PER_CPU_LOCKED(u64 *, tce_page) = NULL;
 
 static void tce_buildmulti_pSeriesLP(struct iommu_table *tbl, long tcenum,
 				     long npages, unsigned long uaddr,

@@ -135,12 +135,13 @@
 	u64 *tcep;
 	u64 rpn;
 	long l, limit;
+	int cpu;
 
 	if (npages == 1)
 		return tce_build_pSeriesLP(tbl, tcenum, npages, uaddr,
 					   direction);
 
-	tcep = __get_cpu_var(tce_page);
+	tcep = get_cpu_var_locked(tce_page, &cpu);
 
 	/* This is safe to do since interrupts are off when we're called
 	 * from iommu_alloc{,_sg}()

@@ -148,10 +149,13 @@
 	if (!tcep) {
 		tcep = (u64 *)__get_free_page(GFP_ATOMIC);
 		/* If allocation fails, fall back to the loop implementation */
-		if (!tcep)
+		if (!tcep) {
+			put_cpu_var_locked(tce_page, cpu);
 			return tce_build_pSeriesLP(tbl, tcenum, npages,
 						   uaddr, direction);
-		__get_cpu_var(tce_page) = tcep;
+		}
+
+		per_cpu_var_locked(tce_page, cpu) = tcep;
 	}
 
 	rpn = (virt_to_abs(uaddr)) >> TCE_SHIFT;

@@ -188,6 +192,8 @@
 		printk("\ttce[0] val = 0x%lx\n", tcep[0]);
 		show_stack(current, (unsigned long *)__get_SP());
 	}
+
+	put_cpu_var_locked(tce_page, cpu);
 }
 
 static void tce_free_pSeriesLP(struct iommu_table *tbl, long tcenum, long npages)

_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Quoted 9 lines · Chirag Jog

Hi,
This patch fixes various paths in the -rt kernel on powerpc64 where per_cpu variables are accessed in a preempt unsafe way.
When a power box with -rt kernel is booted, multiple BUG messages are generated "BUG: init:1 task might have lost a preemption check!". After booting a kernel with these patches applied, these messages don't appear.

Also I ran the realtime tests from ltp to ensure the stability.

That sounds bad tho...

IE. You are changing the code to lock/unlock on all those TLB batching operations, but seem to miss the core reason why it was done that way:
ie, the code assumes that it will not change CPU -between- those calls, since the whole stuff should be already have been within a per-cpu locked section at the caller level.

All these operations are done assuming that tlb_gather_mmu disables preemption and tlb_finish_mmu enables preemption again. This is not true for -rt.
For x86, none of the code paths between tlb_gather_mmu and tlb_finish_mmu access any per_cpu variables.
But this is not true for powerpc64 as we can see.

One way could be to make tlb_gather_mmu disable preemption as it does in mainline but only for powerpc.
Although i am not sure, if this is the right step ahead.

I am attaching a patch below for the same.
I have left out the tce bits, as they are fine.

Note: I haven't extensively tested the patch

Quoted 15 lines

All these operations are done assuming that tlb_gather_mmu disables preemption and tlb_finish_mmu enables preemption again. This is not true for -rt.
For x86, none of the code paths between tlb_gather_mmu and tlb_finish_mmu access any per_cpu variables.
But this is not true for powerpc64 as we can see.

One way could be to make tlb_gather_mmu disable preemption as it does in mainline but only for powerpc.
Although i am not sure, if this is the right step ahead.

I am attaching a patch below for the same.
I have left out the tce bits, as they are fine.

Quoted 1 line

Note: I haven't extensively tested the patch

A better option is to make sure that a context switch does the right thing, flushing the pending batch. I think that's already the case, which means that your original patch may work, but that needs to be double-checked and commented properly.

With the original patch, the pending batch does get flushed in a non-preemptable region.
I am resending the original with just adding the necesary comments.

Quoted 4 lines

With the original patch, the pending batch does get flushed in a non-preemptable region.
I am resending the original with just adding the necesary comments.

Your comment isn't what I meant. What I meant is that if the process is context switched while walking the page tables, the low level powerpc context switch code should also perform a __flush_tlb_pending.

BTW. Is the pte_lock also not a real spinlock anymore ? That may break other assumptions the powerpc mm code is doing.

This -rt stuff is just too scary, it changes some fundamental semantics of the spinlocks. yuck.

There's lots of semantics that are changed with -rt that should make everything still work ;-) Some spinlocks remain real spinlocks, but we shouldn't have a problem with most being mutexes.

There's some cases that uses per CPU variables or other per cpu actions that require a special CPU_LOCK that protects the data in a preemption mode. The slab.c code in -rt handles this.

Quoted 4 lines

With the original patch, the pending batch does get flushed in a non-preemptable region.
I am resending the original with just adding the necesary comments.

Your comment isn't what I meant. What I meant is that if the process is context switched while walking the page tables, the low level powerpc context switch code should also perform a ???__flush_tlb_pending.

BTW. Is the pte_lock also not a real spinlock anymore ? That may break other assumptions the powerpc mm code is doing.

This -rt stuff is just too scary, it changes some fundamental semantics of the spinlocks. yuck.

Ben.