[PATCH] powerpc/mm: bail out early when flushing TLB page

MMU_NO_CONTEXT is conditionally defined as 0 or (unsigned int)-1. However,
in __flush_tlb_page() a corresponding variable is only tested for open
coded 0, which can cause NULL pointer dereference if `mm' argument was
legitimately passed as such.

Bail out early in case the first argument is NULL, thus eliminate confusion
between different values of MMU_NO_CONTEXT and avoid disabling and then
re-enabling preemption unnecessarily.

Signed-off-by: Arseny Solokha <redacted>
---
 arch/powerpc/mm/tlb_nohash.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/mm/tlb_nohash.c b/arch/powerpc/mm/tlb_nohash.c
index f38ea4d..ab0616b 100644
--- a/arch/powerpc/mm/tlb_nohash.c
+++ b/arch/powerpc/mm/tlb_nohash.c

@@ -284,8 +284,11 @@ void __flush_tlb_page(struct mm_struct *mm, unsigned long vmaddr,
 	struct cpumask *cpu_mask;
 	unsigned int pid;
 
+	if (unlikely(!mm))
+		return;
+
 	preempt_disable();
-	pid = mm ? mm->context.id : 0;
+	pid = mm->context.id;
 	if (unlikely(pid == MMU_NO_CONTEXT))
 		goto bail;
 	cpu_mask = mm_cpumask(mm);

-- 
2.2.2

On Fri, 2015-01-30 at 19:08 +0700, Arseny Solokha wrote:

MMU_NO_CONTEXT is conditionally defined as 0 or (unsigned int)-1.

For nohash it is specifically -1.

 However, in __flush_tlb_page() a corresponding variable is only tested
for open coded 0, which can cause NULL pointer dereference if `mm'
argument was legitimately passed as such.

Bail out early in case the first argument is NULL, thus eliminate confusion
between different values of MMU_NO_CONTEXT and avoid disabling and then
re-enabling preemption unnecessarily.

How did you notice this?  Did you see an oops, or was it code
inspection?  I'm wondering what codepath gets here with mm == NULL.

-Scott

On Fri, 2015-01-30 at 19:08 +0700, Arseny Solokha wrote:

quoted

MMU_NO_CONTEXT is conditionally defined as 0 or (unsigned int)-1.

For nohash it is specifically -1.

quoted

 However, in __flush_tlb_page() a corresponding variable is only tested
for open coded 0, which can cause NULL pointer dereference if `mm'
argument was legitimately passed as such.

Bail out early in case the first argument is NULL, thus eliminate confusion
between different values of MMU_NO_CONTEXT and avoid disabling and then
re-enabling preemption unnecessarily.

How did you notice this?  Did you see an oops, or was it code
inspection?  I'm wondering what codepath gets here with mm == NULL.

Just a code inspection. It didn't seemed right at the first glance.

Arsény

-Scott

On Fri, 2015-01-30 at 19:08 +0700, Arseny Solokha wrote:

MMU_NO_CONTEXT is conditionally defined as 0 or (unsigned int)-1. However,
in __flush_tlb_page() a corresponding variable is only tested for open
coded 0, which can cause NULL pointer dereference if `mm' argument was
legitimately passed as such.

Bail out early in case the first argument is NULL, thus eliminate confusion
between different values of MMU_NO_CONTEXT and avoid disabling and then
re-enabling preemption unnecessarily.

So the comment above isn't quite right... we don't *test* it for open
coded 0, we test it for MMU_NO_CONTEXT, however we *set* it to 0 for
NULL mm.

This is actually correct... on all except 8xx :-) 0 *is* the PID of the
kernel context, and NULL mm usually means kernel context.

However, it's correct that this function will not deal properly with a
NULL mm for other reasons. It must only be called for user contexts.

Instead of just returning, I would WARN_ON, because if it's ever called
for a kernel page, then it will not do what's expected and that will
need fixing. Just a silent return isn't right.

This is different from returning on MMU_NO_CONTEXT, in this case, we
know there's no active TLB entries for the process, and thus nothing to
flush.

Cheers,
Ben.

