[PATCH V2 0/4] arm64/mm: Clean ups for do_page_fault()

[PATCH V2 0/4] arm64/mm: Clean ups for do_page_fault()

[Anshuman Khandual]

This contains some clean ups for page fault handling in do_page_fault().
This has been boot tested on arm64 platform along with some stress test
but just build tested on others.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Andrey Konovalov <andreyknvl@google.com>
Cc: Christoph Hellwig <hch@infradead.org>

Changes in V2:

- s/is_write_abort()/is_el0_write_abort() with a comment per Mark Rutland
- s/is_write/is_el0_write/ & updated commit message as required
- s/argument/local variable/ per Christoph Hellwig
- Preserved VMA check order for VM_FAULT_BADMAP & VM_FAULT_BADACCESS per Mark
- Preserved all existing __do_page_fault() in code comments per Mark
- Dropped 'fixes' from the series's subject line per Will Deacon

V1: https://lkml.org/lkml/2019/5/29/431

Anshuman Khandual (4):
  arm64/mm: Drop mmap_sem before calling __do_kernel_fault()
  arm64/mm: Drop task_struct argument from __do_page_fault()
  arm64/mm: Consolidate page fault information capture
  arm64/mm: Drop local variable vm_fault_t from __do_page_fault()

 arch/arm64/mm/fault.c | 72 +++++++++++++++++++++++++--------------------------
 1 file changed, 36 insertions(+), 36 deletions(-)

-- 
2.7.4

[PATCH V2 1/4] arm64/mm: Drop mmap_sem before calling __do_kernel_fault()

[Anshuman Khandual]

There is an inconsistency between down_read_trylock() success and failure
paths while dealing with kernel access for non exception table areas where
it calls __do_kernel_fault(). In case of failure it just bails out without
holding mmap_sem but when it succeeds it does so while holding mmap_sem.
Fix this inconsistency by just dropping mmap_sem in success path as well.

__do_kernel_fault() calls die_kernel_fault() which then calls show_pte().
show_pte() in this path might become bit more unreliable without holding
mmap_sem. But there are already instances [1] in do_page_fault() where
die_kernel_fault() gets called without holding mmap_sem. show_pte() can
be made more robust independently but in a later patch.

[1] Conditional block for (is_ttbr0_addr && is_el1_permission_fault)

Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Andrey Konovalov <andreyknvl@google.com>
---
 arch/arm64/mm/fault.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index a30818e..dc1cf32 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -503,8 +503,10 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
 		 */
 		might_sleep();
 #ifdef CONFIG_DEBUG_VM
-		if (!user_mode(regs) && !search_exception_tables(regs->pc))
+		if (!user_mode(regs) && !search_exception_tables(regs->pc)) {
+			up_read(&mm->mmap_sem);
 			goto no_context;
+		}
 #endif
 	}
 
-- 
2.7.4

Re: [PATCH V2 1/4] arm64/mm: Drop mmap_sem before calling __do_kernel_fault()

[Catalin Marinas]

On Mon, Jun 03, 2019 at 12:11:22PM +0530, Anshuman Khandual wrote:
> There is an inconsistency between down_read_trylock() success and failure
> paths while dealing with kernel access for non exception table areas where
> it calls __do_kernel_fault(). In case of failure it just bails out without
> holding mmap_sem but when it succeeds it does so while holding mmap_sem.
> Fix this inconsistency by just dropping mmap_sem in success path as well.
> 
> __do_kernel_fault() calls die_kernel_fault() which then calls show_pte().
> show_pte() in this path might become bit more unreliable without holding
> mmap_sem. But there are already instances [1] in do_page_fault() where
> die_kernel_fault() gets called without holding mmap_sem. show_pte() can
> be made more robust independently but in a later patch.
> 
> [1] Conditional block for (is_ttbr0_addr && is_el1_permission_fault)
> 
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: James Morse <james.morse@arm.com>
> Cc: Andrey Konovalov <andreyknvl@google.com>

Queued for 5.3. Thanks.

-- 
Catalin

