[PATCH -next 0/2] mm: remove arch's private VM_FAULT_BADMAP/BADACCESS

[PATCH -next 0/2] mm: remove arch's private VM_FAULT_BADMAP/BADACCESS

[Kefeng Wang]

Directly set SEGV_MAPRR or SEGV_ACCERR for arm/arm64 to remove the last
two arch's private vm_fault reasons.

Based on next-20240405.

Kefeng Wang (2):
  arm64: mm: drop VM_FAULT_BADMAP/VM_FAULT_BADACCESS
  arm: mm: drop VM_FAULT_BADMAP/VM_FAULT_BADACCESS

 arch/arm/mm/fault.c   | 28 +++++++++++++---------------
 arch/arm64/mm/fault.c | 41 ++++++++++++++++++-----------------------
 2 files changed, 31 insertions(+), 38 deletions(-)

-- 
2.27.0

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

[Kefeng Wang]

If bad map or access, directly set si_code to SEGV_MAPRR or SEGV_ACCERR,
also set fault to 0 and goto error handling, which make us to drop the
arch's special vm fault reason.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 arch/arm64/mm/fault.c | 41 ++++++++++++++++++-----------------------
 1 file changed, 18 insertions(+), 23 deletions(-)

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;
 
 	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;
+	}
 
+	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;
 
+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);
 	}
 
 	return 0;
-- 
2.27.0

[PATCH 2/2] arm: mm: drop VM_FAULT_BADMAP/VM_FAULT_BADACCESS

[Kefeng Wang]

If bad map or access, directly set si_code to SEGV_MAPRR or SEGV_ACCERR,
also set fault to 0 and goto error handling, which make us to drop the
arch's special vm fault reason.

Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
 arch/arm/mm/fault.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
index 5c4b417e24f9..cab24fb17c4b 100644
--- a/arch/arm/mm/fault.c
+++ b/arch/arm/mm/fault.c
@@ -226,9 +226,6 @@ void do_bad_area(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 }
 
 #ifdef CONFIG_MMU
-#define VM_FAULT_BADMAP		((__force vm_fault_t)0x010000)
-#define VM_FAULT_BADACCESS	((__force vm_fault_t)0x020000)
-
 static inline bool is_permission_fault(unsigned int fsr)
 {
 	int fs = fsr_fs(fsr);
@@ -295,7 +292,8 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 	if (!(vma->vm_flags & vm_flags)) {
 		vma_end_read(vma);
 		count_vm_vma_lock_event(VMA_LOCK_SUCCESS);
-		fault = VM_FAULT_BADACCESS;
+		fault = 0;
+		code = SEGV_ACCERR;
 		goto bad_area;
 	}
 	fault = handle_mm_fault(vma, addr, flags | FAULT_FLAG_VMA_LOCK, regs);
@@ -321,7 +319,8 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 retry:
 	vma = lock_mm_and_find_vma(mm, addr, regs);
 	if (unlikely(!vma)) {
-		fault = VM_FAULT_BADMAP;
+		fault = 0;
+		code = SEGV_MAPERR;
 		goto bad_area;
 	}
 
@@ -329,10 +328,13 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 	 * ok, we have a good vm_area for this memory access, check the
 	 * permissions on the VMA allow for the fault which occurred.
 	 */
-	if (!(vma->vm_flags & vm_flags))
-		fault = VM_FAULT_BADACCESS;
-	else
-		fault = handle_mm_fault(vma, addr & PAGE_MASK, flags, regs);
+	if (!(vma->vm_flags & vm_flags)) {
+		fault = 0;
+		code = SEGV_ACCERR;
+		goto bad_area;
+	}
+
+	fault = handle_mm_fault(vma, addr & PAGE_MASK, 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
@@ -358,10 +360,8 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 	mmap_read_unlock(mm);
 done:
 
-	/*
-	 * Handle the "normal" case first - VM_FAULT_MAJOR
-	 */
-	if (likely(!(fault & (VM_FAULT_ERROR | VM_FAULT_BADMAP | VM_FAULT_BADACCESS))))
+	/* Handle the "normal" case first */
+	if (likely(!(fault & VM_FAULT_ERROR)))
 		return 0;
 
 bad_area:
@@ -395,8 +395,6 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
 		 * isn't in our memory map..
 		 */
 		sig = SIGSEGV;
-		code = fault == VM_FAULT_BADACCESS ?
-			SEGV_ACCERR : SEGV_MAPERR;
 	}
 
 	__do_user_fault(addr, fsr, sig, code, regs);
-- 
2.27.0

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

[Catalin Marinas]

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().

>  	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).

