Re: [PATCH 1/2] arm64: mm: drop VM_FAULT_BADMAP/VM_FAULT_BADACCESS

[Kefeng Wang <wangkefeng.wang@huawei.com>]

On 2024/4/10 9:30, Kefeng Wang wrote:
> 
> 
> On 2024/4/9 22:28, Catalin Marinas wrote:
>> Hi Kefeng,
>>
>> On Sun, Apr 07, 2024 at 04:12:10PM +0800, Kefeng Wang wrote:
>>> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
>>> index 405f9aa831bd..61a2acae0dca 100644
>>> --- a/arch/arm64/mm/fault.c
>>> +++ b/arch/arm64/mm/fault.c
>>> @@ -500,9 +500,6 @@ static bool is_write_abort(unsigned long esr)
>>>       return (esr & ESR_ELx_WNR) && !(esr & ESR_ELx_CM);
>>>   }
>>> -#define VM_FAULT_BADMAP        ((__force vm_fault_t)0x010000)
>>> -#define VM_FAULT_BADACCESS    ((__force vm_fault_t)0x020000)
>>> -
>>>   static int __kprobes do_page_fault(unsigned long far, unsigned long 
>>> esr,
>>>                      struct pt_regs *regs)
>>>   {
>>> @@ -513,6 +510,7 @@ static int __kprobes do_page_fault(unsigned long 
>>> far, unsigned long esr,
>>>       unsigned int mm_flags = FAULT_FLAG_DEFAULT;
>>>       unsigned long addr = untagged_addr(far);
>>>       struct vm_area_struct *vma;
>>> +    int si_code;
>>
>> I think we should initialise this to 0. Currently all paths seem to set
>> si_code to something meaningful but I'm not sure the last 'else' close
>> in this patch is guaranteed to always cover exactly those earlier code
>> paths updating si_code. I'm not talking about the 'goto bad_area' paths
>> since they set 'fault' to 0 but the fall through after the second (under
>> the mm lock) handle_mm_fault().
> 
> Recheck it, without this patch, the second handle_mm_fault() never
> return VM_FAULT_BADACCESS, but could return VM_FAULT_SIGSEGV(maybe
> other), which not handled in the other error path,
> 
>   handle_mm_fault
>      ret = sanitize_fault_flags(vma, &flags);
>      if (!arch_vma_access_permitted())
>       ret = VM_FAULT_SIGSEGV;
> 
> so the orignal logical will set si_code to SEGV_MAPERR
> 
>    fault == VM_FAULT_BADACCESS ? SEGV_ACCERR : SEGV_MAPERR,
> 
> therefore, i think we should set the default si_code to SEGV_MAPERR.
> 
> 
>>
>>>       if (kprobe_page_fault(regs, esr))
>>>           return 0;
>>> @@ -572,9 +570,10 @@ static int __kprobes do_page_fault(unsigned long 
>>> far, unsigned long esr,
>>>       if (!(vma->vm_flags & vm_flags)) {
>>>           vma_end_read(vma);
>>> -        fault = VM_FAULT_BADACCESS;
>>> +        fault = 0;
>>> +        si_code = SEGV_ACCERR;
>>>           count_vm_vma_lock_event(VMA_LOCK_SUCCESS);
>>> -        goto done;
>>> +        goto bad_area;
>>>       }
>>>       fault = handle_mm_fault(vma, addr, mm_flags | 
>>> FAULT_FLAG_VMA_LOCK, regs);
>>>       if (!(fault & (VM_FAULT_RETRY | VM_FAULT_COMPLETED)))
>>> @@ -599,15 +598,18 @@ static int __kprobes do_page_fault(unsigned 
>>> long far, unsigned long esr,
>>>   retry:
>>>       vma = lock_mm_and_find_vma(mm, addr, regs);
>>>       if (unlikely(!vma)) {
>>> -        fault = VM_FAULT_BADMAP;
>>> -        goto done;
>>> +        fault = 0;
>>> +        si_code = SEGV_MAPERR;
>>> +        goto bad_area;
>>>       }
>>> -    if (!(vma->vm_flags & vm_flags))
>>> -        fault = VM_FAULT_BADACCESS;
>>> -    else
>>> -        fault = handle_mm_fault(vma, addr, mm_flags, regs);
>>> +    if (!(vma->vm_flags & vm_flags)) {
>>> +        fault = 0;
>>> +        si_code = SEGV_ACCERR;
>>> +        goto bad_area;
>>> +    }
>>
>> What's releasing the mm lock here? Prior to this change, it is falling
>> through to mmap_read_unlock() below or handle_mm_fault() was releasing
>> the lock (VM_FAULT_RETRY, VM_FAULT_COMPLETED).
> 
> Indeed, will fix,
> 
>>
>>> +    fault = handle_mm_fault(vma, addr, mm_flags, regs);
>>>       /* Quick path to respond to signals */
>>>       if (fault_signal_pending(fault, regs)) {
>>>           if (!user_mode(regs))
>>> @@ -626,13 +628,11 @@ static int __kprobes do_page_fault(unsigned 
>>> long far, unsigned long esr,
>>>       mmap_read_unlock(mm);
>>>   done:
>>> -    /*
>>> -     * Handle the "normal" (no error) case first.
>>> -     */
>>> -    if (likely(!(fault & (VM_FAULT_ERROR | VM_FAULT_BADMAP |
>>> -                  VM_FAULT_BADACCESS))))
>>> +    /* Handle the "normal" (no error) case first. */
>>> +    if (likely(!(fault & VM_FAULT_ERROR)))
>>>           return 0;

Another choice, we set si_code = SEGV_MAPERR here, since normal
pagefault don't use si_code, only the error patch need to initialize.


>>> +bad_area:
>>>       /*
>>>        * If we are in kernel mode at this point, we have no context to
>>>        * handle this fault with.
>>> @@ -667,13 +667,8 @@ static int __kprobes do_page_fault(unsigned long 
>>> far, unsigned long esr,
>>>           arm64_force_sig_mceerr(BUS_MCEERR_AR, far, lsb, inf->name);
>>>       } else {
>>> -        /*
>>> -         * Something tried to access memory that isn't in our memory
>>> -         * map.
>>> -         */
>>> -        arm64_force_sig_fault(SIGSEGV,
>>> -                      fault == VM_FAULT_BADACCESS ? SEGV_ACCERR : 
>>> SEGV_MAPERR,
>>> -                      far, inf->name);
>>> +        /* Something tried to access memory that out of memory map */
>>> +        arm64_force_sig_fault(SIGSEGV, si_code, far, inf->name);
>>>       }
>>
>> We can get to the 'else' close after the second handle_mm_fault(). Do we
>> guarantee that 'fault == 0' in this last block? If not, maybe a warning
>> and some safe initialisation for 'si_code' to avoid leaking stack data.
> 
> As analyzed above, it is sufficient that make si_code to SEGV_MAPPER by 
> default, right?
> 
> Thanks.
> 
> 
>>
>