[PATCH V2 2/4] arm64/mm: Drop task_struct argument from __do_page_fault()

[Anshuman Khandual]

The task_struct argument is not getting used in __do_page_fault(). Hence
just drop it and use current or cuurent->mm instead where ever required.
This does not change any functionality.

Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Andrey Konovalov <andreyknvl@google.com>
---
 arch/arm64/mm/fault.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index dc1cf32..da02678 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -395,8 +395,7 @@ static void do_bad_area(unsigned long addr, unsigned int esr, struct pt_regs *re
 #define VM_FAULT_BADACCESS	0x020000
 
 static vm_fault_t __do_page_fault(struct mm_struct *mm, unsigned long addr,
-			   unsigned int mm_flags, unsigned long vm_flags,
-			   struct task_struct *tsk)
+			   unsigned int mm_flags, unsigned long vm_flags)
 {
 	struct vm_area_struct *vma;
 	vm_fault_t fault;
@@ -440,8 +439,7 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
 				   struct pt_regs *regs)
 {
 	const struct fault_info *inf;
-	struct task_struct *tsk;
-	struct mm_struct *mm;
+	struct mm_struct *mm = current->mm;
 	vm_fault_t fault, major = 0;
 	unsigned long vm_flags = VM_READ | VM_WRITE;
 	unsigned int mm_flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
@@ -449,9 +447,6 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
 	if (notify_page_fault(regs, esr))
 		return 0;
 
-	tsk = current;
-	mm  = tsk->mm;
-
 	/*
 	 * If we're in an interrupt or have no user context, we must not take
 	 * the fault.
@@ -510,7 +505,7 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
 #endif
 	}
 
-	fault = __do_page_fault(mm, addr, mm_flags, vm_flags, tsk);
+	fault = __do_page_fault(mm, addr, mm_flags, vm_flags);
 	major |= fault & VM_FAULT_MAJOR;
 
 	if (fault & VM_FAULT_RETRY) {
@@ -550,11 +545,11 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
 		 * that point.
 		 */
 		if (major) {
-			tsk->maj_flt++;
+			current->maj_flt++;
 			perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS_MAJ, 1, regs,
 				      addr);
 		} else {
-			tsk->min_flt++;
+			current->min_flt++;
 			perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS_MIN, 1, regs,
 				      addr);
 		}
-- 
2.7.4

Re: [PATCH V2 2/4] arm64/mm: Drop task_struct argument from __do_page_fault()

[Catalin Marinas]

On Mon, Jun 03, 2019 at 12:11:23PM +0530, Anshuman Khandual wrote:
> The task_struct argument is not getting used in __do_page_fault(). Hence
> just drop it and use current or cuurent->mm instead where ever required.
> This does not change any functionality.
> 
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: James Morse <james.morse@arm.com>
> Cc: Andrey Konovalov <andreyknvl@google.com>

Queued for 5.3. Thanks.

-- 
Catalin

[PATCH V2 3/4] arm64/mm: Consolidate page fault information capture

[Anshuman Khandual]

This consolidates page fault information capture and move them bit earlier.
While here it also adds an wrapper is_el0_write_abort(). It also saves some
cycles by replacing multiple user_mode() calls into a single one earlier
during the fault.

Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Andrey Konovalov <andreyknvl@google.com>
---
 arch/arm64/mm/fault.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index da02678..4bb65f3 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -435,6 +435,14 @@ static bool is_el0_instruction_abort(unsigned int esr)
 	return ESR_ELx_EC(esr) == ESR_ELx_EC_IABT_LOW;
 }
 