>  
> +	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;
>  
> +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.

-- 
Catalin

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

[Kefeng Wang]

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;
>>   
>> +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.


> 

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

[Kefeng Wang]

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.
> 
> 
>>
> 

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

[Aishwarya TCV]

On 07/04/2024 09:12, Kefeng Wang wrote:
> If bad map or access, directly set si_code to SEGV_MAPRR or SEGV_ACCERR,
> also set fault to 0 and goto error handling, which make us to drop the
> arch's special vm fault reason.
> 
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
>  arch/arm64/mm/fault.c | 41 ++++++++++++++++++-----------------------
>  1 file changed, 18 insertions(+), 23 deletions(-)

Hi Kefeng,

While running LTP test suite, I observed that few test cases are unable
to kill exe when run against next-master(next-20240409) kernel with
Arm64 on JUNO in our CI. I can send the full logs if required, but it
doesn't say much.

A bisect identified cf0049a15207a5a78798105eff789c2025bcf652 as the
first bad commit. Bisected it on the tag "next-20240409" at repo
"https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git".

This works fine on Linux version v6.9-rc3

Here are some of the failing test cases in LTP quickhit:
-----------------------------
chdir01
link04
select03
unlink07


Failure log:
-----------
06:06:49.288908  Test timeouted, sending SIGKILL!
06:06:54.293806  Test timeouted, sending SIGKILL!
06:06:59.296872  Test timeouted, sending SIGKILL!
06:07:04.292044  Test timeouted, sending SIGKILL!
06:07:09.290185  Test timeouted, sending SIGKILL!
06:07:14.295134  Test timeouted, sending SIGKILL!
06:07:19.293279  Test timeouted, sending SIGKILL!
06:07:24.292405  Test timeouted, sending SIGKILL!
06:07:24.292790  Cannot kill test processes!
06:07:24.296564  Congratulation, likely test hit a kernel bug.
06:07:24.301315  Exiting uncleanly...


Bisect log:
----------
git bisect start
# good: [fec50db7033ea478773b159e0e2efb135270e3b7] Linux 6.9-rc3
git bisect good fec50db7033ea478773b159e0e2efb135270e3b7
# bad: [a053fd3ca5d1b927a8655f239c84b0d790218fda] Add linux-next
specific files for 20240409
git bisect bad a053fd3ca5d1b927a8655f239c84b0d790218fda
# bad: [4eb0063b031ea720cd8971e3e3d2426d27c5d7a6] Merge branch
'mtd/next' of git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git
git bisect bad 4eb0063b031ea720cd8971e3e3d2426d27c5d7a6
# bad: [24dde11bfadd5f38c6cca3cea6f16971bd10dc86] Merge branch
'for-next' of git://github.com/Xilinx/linux-xlnx.git
git bisect bad 24dde11bfadd5f38c6cca3cea6f16971bd10dc86
# bad: [bef23348d7e75c502399ba1a24627aa447b816dc] Merge branch
'mm-everything' of git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
git bisect bad bef23348d7e75c502399ba1a24627aa447b816dc
# good: [3317f7faabc24b500c26d02615ac75ca2786e272] mm: swap:
free_swap_and_cache_nr() as batched free_swap_and_cache()
git bisect good 3317f7faabc24b500c26d02615ac75ca2786e272
# good: [541970e62546ff5c96622669f2796d43b1a406e3] Merge branch
'gpio/for-current' of
git://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git
git bisect good 541970e62546ff5c96622669f2796d43b1a406e3
# bad: [1df265717e0f9d96079073189f6e6c52a48e493c]
__mod_memcg_lruvec_state-enhance-diagnostics-fix
git bisect bad 1df265717e0f9d96079073189f6e6c52a48e493c
# bad: [19ab4054346474c2b456f9bff6a98e41e5e46224] hugetlb: Simplify
hugetlb_wp() arguments
git bisect bad 19ab4054346474c2b456f9bff6a98e41e5e46224
# good: [75ef450aa9828ca20817c46c5b99f131156f6eee] x86: mm: accelerate
pagefault when badaccess
git bisect good 75ef450aa9828ca20817c46c5b99f131156f6eee
# bad: [c6b5a19e679bb713efd57d82f9080f780e0bb60a] mm/treewide: rename
CONFIG_HAVE_FAST_GUP to CONFIG_HAVE_GUP_FAST
git bisect bad c6b5a19e679bb713efd57d82f9080f780e0bb60a
# bad: [01446d1e9e627945664f2a1daa53e8720946d0cd] mm: remove struct page
from get_shadow_from_swap_cache
git bisect bad 01446d1e9e627945664f2a1daa53e8720946d0cd
# bad: [8f9d6a30dd992c44debea3161083a6c2cd3ad87f] arm: mm: drop
VM_FAULT_BADMAP/VM_FAULT_BADACCESS
git bisect bad 8f9d6a30dd992c44debea3161083a6c2cd3ad87f
# bad: [cf0049a15207a5a78798105eff789c2025bcf652] arm64: mm: drop
VM_FAULT_BADMAP/VM_FAULT_BADACCESS
git bisect bad cf0049a15207a5a78798105eff789c2025bcf652
# first bad commit: [cf0049a15207a5a78798105eff789c2025bcf652] arm64:
mm: drop VM_FAULT_BADMAP/VM_FAULT_BADACCESSgit bisect log