Here we test mm, if we pass NULL, that means the kernel mm which has PID
0, which is not MMU_NO_CONTEXT

;
+	pid = mm->context.id;
 	if (unlikely(pid == MMU_NO_CONTEXT))
 		goto bail;
 	cpu_mask = mm_cpumask(mm);

On Fri, 2015-01-30 at 19:08 +0700, Arseny Solokha wrote:

quoted

MMU_NO_CONTEXT is conditionally defined as 0 or (unsigned int)-1. However,
in __flush_tlb_page() a corresponding variable is only tested for open
coded 0, which can cause NULL pointer dereference if `mm' argument was
legitimately passed as such.

Bail out early in case the first argument is NULL, thus eliminate confusion
between different values of MMU_NO_CONTEXT and avoid disabling and then
re-enabling preemption unnecessarily.

So the comment above isn't quite right... we don't *test* it for open
coded 0, we test it for MMU_NO_CONTEXT, however we *set* it to 0 for
NULL mm.

This is actually correct... on all except 8xx :-) 0 *is* the PID of the
kernel context, and NULL mm usually means kernel context.

However, it's correct that this function will not deal properly with a
NULL mm for other reasons. It must only be called for user contexts.

Instead of just returning, I would WARN_ON, because if it's ever called
for a kernel page, then it will not do what's expected and that will
need fixing. Just a silent return isn't right.

Does this also hold true for __local_flush_tlb_page()? It's called from
local_flush_tlb_page() as follows:

  __local_flush_tlb_page(vma ? vma->vm_mm : NULL, vmaddr,
  			 mmu_get_tsize(mmu_virtual_psize), 0);

However, if MMU_NO_CONTEXT is 0 and __local_flush_tlb_page() is called with mm
set to NULL, it's effectively a no-op. What else am I missing?

Thanks,

Arsény

This is different from returning on MMU_NO_CONTEXT, in this case, we
know there's no active TLB entries for the process, and thus nothing to
flush.

Cheers,
Ben.

quoted

Signed-off-by: Arseny Solokha <redacted>
---
 arch/powerpc/mm/tlb_nohash.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/mm/tlb_nohash.c b/arch/powerpc/mm/tlb_nohash.c
index f38ea4d..ab0616b 100644
--- a/arch/powerpc/mm/tlb_nohash.c
+++ b/arch/powerpc/mm/tlb_nohash.c

@@ -284,8 +284,11 @@ void __flush_tlb_page(struct mm_struct *mm, unsigned long vmaddr,
 	struct cpumask *cpu_mask;
 	unsigned int pid;
 
+	if (unlikely(!mm))
+		return;
+
 	preempt_disable();
-	pid = mm ? mm->context.id : 0

Here we test mm, if we pass NULL, that means the kernel mm which has PID
0, which is not MMU_NO_CONTEXT

quoted

;
+	pid = mm->context.id;
 	if (unlikely(pid == MMU_NO_CONTEXT))
 		goto bail;
 	cpu_mask = mm_cpumask(mm);

Function __flush_tlb_page() must only be called for user contexts, so
put in extra hardening to warn on calling it for kernel context.

Signed-off-by: Arseny Solokha <redacted>
---
 arch/powerpc/mm/tlb_nohash.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/powerpc/mm/tlb_nohash.c b/arch/powerpc/mm/tlb_nohash.c
index f38ea4d..cbd3d06 100644
--- a/arch/powerpc/mm/tlb_nohash.c
+++ b/arch/powerpc/mm/tlb_nohash.c

@@ -284,8 +284,15 @@ void __flush_tlb_page(struct mm_struct *mm, unsigned long vmaddr,
 	struct cpumask *cpu_mask;
 	unsigned int pid;
 
+	/*
+	 * This function as well as __local_flush_tlb_page() must only be called
+	 * for user contexts.
+	 */
+	if (unlikely(WARN_ON(!mm)))
+		return;
+
 	preempt_disable();
-	pid = mm ? mm->context.id : 0;
+	pid = mm->context.id;
 	if (unlikely(pid == MMU_NO_CONTEXT))
 		goto bail;
 	cpu_mask = mm_cpumask(mm);

-- 
2.2.2