+/*
+ * This is applicable only for EL0 write aborts.
+ */
+static bool is_el0_write_abort(unsigned int esr)
+{
+	return (esr & ESR_ELx_WNR) && !(esr & ESR_ELx_CM);
+}
+
 static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
 				   struct pt_regs *regs)
 {
@@ -443,6 +451,9 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
 	vm_fault_t fault, major = 0;
 	unsigned long vm_flags = VM_READ | VM_WRITE;
 	unsigned int mm_flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
+	bool is_user = user_mode(regs);
+	bool is_el0_exec = is_el0_instruction_abort(esr);
+	bool is_el0_write = is_el0_write_abort(esr);
 
 	if (notify_page_fault(regs, esr))
 		return 0;
@@ -454,12 +465,12 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
 	if (faulthandler_disabled() || !mm)
 		goto no_context;
 
-	if (user_mode(regs))
+	if (is_user)
 		mm_flags |= FAULT_FLAG_USER;
 
-	if (is_el0_instruction_abort(esr)) {
+	if (is_el0_exec) {
 		vm_flags = VM_EXEC;
-	} else if ((esr & ESR_ELx_WNR) && !(esr & ESR_ELx_CM)) {
+	} else if (is_el0_write) {
 		vm_flags = VM_WRITE;
 		mm_flags |= FAULT_FLAG_WRITE;
 	}
@@ -487,7 +498,7 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
 	 * we can bug out early if this is from code which shouldn't.
 	 */
 	if (!down_read_trylock(&mm->mmap_sem)) {
-		if (!user_mode(regs) && !search_exception_tables(regs->pc))
+		if (!is_user && !search_exception_tables(regs->pc))
 			goto no_context;
 retry:
 		down_read(&mm->mmap_sem);
@@ -498,7 +509,7 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
 		 */
 		might_sleep();
 #ifdef CONFIG_DEBUG_VM
-		if (!user_mode(regs) && !search_exception_tables(regs->pc)) {
+		if (!is_user && !search_exception_tables(regs->pc)) {
 			up_read(&mm->mmap_sem);
 			goto no_context;
 		}
@@ -516,7 +527,7 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
 		 * in __lock_page_or_retry in mm/filemap.c.
 		 */
 		if (fatal_signal_pending(current)) {
-			if (!user_mode(regs))
+			if (!is_user)
 				goto no_context;
 			return 0;
 		}
@@ -561,7 +572,7 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
 	 * If we are in kernel mode at this point, we have no context to
 	 * handle this fault with.
 	 */
-	if (!user_mode(regs))
+	if (!is_user)
 		goto no_context;
 
 	if (fault & VM_FAULT_OOM) {
-- 
2.7.4

Re: [PATCH V2 3/4] arm64/mm: Consolidate page fault information capture

[Catalin Marinas]

On Mon, Jun 03, 2019 at 12:11:24PM +0530, Anshuman Khandual wrote:
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index da02678..4bb65f3 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -435,6 +435,14 @@ static bool is_el0_instruction_abort(unsigned int esr)
>  	return ESR_ELx_EC(esr) == ESR_ELx_EC_IABT_LOW;
>  }
>  
> +/*
> + * This is applicable only for EL0 write aborts.
> + */
> +static bool is_el0_write_abort(unsigned int esr)
> +{
> +	return (esr & ESR_ELx_WNR) && !(esr & ESR_ELx_CM);
> +}

What makes this EL0 only?

> +
>  static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
>  				   struct pt_regs *regs)
>  {
> @@ -443,6 +451,9 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
>  	vm_fault_t fault, major = 0;
>  	unsigned long vm_flags = VM_READ | VM_WRITE;
>  	unsigned int mm_flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
> +	bool is_user = user_mode(regs);
> +	bool is_el0_exec = is_el0_instruction_abort(esr);
> +	bool is_el0_write = is_el0_write_abort(esr);
>  
>  	if (notify_page_fault(regs, esr))
>  		return 0;
> @@ -454,12 +465,12 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
>  	if (faulthandler_disabled() || !mm)
>  		goto no_context;
>  
> -	if (user_mode(regs))
> +	if (is_user)
>  		mm_flags |= FAULT_FLAG_USER;
>  
> -	if (is_el0_instruction_abort(esr)) {
> +	if (is_el0_exec) {
>  		vm_flags = VM_EXEC;
> -	} else if ((esr & ESR_ELx_WNR) && !(esr & ESR_ELx_CM)) {
> +	} else if (is_el0_write) {
>  		vm_flags = VM_WRITE;
>  		mm_flags |= FAULT_FLAG_WRITE;
>  	}

This can be triggered by an EL1 write to a user mapping, so is_el0_write
is misleading.

-- 
Catalin

Re: [PATCH V2 3/4] arm64/mm: Consolidate page fault information capture

[Mark Rutland]

On Tue, Jun 04, 2019 at 03:42:09PM +0100, Catalin Marinas wrote:
> On Mon, Jun 03, 2019 at 12:11:24PM +0530, Anshuman Khandual wrote:
> > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> > index da02678..4bb65f3 100644
> > --- a/arch/arm64/mm/fault.c
> > +++ b/arch/arm64/mm/fault.c
> > @@ -435,6 +435,14 @@ static bool is_el0_instruction_abort(unsigned int esr)
> >  	return ESR_ELx_EC(esr) == ESR_ELx_EC_IABT_LOW;
> >  }
> >  
> > +/*
> > + * This is applicable only for EL0 write aborts.
> > + */
> > +static bool is_el0_write_abort(unsigned int esr)
> > +{
> > +	return (esr & ESR_ELx_WNR) && !(esr & ESR_ELx_CM);
> > +}
> 
> What makes this EL0 only?

It returns false for EL1 faults caused by DC IVAC, where write
permission is required. EL0 can only issue maintenance that requires
read permission.

For whatever reason, the architecture says that WnR is always 1b1, even
if read permission was sufficient.

How about:

/*
 * Note: not valid for EL1 DC IVAC, but we never use that such that it
 * should fault.
 */
static bool is_write_abort(unsigned int esr)
{
	return (esr & ESR_ELx_WNR) && !(esr & ESR_ELx_CM);
}

... which would also address your concern about EL1 writes to a user
mapping.

Thanks,
Mark.

Re: [PATCH V2 3/4] arm64/mm: Consolidate page fault information capture

[Catalin Marinas]

On Thu, Jun 06, 2019 at 10:38:11AM +0100, Mark Rutland wrote:
> On Tue, Jun 04, 2019 at 03:42:09PM +0100, Catalin Marinas wrote:
> > On Mon, Jun 03, 2019 at 12:11:24PM +0530, Anshuman Khandual wrote:
> > > diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> > > index da02678..4bb65f3 100644
> > > --- a/arch/arm64/mm/fault.c
> > > +++ b/arch/arm64/mm/fault.c
> > > @@ -435,6 +435,14 @@ static bool is_el0_instruction_abort(unsigned int esr)
> > >  	return ESR_ELx_EC(esr) == ESR_ELx_EC_IABT_LOW;
> > >  }
> > >  
> > > +/*
> > > + * This is applicable only for EL0 write aborts.
> > > + */
> > > +static bool is_el0_write_abort(unsigned int esr)
> > > +{
> > > +	return (esr & ESR_ELx_WNR) && !(esr & ESR_ELx_CM);
> > > +}
> > 
> > What makes this EL0 only?
> 
> It returns false for EL1 faults caused by DC IVAC, where write
> permission is required. EL0 can only issue maintenance that requires
> read permission.
> 
> For whatever reason, the architecture says that WnR is always 1b1, even
> if read permission was sufficient.
> 
> How about:
> 
> /*
>  * Note: not valid for EL1 DC IVAC, but we never use that such that it
>  * should fault.
>  */

For completeness, I'd add "... should fault. EL0 cannot issue DC IVAC
(undef)." or something like that.

Looks fine otherwise.

-- 
Catalin

[PATCH V2 4/4] arm64/mm: Drop local variable vm_fault_t from __do_page_fault()

[Anshuman Khandual]

__do_page_fault() is over complicated with multiple goto statements. This
cleans up the code flow and while there drops local variable vm_fault_t.

Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: James Morse <james.morse@arm.com>
Cc: Andrey Konovalov <andreyknvl@google.com>
Cc: Christoph Hellwig <hch@infradead.org>
---
 arch/arm64/mm/fault.c | 30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 4bb65f3..41fa905 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -397,37 +397,29 @@ static void do_bad_area(unsigned long addr, unsigned int esr, struct pt_regs *re
 static vm_fault_t __do_page_fault(struct mm_struct *mm, unsigned long addr,
 			   unsigned int mm_flags, unsigned long vm_flags)
 {
-	struct vm_area_struct *vma;
-	vm_fault_t fault;
+	struct vm_area_struct *vma = find_vma(mm, addr);
 
-	vma = find_vma(mm, addr);
-	fault = VM_FAULT_BADMAP;
 	if (unlikely(!vma))
-		goto out;
-	if (unlikely(vma->vm_start > addr))
-		goto check_stack;
+		return VM_FAULT_BADMAP;
 
 	/*
 	 * Ok, we have a good vm_area for this memory access, so we can handle
 	 * it.
 	 */
-good_area:
+	if (unlikely(vma->vm_start > addr)) {
+		if (!(vma->vm_flags & VM_GROWSDOWN))
+			return VM_FAULT_BADMAP;
+		if (expand_stack(vma, addr))
+			return VM_FAULT_BADMAP;
+	}
+
 	/*
 	 * Check that the permissions on the VMA allow for the fault which
 	 * occurred.
 	 */
-	if (!(vma->vm_flags & vm_flags)) {
-		fault = VM_FAULT_BADACCESS;
-		goto out;
-	}
-
+	if (!(vma->vm_flags & vm_flags))
+		return VM_FAULT_BADACCESS;
 	return handle_mm_fault(vma, addr & PAGE_MASK, mm_flags);
-
-check_stack:
-	if (vma->vm_flags & VM_GROWSDOWN && !expand_stack(vma, addr))
-		goto good_area;
-out:
-	return fault;
 }
 
 static bool is_el0_instruction_abort(unsigned int esr)
-- 
2.7.4

Re: [PATCH V2 4/4] arm64/mm: Drop local variable vm_fault_t from __do_page_fault()

[Catalin Marinas]

On Mon, Jun 03, 2019 at 12:11:25PM +0530, Anshuman Khandual wrote:
> __do_page_fault() is over complicated with multiple goto statements. This
> cleans up the code flow and while there drops local variable vm_fault_t.

I'd change the subject as well here to something like refactor or
simplify __do_page_fault().

> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index 4bb65f3..41fa905 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -397,37 +397,29 @@ static void do_bad_area(unsigned long addr, unsigned int esr, struct pt_regs *re
>  static vm_fault_t __do_page_fault(struct mm_struct *mm, unsigned long addr,
>  			   unsigned int mm_flags, unsigned long vm_flags)
>  {
> -	struct vm_area_struct *vma;
> -	vm_fault_t fault;
> +	struct vm_area_struct *vma = find_vma(mm, addr);
>  
> -	vma = find_vma(mm, addr);
> -	fault = VM_FAULT_BADMAP;
>  	if (unlikely(!vma))
> -		goto out;
> -	if (unlikely(vma->vm_start > addr))
> -		goto check_stack;
> +		return VM_FAULT_BADMAP;
>  
>  	/*
>  	 * Ok, we have a good vm_area for this memory access, so we can handle
>  	 * it.
>  	 */
> -good_area:
> +	if (unlikely(vma->vm_start > addr)) {
> +		if (!(vma->vm_flags & VM_GROWSDOWN))
> +			return VM_FAULT_BADMAP;
> +		if (expand_stack(vma, addr))
> +			return VM_FAULT_BADMAP;
> +	}

You could have a single return here:

	if (unlikely(vma->vm_start > addr) &&
	    (!(vma->vm_flags & VM_GROWSDOWN) || expand_stack(vma, addr)))
		return VM_FAULT_BADMAP;

Not sure it's any clearer though.

-- 
Catalin

Re: [PATCH V2 4/4] arm64/mm: Drop local variable vm_fault_t from __do_page_fault()

[Anshuman Khandual]

On 06/04/2019 08:26 PM, Catalin Marinas wrote:
> On Mon, Jun 03, 2019 at 12:11:25PM +0530, Anshuman Khandual wrote:
>> __do_page_fault() is over complicated with multiple goto statements. This
>> cleans up the code flow and while there drops local variable vm_fault_t.
> 
> I'd change the subject as well here to something like refactor or
> simplify __do_page_fault().

Sure.

> 
>> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
>> index 4bb65f3..41fa905 100644
>> --- a/arch/arm64/mm/fault.c
>> +++ b/arch/arm64/mm/fault.c
>> @@ -397,37 +397,29 @@ static void do_bad_area(unsigned long addr, unsigned int esr, struct pt_regs *re
>>  static vm_fault_t __do_page_fault(struct mm_struct *mm, unsigned long addr,
>>  			   unsigned int mm_flags, unsigned long vm_flags)
>>  {
>> -	struct vm_area_struct *vma;
>> -	vm_fault_t fault;
>> +	struct vm_area_struct *vma = find_vma(mm, addr);
>>  
>> -	vma = find_vma(mm, addr);
>> -	fault = VM_FAULT_BADMAP;
>>  	if (unlikely(!vma))
>> -		goto out;
>> -	if (unlikely(vma->vm_start > addr))
>> -		goto check_stack;
>> +		return VM_FAULT_BADMAP;
>>  
>>  	/*
>>  	 * Ok, we have a good vm_area for this memory access, so we can handle
>>  	 * it.
>>  	 */
>> -good_area:
>> +	if (unlikely(vma->vm_start > addr)) {
>> +		if (!(vma->vm_flags & VM_GROWSDOWN))
>> +			return VM_FAULT_BADMAP;
>> +		if (expand_stack(vma, addr))
>> +			return VM_FAULT_BADMAP;
>> +	}
> 
> You could have a single return here:
> 
> 	if (unlikely(vma->vm_start > addr) &&
> 	    (!(vma->vm_flags & VM_GROWSDOWN) || expand_stack(vma, addr)))
> 		return VM_FAULT_BADMAP;
> 
> Not sure it's any clearer though.
> 

TBH the proposed one seems clearer as it separates effect (vma->vm_start > addr)
from required permission check (vma->vm_flags & VM_GROWSDOWN) and required action
(expand_stack(vma, addr)). But I am happy to change as you have mentioned if that
is preferred.

Re: [PATCH V2 4/4] arm64/mm: Drop local variable vm_fault_t from __do_page_fault()

[Catalin Marinas]

On Thu, Jun 06, 2019 at 10:24:01AM +0530, Anshuman Khandual wrote:
> On 06/04/2019 08:26 PM, Catalin Marinas wrote:
> > On Mon, Jun 03, 2019 at 12:11:25PM +0530, Anshuman Khandual wrote:
> >> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> >> index 4bb65f3..41fa905 100644
> >> --- a/arch/arm64/mm/fault.c
> >> +++ b/arch/arm64/mm/fault.c
> >> @@ -397,37 +397,29 @@ static void do_bad_area(unsigned long addr, unsigned int esr, struct pt_regs *re
> >>  static vm_fault_t __do_page_fault(struct mm_struct *mm, unsigned long addr,
> >>  			   unsigned int mm_flags, unsigned long vm_flags)
> >>  {
> >> -	struct vm_area_struct *vma;
> >> -	vm_fault_t fault;
> >> +	struct vm_area_struct *vma = find_vma(mm, addr);
> >>  
> >> -	vma = find_vma(mm, addr);
> >> -	fault = VM_FAULT_BADMAP;
> >>  	if (unlikely(!vma))
> >> -		goto out;
> >> -	if (unlikely(vma->vm_start > addr))
> >> -		goto check_stack;
> >> +		return VM_FAULT_BADMAP;
> >>  
> >>  	/*
> >>  	 * Ok, we have a good vm_area for this memory access, so we can handle
> >>  	 * it.
> >>  	 */
> >> -good_area:
> >> +	if (unlikely(vma->vm_start > addr)) {
> >> +		if (!(vma->vm_flags & VM_GROWSDOWN))
> >> +			return VM_FAULT_BADMAP;
> >> +		if (expand_stack(vma, addr))
> >> +			return VM_FAULT_BADMAP;
> >> +	}
> > 
> > You could have a single return here:
> > 
> > 	if (unlikely(vma->vm_start > addr) &&
> > 	    (!(vma->vm_flags & VM_GROWSDOWN) || expand_stack(vma, addr)))
> > 		return VM_FAULT_BADMAP;
> > 
> > Not sure it's any clearer though.
> 
> TBH the proposed one seems clearer as it separates effect (vma->vm_start > addr)
> from required permission check (vma->vm_flags & VM_GROWSDOWN) and required action
> (expand_stack(vma, addr)). But I am happy to change as you have mentioned if that
> is preferred.

Not bothered really. You can leave them as in your proposal (I was just
seeing the VM_GROWSDOWN check tightly coupled with the expand_stack(),
it's fine either way).

-- 
Catalin

Re: [PATCH V2 4/4] arm64/mm: Drop local variable vm_fault_t from __do_page_fault()

[Mark Rutland]

On Thu, Jun 06, 2019 at 12:27:40PM +0100, Catalin Marinas wrote:
> On Thu, Jun 06, 2019 at 10:24:01AM +0530, Anshuman Khandual wrote:
> > On 06/04/2019 08:26 PM, Catalin Marinas wrote:
> > > On Mon, Jun 03, 2019 at 12:11:25PM +0530, Anshuman Khandual wrote:
> > >> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> > >> index 4bb65f3..41fa905 100644
> > >> --- a/arch/arm64/mm/fault.c
> > >> +++ b/arch/arm64/mm/fault.c
> > >> @@ -397,37 +397,29 @@ static void do_bad_area(unsigned long addr, unsigned int esr, struct pt_regs *re
> > >>  static vm_fault_t __do_page_fault(struct mm_struct *mm, unsigned long addr,
> > >>  			   unsigned int mm_flags, unsigned long vm_flags)
> > >>  {
> > >> -	struct vm_area_struct *vma;
> > >> -	vm_fault_t fault;
> > >> +	struct vm_area_struct *vma = find_vma(mm, addr);
> > >>  
> > >> -	vma = find_vma(mm, addr);
> > >> -	fault = VM_FAULT_BADMAP;
> > >>  	if (unlikely(!vma))
> > >> -		goto out;
> > >> -	if (unlikely(vma->vm_start > addr))
> > >> -		goto check_stack;
> > >> +		return VM_FAULT_BADMAP;
> > >>  
> > >>  	/*
> > >>  	 * Ok, we have a good vm_area for this memory access, so we can handle
> > >>  	 * it.
> > >>  	 */
> > >> -good_area:
> > >> +	if (unlikely(vma->vm_start > addr)) {
> > >> +		if (!(vma->vm_flags & VM_GROWSDOWN))
> > >> +			return VM_FAULT_BADMAP;
> > >> +		if (expand_stack(vma, addr))
> > >> +			return VM_FAULT_BADMAP;
> > >> +	}
> > > 
> > > You could have a single return here:
> > > 
> > > 	if (unlikely(vma->vm_start > addr) &&
> > > 	    (!(vma->vm_flags & VM_GROWSDOWN) || expand_stack(vma, addr)))
> > > 		return VM_FAULT_BADMAP;
> > > 
> > > Not sure it's any clearer though.
> > 
> > TBH the proposed one seems clearer as it separates effect (vma->vm_start > addr)
> > from required permission check (vma->vm_flags & VM_GROWSDOWN) and required action
> > (expand_stack(vma, addr)). But I am happy to change as you have mentioned if that
> > is preferred.
> 
> Not bothered really. You can leave them as in your proposal (I was just
> seeing the VM_GROWSDOWN check tightly coupled with the expand_stack(),
> it's fine either way).

Personally, I find it clearer as separate statements, so I'd suggest
keeping it as per Anshuman's proposal.

Thanks,
Mark.