Thanks,
Aishwarya

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

[Kefeng Wang]

On 2024/4/10 19:24, Aishwarya TCV wrote:
> 
> 
> On 07/04/2024 09:12, Kefeng Wang wrote:
>> If bad map or access, directly set si_code to SEGV_MAPRR or SEGV_ACCERR,
>> also set fault to 0 and goto error handling, which make us to drop the
>> arch's special vm fault reason.
>>
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> ---
>>   arch/arm64/mm/fault.c | 41 ++++++++++++++++++-----------------------
>>   1 file changed, 18 insertions(+), 23 deletions(-)
> 
> Hi Kefeng,
> 
> While running LTP test suite, I observed that few test cases are unable
> to kill exe when run against next-master(next-20240409) kernel with
> Arm64 on JUNO in our CI. I can send the full logs if required, but it
> doesn't say much.

Sorry about it, as Catalin pointed, there is issue in this patch, and
I replied today, could you help to test with following changes, many thanks.

diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 61a2acae0dca..451ba7cbd5ad 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -604,6 +604,7 @@ static int __kprobes do_page_fault(unsigned long 
far, unsigned long esr,
         }

         if (!(vma->vm_flags & vm_flags)) {
+               mmap_read_unlock(mm);
                 fault = 0;
                 si_code = SEGV_ACCERR;
                 goto bad_area;
@@ -632,6 +633,7 @@ static int __kprobes do_page_fault(unsigned long 
far, unsigned long esr,
         if (likely(!(fault & VM_FAULT_ERROR)))
                 return 0;

+       si_code = SEGV_MAPERR;
  bad_area:
         /*
          * If we are in kernel mode at this point, we have no context to

> 
> A bisect identified cf0049a15207a5a78798105eff789c2025bcf652 as the
> first bad commit. Bisected it on the tag "next-20240409" at repo
> "https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git".
> 
> This works fine on Linux version v6.9-rc3
> 
> Here are some of the failing test cases in LTP quickhit:
> -----------------------------
> chdir01
> link04
> select03
> unlink07
> 
> 
> Failure log:
> -----------
> 06:06:49.288908  Test timeouted, sending SIGKILL!
> 06:06:54.293806  Test timeouted, sending SIGKILL!
> 06:06:59.296872  Test timeouted, sending SIGKILL!
> 06:07:04.292044  Test timeouted, sending SIGKILL!
> 06:07:09.290185  Test timeouted, sending SIGKILL!
> 06:07:14.295134  Test timeouted, sending SIGKILL!
> 06:07:19.293279  Test timeouted, sending SIGKILL!
> 06:07:24.292405  Test timeouted, sending SIGKILL!
> 06:07:24.292790  Cannot kill test processes!
> 06:07:24.296564  Congratulation, likely test hit a kernel bug.
> 06:07:24.301315  Exiting uncleanly...
> 
> 
> Bisect log:
> ----------
> git bisect start
> # good: [fec50db7033ea478773b159e0e2efb135270e3b7] Linux 6.9-rc3
> git bisect good fec50db7033ea478773b159e0e2efb135270e3b7
> # bad: [a053fd3ca5d1b927a8655f239c84b0d790218fda] Add linux-next
> specific files for 20240409
> git bisect bad a053fd3ca5d1b927a8655f239c84b0d790218fda
> # bad: [4eb0063b031ea720cd8971e3e3d2426d27c5d7a6] Merge branch
> 'mtd/next' of git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git
> git bisect bad 4eb0063b031ea720cd8971e3e3d2426d27c5d7a6
> # bad: [24dde11bfadd5f38c6cca3cea6f16971bd10dc86] Merge branch
> 'for-next' of git://github.com/Xilinx/linux-xlnx.git
> git bisect bad 24dde11bfadd5f38c6cca3cea6f16971bd10dc86
> # bad: [bef23348d7e75c502399ba1a24627aa447b816dc] Merge branch
> 'mm-everything' of git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
> git bisect bad bef23348d7e75c502399ba1a24627aa447b816dc
> # good: [3317f7faabc24b500c26d02615ac75ca2786e272] mm: swap:
> free_swap_and_cache_nr() as batched free_swap_and_cache()
> git bisect good 3317f7faabc24b500c26d02615ac75ca2786e272
> # good: [541970e62546ff5c96622669f2796d43b1a406e3] Merge branch
> 'gpio/for-current' of
> git://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git
> git bisect good 541970e62546ff5c96622669f2796d43b1a406e3
> # bad: [1df265717e0f9d96079073189f6e6c52a48e493c]
> __mod_memcg_lruvec_state-enhance-diagnostics-fix
> git bisect bad 1df265717e0f9d96079073189f6e6c52a48e493c
> # bad: [19ab4054346474c2b456f9bff6a98e41e5e46224] hugetlb: Simplify
> hugetlb_wp() arguments
> git bisect bad 19ab4054346474c2b456f9bff6a98e41e5e46224
> # good: [75ef450aa9828ca20817c46c5b99f131156f6eee] x86: mm: accelerate
> pagefault when badaccess
> git bisect good 75ef450aa9828ca20817c46c5b99f131156f6eee
> # bad: [c6b5a19e679bb713efd57d82f9080f780e0bb60a] mm/treewide: rename
> CONFIG_HAVE_FAST_GUP to CONFIG_HAVE_GUP_FAST
> git bisect bad c6b5a19e679bb713efd57d82f9080f780e0bb60a
> # bad: [01446d1e9e627945664f2a1daa53e8720946d0cd] mm: remove struct page
> from get_shadow_from_swap_cache
> git bisect bad 01446d1e9e627945664f2a1daa53e8720946d0cd
> # bad: [8f9d6a30dd992c44debea3161083a6c2cd3ad87f] arm: mm: drop
> VM_FAULT_BADMAP/VM_FAULT_BADACCESS
> git bisect bad 8f9d6a30dd992c44debea3161083a6c2cd3ad87f
> # bad: [cf0049a15207a5a78798105eff789c2025bcf652] arm64: mm: drop
> VM_FAULT_BADMAP/VM_FAULT_BADACCESS
> git bisect bad cf0049a15207a5a78798105eff789c2025bcf652
> # first bad commit: [cf0049a15207a5a78798105eff789c2025bcf652] arm64:
> mm: drop VM_FAULT_BADMAP/VM_FAULT_BADACCESSgit bisect log
> 
> 
> Thanks,
> Aishwarya

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

[Cristian Marussi]

On Wed, Apr 10, 2024 at 07:53:21PM +0800, Kefeng Wang wrote:
> 
> 
> On 2024/4/10 19:24, Aishwarya TCV wrote:
> > 

Hi,

> > 
> > On 07/04/2024 09:12, Kefeng Wang wrote:
> > > If bad map or access, directly set si_code to SEGV_MAPRR or SEGV_ACCERR,
> > > also set fault to 0 and goto error handling, which make us to drop the
> > > arch's special vm fault reason.
> > > 
> > > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> > > ---
> > >   arch/arm64/mm/fault.c | 41 ++++++++++++++++++-----------------------
> > >   1 file changed, 18 insertions(+), 23 deletions(-)
> > 
> > Hi Kefeng,
> > 
> > While running LTP test suite, I observed that few test cases are unable
> > to kill exe when run against next-master(next-20240409) kernel with
> > Arm64 on JUNO in our CI. I can send the full logs if required, but it
> > doesn't say much.
> 
> Sorry about it, as Catalin pointed, there is issue in this patch, and
> I replied today, could you help to test with following changes, many thanks.
> 
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index 61a2acae0dca..451ba7cbd5ad 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -604,6 +604,7 @@ static int __kprobes do_page_fault(unsigned long far,
> unsigned long esr,
>         }
> 
>         if (!(vma->vm_flags & vm_flags)) {
> +               mmap_read_unlock(mm);
>                 fault = 0;
>                 si_code = SEGV_ACCERR;
>                 goto bad_area;
> @@ -632,6 +633,7 @@ static int __kprobes do_page_fault(unsigned long far,
> unsigned long esr,
>         if (likely(!(fault & VM_FAULT_ERROR)))
>                 return 0;
> 
> +       si_code = SEGV_MAPERR;
>  bad_area:
>         /*
>          * If we are in kernel mode at this point, we have no context to
> 

On my JUNO setup, the above patch solves the 'un-killable tests' issues and
all the previously failing tests passes. (I have NOT run the full LTP
test-suite, though, only replayed the tests that were failing before as
mentioned by Aishwarya)

Thanks,
Cristian

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

[Kefeng Wang]

On 2024/4/10 20:39, Cristian Marussi wrote:
> On Wed, Apr 10, 2024 at 07:53:21PM +0800, Kefeng Wang wrote:
>>
>>
>> On 2024/4/10 19:24, Aishwarya TCV wrote:
>>>
> 
> Hi,
> 
>>>
>>> On 07/04/2024 09:12, Kefeng Wang wrote:
>>>> If bad map or access, directly set si_code to SEGV_MAPRR or SEGV_ACCERR,
>>>> also set fault to 0 and goto error handling, which make us to drop the
>>>> arch's special vm fault reason.
>>>>
>>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>>>> ---
>>>>    arch/arm64/mm/fault.c | 41 ++++++++++++++++++-----------------------
>>>>    1 file changed, 18 insertions(+), 23 deletions(-)
>>>
>>> Hi Kefeng,
>>>
>>> While running LTP test suite, I observed that few test cases are unable
>>> to kill exe when run against next-master(next-20240409) kernel with
>>> Arm64 on JUNO in our CI. I can send the full logs if required, but it
>>> doesn't say much.
>>
>> Sorry about it, as Catalin pointed, there is issue in this patch, and
>> I replied today, could you help to test with following changes, many thanks.
>>
>> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
>> index 61a2acae0dca..451ba7cbd5ad 100644
>> --- a/arch/arm64/mm/fault.c
>> +++ b/arch/arm64/mm/fault.c
>> @@ -604,6 +604,7 @@ static int __kprobes do_page_fault(unsigned long far,
>> unsigned long esr,
>>          }
>>
>>          if (!(vma->vm_flags & vm_flags)) {
>> +               mmap_read_unlock(mm);
>>                  fault = 0;
>>                  si_code = SEGV_ACCERR;
>>                  goto bad_area;
>> @@ -632,6 +633,7 @@ static int __kprobes do_page_fault(unsigned long far,
>> unsigned long esr,
>>          if (likely(!(fault & VM_FAULT_ERROR)))
>>                  return 0;
>>
>> +       si_code = SEGV_MAPERR;
>>   bad_area:
>>          /*
>>           * If we are in kernel mode at this point, we have no context to
>>
> 
> On my JUNO setup, the above patch solves the 'un-killable tests' issues and
> all the previously failing tests passes. (I have NOT run the full LTP
> test-suite, though, only replayed the tests that were failing before as
> mentioned by Aishwarya)

Thanks, I will resend v2, but let's wait for Catalin to see if there is 
any other opinion.
> 
> Thanks,
> Cristian

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

[Andrew Morton]

On Wed, 10 Apr 2024 20:48:28 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote:

> > On my JUNO setup, the above patch solves the 'un-killable tests' issues and
> > all the previously failing tests passes. (I have NOT run the full LTP
> > test-suite, though, only replayed the tests that were failing before as
> > mentioned by Aishwarya)
> 
> Thanks, I will resend v2, but let's wait for Catalin to see if there is 
> any other opinion.

Thanks, I dropped v1.

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

[Catalin Marinas]

On Wed, Apr 10, 2024 at 06:58:27PM +0800, Kefeng Wang wrote:
> On 2024/4/10 9:30, Kefeng Wang wrote:
> > On 2024/4/9 22:28, Catalin Marinas wrote:
> > > 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().
[...]
> > > > +    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.

Yes, I think initialising it here would be fine. That's the fall-through
case I was concerned about. All the other goto bad_area places already
initialise si_code.

-- 
Catalin

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

[Kefeng Wang]

On 2024/4/11 17:59, Catalin Marinas wrote:
> On Wed, Apr 10, 2024 at 06:58:27PM +0800, Kefeng Wang wrote:
>> On 2024/4/10 9:30, Kefeng Wang wrote:
>>> On 2024/4/9 22:28, Catalin Marinas wrote:
>>>> 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().
> [...]
>>>>> +    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.
> 
> Yes, I think initialising it here would be fine. That's the fall-through
> case I was concerned about. All the other goto bad_area places already
> initialise si_code.
> 
Thanks for your confirm, will send v2 soon.