* [PATCH V2 1/4] arm64/mm: Drop mmap_sem before calling __do_kernel_fault()
2019-06-03 6:41 [PATCH V2 0/4] arm64/mm: Clean ups for do_page_fault() Anshuman Khandual
@ 2019-06-03 6:41 ` Anshuman Khandual
2019-06-04 14:44 ` Catalin Marinas
2019-06-03 6:41 ` [PATCH V2 2/4] arm64/mm: Drop task_struct argument from __do_page_fault() Anshuman Khandual
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: Anshuman Khandual @ 2019-06-03 6:41 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel
Cc: Anshuman Khandual, Catalin Marinas, Will Deacon, Mark Rutland,
James Morse, Andrey Konovalov
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH V2 1/4] arm64/mm: Drop mmap_sem before calling __do_kernel_fault()
2019-06-03 6:41 ` [PATCH V2 1/4] arm64/mm: Drop mmap_sem before calling __do_kernel_fault() Anshuman Khandual
@ 2019-06-04 14:44 ` Catalin Marinas
0 siblings, 0 replies; 14+ messages in thread
From: Catalin Marinas @ 2019-06-04 14:44 UTC (permalink / raw)
To: Anshuman Khandual
Cc: linux-kernel, linux-arm-kernel, Will Deacon, Mark Rutland,
James Morse, Andrey Konovalov
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH V2 2/4] arm64/mm: Drop task_struct argument from __do_page_fault()
2019-06-03 6:41 [PATCH V2 0/4] arm64/mm: Clean ups for do_page_fault() Anshuman Khandual
2019-06-03 6:41 ` [PATCH V2 1/4] arm64/mm: Drop mmap_sem before calling __do_kernel_fault() Anshuman Khandual
@ 2019-06-03 6:41 ` Anshuman Khandual
2019-06-04 14:45 ` Catalin Marinas
2019-06-03 6:41 ` [PATCH V2 3/4] arm64/mm: Consolidate page fault information capture Anshuman Khandual
2019-06-03 6:41 ` [PATCH V2 4/4] arm64/mm: Drop local variable vm_fault_t from __do_page_fault() Anshuman Khandual
3 siblings, 1 reply; 14+ messages in thread
From: Anshuman Khandual @ 2019-06-03 6:41 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel
Cc: Anshuman Khandual, Catalin Marinas, Will Deacon, Mark Rutland,
James Morse, Andrey Konovalov
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH V2 2/4] arm64/mm: Drop task_struct argument from __do_page_fault()
2019-06-03 6:41 ` [PATCH V2 2/4] arm64/mm: Drop task_struct argument from __do_page_fault() Anshuman Khandual
@ 2019-06-04 14:45 ` Catalin Marinas
0 siblings, 0 replies; 14+ messages in thread
From: Catalin Marinas @ 2019-06-04 14:45 UTC (permalink / raw)
To: Anshuman Khandual
Cc: linux-kernel, linux-arm-kernel, Will Deacon, Mark Rutland,
James Morse, Andrey Konovalov
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH V2 3/4] arm64/mm: Consolidate page fault information capture
2019-06-03 6:41 [PATCH V2 0/4] arm64/mm: Clean ups for do_page_fault() Anshuman Khandual
2019-06-03 6:41 ` [PATCH V2 1/4] arm64/mm: Drop mmap_sem before calling __do_kernel_fault() Anshuman Khandual
2019-06-03 6:41 ` [PATCH V2 2/4] arm64/mm: Drop task_struct argument from __do_page_fault() Anshuman Khandual
@ 2019-06-03 6:41 ` Anshuman Khandual
2019-06-04 14:42 ` Catalin Marinas
2019-06-03 6:41 ` [PATCH V2 4/4] arm64/mm: Drop local variable vm_fault_t from __do_page_fault() Anshuman Khandual
3 siblings, 1 reply; 14+ messages in thread
From: Anshuman Khandual @ 2019-06-03 6:41 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel
Cc: Anshuman Khandual, Catalin Marinas, Will Deacon, Mark Rutland,
James Morse, Andrey Konovalov
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH V2 3/4] arm64/mm: Consolidate page fault information capture
2019-06-03 6:41 ` [PATCH V2 3/4] arm64/mm: Consolidate page fault information capture Anshuman Khandual
@ 2019-06-04 14:42 ` Catalin Marinas
2019-06-06 9:38 ` Mark Rutland
0 siblings, 1 reply; 14+ messages in thread
From: Catalin Marinas @ 2019-06-04 14:42 UTC (permalink / raw)
To: Anshuman Khandual
Cc: linux-kernel, linux-arm-kernel, Will Deacon, Mark Rutland,
James Morse, Andrey Konovalov
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V2 3/4] arm64/mm: Consolidate page fault information capture
2019-06-04 14:42 ` Catalin Marinas
@ 2019-06-06 9:38 ` Mark Rutland
2019-06-06 11:23 ` Catalin Marinas
0 siblings, 1 reply; 14+ messages in thread
From: Mark Rutland @ 2019-06-06 9:38 UTC (permalink / raw)
To: Catalin Marinas
Cc: Anshuman Khandual, linux-kernel, linux-arm-kernel, Will Deacon,
James Morse, Andrey Konovalov
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V2 3/4] arm64/mm: Consolidate page fault information capture
2019-06-06 9:38 ` Mark Rutland
@ 2019-06-06 11:23 ` Catalin Marinas
0 siblings, 0 replies; 14+ messages in thread
From: Catalin Marinas @ 2019-06-06 11:23 UTC (permalink / raw)
To: Mark Rutland
Cc: Anshuman Khandual, linux-kernel, linux-arm-kernel, Will Deacon,
James Morse, Andrey Konovalov
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH V2 4/4] arm64/mm: Drop local variable vm_fault_t from __do_page_fault()
2019-06-03 6:41 [PATCH V2 0/4] arm64/mm: Clean ups for do_page_fault() Anshuman Khandual
` (2 preceding siblings ...)
2019-06-03 6:41 ` [PATCH V2 3/4] arm64/mm: Consolidate page fault information capture Anshuman Khandual
@ 2019-06-03 6:41 ` Anshuman Khandual
2019-06-04 14:56 ` Catalin Marinas
3 siblings, 1 reply; 14+ messages in thread
From: Anshuman Khandual @ 2019-06-03 6:41 UTC (permalink / raw)
To: linux-kernel, linux-arm-kernel
Cc: Anshuman Khandual, Catalin Marinas, Will Deacon, Mark Rutland,
James Morse, Andrey Konovalov, Christoph Hellwig
__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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH V2 4/4] arm64/mm: Drop local variable vm_fault_t from __do_page_fault()
2019-06-03 6:41 ` [PATCH V2 4/4] arm64/mm: Drop local variable vm_fault_t from __do_page_fault() Anshuman Khandual
@ 2019-06-04 14:56 ` Catalin Marinas
2019-06-06 4:54 ` Anshuman Khandual
0 siblings, 1 reply; 14+ messages in thread
From: Catalin Marinas @ 2019-06-04 14:56 UTC (permalink / raw)
To: Anshuman Khandual
Cc: linux-kernel, linux-arm-kernel, Will Deacon, Mark Rutland,
James Morse, Andrey Konovalov, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V2 4/4] arm64/mm: Drop local variable vm_fault_t from __do_page_fault()
2019-06-04 14:56 ` Catalin Marinas
@ 2019-06-06 4:54 ` Anshuman Khandual
2019-06-06 11:27 ` Catalin Marinas
0 siblings, 1 reply; 14+ messages in thread
From: Anshuman Khandual @ 2019-06-06 4:54 UTC (permalink / raw)
To: Catalin Marinas
Cc: linux-kernel, linux-arm-kernel, Will Deacon, Mark Rutland,
James Morse, Andrey Konovalov, Christoph Hellwig
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V2 4/4] arm64/mm: Drop local variable vm_fault_t from __do_page_fault()
2019-06-06 4:54 ` Anshuman Khandual
@ 2019-06-06 11:27 ` Catalin Marinas
2019-06-06 11:31 ` Mark Rutland
0 siblings, 1 reply; 14+ messages in thread
From: Catalin Marinas @ 2019-06-06 11:27 UTC (permalink / raw)
To: Anshuman Khandual
Cc: linux-kernel, linux-arm-kernel, Will Deacon, Mark Rutland,
James Morse, Andrey Konovalov, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V2 4/4] arm64/mm: Drop local variable vm_fault_t from __do_page_fault()
2019-06-06 11:27 ` Catalin Marinas
@ 2019-06-06 11:31 ` Mark Rutland
0 siblings, 0 replies; 14+ messages in thread
From: Mark Rutland @ 2019-06-06 11:31 UTC (permalink / raw)
To: Catalin Marinas
Cc: Anshuman Khandual, linux-kernel, linux-arm-kernel, Will Deacon,
James Morse, Andrey Konovalov, Christoph Hellwig
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.
^ permalink raw reply [flat|nested] 14+ messages in thread