Message ID | 20210529034138.83384-2-wangkefeng.wang@huawei.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | ARM: fix PXN process with LPAE feature | expand |
On Sat, May 29, 2021 at 11:41:37AM +0800, Kefeng Wang wrote: > 1. cleanup access_error(), make vma flags set and check into > __do_page_fault() and do_page_fault() directly. > > 2. drop fsr and task argument, instead, using vm_flags in > __do_page_fault(). > > 3. cleans up the multiple goto statements in __do_page_fault(). > > 4. use current->mm directly in do_page_fault(). > > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> This patch is a really good example of something that is very difficult to review and see that there are no unintended changes. Many people have complained about my patches, where I create a series of many patches where each patch does exactly _one_ simple transformation to the code. This is a good example _why_ I do that - a step by step single transformation approach is way easier to review. Sorry, but I'm not able to sensibly review this patch, and therefore I won't apply it. Please split it into smaller changes.
On 2021/6/1 22:31, Russell King (Oracle) wrote: > On Sat, May 29, 2021 at 11:41:37AM +0800, Kefeng Wang wrote: >> 1. cleanup access_error(), make vma flags set and check into >> __do_page_fault() and do_page_fault() directly. >> >> 2. drop fsr and task argument, instead, using vm_flags in >> __do_page_fault(). >> >> 3. cleans up the multiple goto statements in __do_page_fault(). >> >> 4. use current->mm directly in do_page_fault(). >> >> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > This patch is a really good example of something that is very difficult > to review and see that there are no unintended changes. > > Many people have complained about my patches, where I create a series of > many patches where each patch does exactly _one_ simple transformation to > the code. This is a good example _why_ I do that - a step by step single > transformation approach is way easier to review. > > Sorry, but I'm not able to sensibly review this patch, and therefore > I won't apply it. Please split it into smaller changes. Ok, will split it and send v2, thanks. >
On Tue, 1 Jun 2021 at 16:32, Russell King (Oracle) <linux@armlinux.org.uk> wrote: > > On Sat, May 29, 2021 at 11:41:37AM +0800, Kefeng Wang wrote: > > 1. cleanup access_error(), make vma flags set and check into > > __do_page_fault() and do_page_fault() directly. > > > > 2. drop fsr and task argument, instead, using vm_flags in > > __do_page_fault(). > > > > 3. cleans up the multiple goto statements in __do_page_fault(). > > > > 4. use current->mm directly in do_page_fault(). > > > > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > > This patch is a really good example of something that is very difficult > to review and see that there are no unintended changes. > > Many people have complained about my patches, where I create a series of > many patches where each patch does exactly _one_ simple transformation to > the code. This is a good example _why_ I do that - a step by step single > transformation approach is way easier to review. > > Sorry, but I'm not able to sensibly review this patch, and therefore > I won't apply it. Please split it into smaller changes. > Agreed. If your commit message contains an enumeration of things the patch does, it is a very strong hint that each of those things needs to be a separate patch if at all possible.
On Fri, 2 Jun 2023 at 11:49, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Tue, 1 Jun 2021 at 16:32, Russell King (Oracle) > <linux@armlinux.org.uk> wrote: > > > > On Sat, May 29, 2021 at 11:41:37AM +0800, Kefeng Wang wrote: > > > 1. cleanup access_error(), make vma flags set and check into > > > __do_page_fault() and do_page_fault() directly. > > > > > > 2. drop fsr and task argument, instead, using vm_flags in > > > __do_page_fault(). > > > > > > 3. cleans up the multiple goto statements in __do_page_fault(). > > > > > > 4. use current->mm directly in do_page_fault(). > > > > > > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > > > > This patch is a really good example of something that is very difficult > > to review and see that there are no unintended changes. > > > > Many people have complained about my patches, where I create a series of > > many patches where each patch does exactly _one_ simple transformation to > > the code. This is a good example _why_ I do that - a step by step single > > transformation approach is way easier to review. > > > > Sorry, but I'm not able to sensibly review this patch, and therefore > > I won't apply it. Please split it into smaller changes. > > > > Agreed. If your commit message contains an enumeration of things the > patch does, it is a very strong hint that each of those things needs > to be a separate patch if at all possible. Also, apologies for digging up this 2 year old thread :-) I did so unintentionally. (Somehow, it turned up as new/unread in my LAKML folder)
On 2023/6/2 17:51, Ard Biesheuvel wrote: > On Fri, 2 Jun 2023 at 11:49, Ard Biesheuvel <ardb@kernel.org> wrote: >> >> On Tue, 1 Jun 2021 at 16:32, Russell King (Oracle) >> <linux@armlinux.org.uk> wrote: >>> >>> On Sat, May 29, 2021 at 11:41:37AM +0800, Kefeng Wang wrote: >>>> 1. cleanup access_error(), make vma flags set and check into >>>> __do_page_fault() and do_page_fault() directly. >>>> >>>> 2. drop fsr and task argument, instead, using vm_flags in >>>> __do_page_fault(). >>>> >>>> 3. cleans up the multiple goto statements in __do_page_fault(). >>>> >>>> 4. use current->mm directly in do_page_fault(). >>>> >>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> >>> >>> This patch is a really good example of something that is very difficult >>> to review and see that there are no unintended changes. >>> >>> Many people have complained about my patches, where I create a series of >>> many patches where each patch does exactly _one_ simple transformation to >>> the code. This is a good example _why_ I do that - a step by step single >>> transformation approach is way easier to review. >>> >>> Sorry, but I'm not able to sensibly review this patch, and therefore >>> I won't apply it. Please split it into smaller changes. >>> >> >> Agreed. If your commit message contains an enumeration of things the >> patch does, it is a very strong hint that each of those things needs >> to be a separate patch if at all possible. Yes, already split it and the new version is merged, > > Also, apologies for digging up this 2 year old thread :-) I did so > unintentionally. Never mind, thank for all kind of reviews :) > > (Somehow, it turned up as new/unread in my LAKML folder)
diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c index efa402025031..81cf3e6e2a3d 100644 --- a/arch/arm/mm/fault.c +++ b/arch/arm/mm/fault.c @@ -183,74 +183,45 @@ void do_bad_area(unsigned long addr, unsigned int fsr, struct pt_regs *regs) #define VM_FAULT_BADMAP 0x010000 #define VM_FAULT_BADACCESS 0x020000 -/* - * Check that the permissions on the VMA allow for the fault which occurred. - * If we encountered a write fault, we must have write permission, otherwise - * we allow any permission. - */ -static inline bool access_error(unsigned int fsr, struct vm_area_struct *vma) -{ - unsigned int mask = VM_ACCESS_FLAGS; - - if ((fsr & FSR_WRITE) && !(fsr & FSR_CM)) - mask = VM_WRITE; - if (fsr & FSR_LNX_PF) - mask = VM_EXEC; - - return vma->vm_flags & mask ? false : true; -} - static vm_fault_t __kprobes -__do_page_fault(struct mm_struct *mm, unsigned long addr, unsigned int fsr, - unsigned int flags, struct task_struct *tsk, - struct pt_regs *regs) +__do_page_fault(struct mm_struct *mm, unsigned long addr, unsigned int flags, + unsigned long vma_flags, struct pt_regs *regs) { - struct vm_area_struct *vma; - vm_fault_t fault; - - vma = find_vma(mm, addr); - fault = VM_FAULT_BADMAP; + struct vm_area_struct *vma = find_vma(mm, addr); if (unlikely(!vma)) - goto out; - if (unlikely(vma->vm_start > addr)) - goto check_stack; + return VM_FAULT_BADMAP; + + if (unlikely(vma->vm_start > addr)) { + if (!(vma->vm_flags & VM_GROWSDOWN)) + return VM_FAULT_BADMAP; + if (addr < FIRST_USER_ADDRESS) + return VM_FAULT_BADMAP; + if (expand_stack(vma, addr)) + return VM_FAULT_BADMAP; + } /* * Ok, we have a good vm_area for this * memory access, so we can handle it. */ -good_area: - if (access_error(fsr, vma)) { - fault = VM_FAULT_BADACCESS; - goto out; - } + if (!(vma->vm_flags & vma_flags)) + return VM_FAULT_BADACCESS; return handle_mm_fault(vma, addr & PAGE_MASK, flags, regs); - -check_stack: - /* Don't allow expansion below FIRST_USER_ADDRESS */ - if (vma->vm_flags & VM_GROWSDOWN && - addr >= FIRST_USER_ADDRESS && !expand_stack(vma, addr)) - goto good_area; -out: - return fault; } static int __kprobes do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs) { - struct task_struct *tsk; - struct mm_struct *mm; + struct mm_struct *mm = current->mm; int sig, code; vm_fault_t fault; unsigned int flags = FAULT_FLAG_DEFAULT; + unsigned long vm_flags = VM_ACCESS_FLAGS; if (kprobe_page_fault(regs, fsr)) return 0; - tsk = current; - mm = tsk->mm; - /* Enable interrupts if they were enabled in the parent context. */ if (interrupts_enabled(regs)) local_irq_enable(); @@ -264,8 +235,14 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs) if (user_mode(regs)) flags |= FAULT_FLAG_USER; - if ((fsr & FSR_WRITE) && !(fsr & FSR_CM)) + + if ((fsr & FSR_WRITE) && !(fsr & FSR_CM)) { flags |= FAULT_FLAG_WRITE; + vm_flags = VM_WRITE; + } + + if (fsr & FSR_LNX_PF) + vm_flags = VM_EXEC; perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, regs, addr); @@ -293,7 +270,7 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs) #endif } - fault = __do_page_fault(mm, addr, fsr, flags, tsk, regs); + fault = __do_page_fault(mm, addr, flags, vm_flags, regs); /* If we need to retry but a fatal signal is pending, handle the * signal first. We do not need to release the mmap_lock because
1. cleanup access_error(), make vma flags set and check into __do_page_fault() and do_page_fault() directly. 2. drop fsr and task argument, instead, using vm_flags in __do_page_fault(). 3. cleans up the multiple goto statements in __do_page_fault(). 4. use current->mm directly in do_page_fault(). Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> --- arch/arm/mm/fault.c | 73 ++++++++++++++++----------------------------- 1 file changed, 25 insertions(+), 48 deletions(-)