Re: [PATCH V2 4/4] arm64/mm: Drop local variable vm_fault_t from __do_page_fault()
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.
Re: [PATCH V2 4/4] arm64/mm: Drop local variable vm_fault_t from __do_page_fault()
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()
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()
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
[PATCH V2 4/4] arm64/mm: Drop local variable vm_fault_t from __do_page_fault()
__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
Cc: Catalin Marinas
Cc: Will Deacon
Cc: Mark Rutland
Cc: James Morse
Cc: Andrey Konovalov
Cc: Christoph Hellwig
---
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
