Re: [RFC PATCH 4/5] powerpc/mm: Remove custom stack expansion checking

Excerpts from Christophe Leroy's message of July 6, 2020 3:49 am:

Le 03/07/2020 à 16:13, Michael Ellerman a écrit :

quoted

We have powerpc specific logic in our page fault handling to decide if
an access to an unmapped address below the stack pointer should expand
the stack VMA.

The logic aims to prevent userspace from doing bad accesses below the
stack pointer. However as long as the stack is < 1MB in size, we allow
all accesses without further checks. Adding some debug I see that I
can do a full kernel build and LTP run, and not a single process has
used more than 1MB of stack. So for the majority of processes the
logic never even fires.

We also recently found a nasty bug in this code which could cause
userspace programs to be killed during signal delivery. It went
unnoticed presumably because most processes use < 1MB of stack.

The generic mm code has also grown support for stack guard pages since
this code was originally written, so the most heinous case of the
stack expanding into other mappings is now handled for us.

Finally although some other arches have special logic in this path,
from what I can tell none of x86, arm64, arm and s390 impose any extra
checks other than those in expand_stack().

So drop our complicated logic and like other architectures just let
the stack expand as long as its within the rlimit.

I agree that's probably not worth a so complicated logic that is nowhere 
documented.

Agreed.

quoted

@@ -569,30 +488,15 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,
  	vma = find_vma(mm, address);
  	if (unlikely(!vma))
  		return bad_area(regs, address);
-	if (likely(vma->vm_start <= address))
-		goto good_area;
-	if (unlikely(!(vma->vm_flags & VM_GROWSDOWN)))
-		return bad_area(regs, address);
  
-	/* The stack is being expanded, check if it's valid */
-	if (unlikely(bad_stack_expansion(regs, address, vma, flags,
-					 &must_retry))) {
-		if (!must_retry)
+	if (unlikely(vma->vm_start > address)) {
+		if (unlikely(!(vma->vm_flags & VM_GROWSDOWN)))

We are already in an unlikely() branch, I don't think it is worth having 
a second level of unlikely(), better let gcc decide what's most efficient.

I'm not sure being nested matters. It does in terms of how the code is 
generated and how much it might acutally matter, but if we say we 
optimise the expand stack case rather than the segfault case, then 
unlikely is fine here. I find it can be a readability aid as well.

Thanks,
Nick