[PATCH 0/3] Enhance soft hwpoison handling and injection

[PATCH 0/3] Enhance soft hwpoison handling and injection

[Jane Chu]

This series aim at the following enhancement -
1. Let one hwpoison injector, that is, madvise(MADV_HWPOISON) to behave
   more like as if a real UE occurred. Because the other two injectors
   such as hwpoison-inject and the 'einj' on x86 can't, and it seems to
   me we need a better simulation to real UE scenario.
2. For years, if the kernel is unable to unmap a hwpoisoned page, it send
   a SIGKILL instead of SIGBUS to prevent user process from potentially
   accessing the page again. But in doing so, the user process also lose
   important information: vaddr, for recovery.  Fortunately, the kernel
   already has code to kill process re-accessing a hwpoisoned page, so
   remove the '!unmap_success' check.
3. Right now, if a thp page under GUP longterm pin is hwpoisoned, and
   kernel cannot split the thp page, memory-failure simply ignores
   the UE and returns.  That's not ideal, it could deliver a SIGBUS with
   useful information for userspace recovery.


Jane Chu (3):
  mm/memory-failure: try to send SIGBUS even if unmap failed
  mm/madvise: Add MF_ACTION_REQUIRED to madvise(MADV_HWPOISON)
  mm/memory-failure: send SIGBUS in the event of thp split fail

 mm/madvise.c        |  2 +-
 mm/memory-failure.c | 49 ++++++++++++++++++++++++++++++++++++---------
 2 files changed, 41 insertions(+), 10 deletions(-)

-- 
2.39.3

[PATCH 1/3] mm/memory-failure: try to send SIGBUS even if unmap failed

[Jane Chu]

For years when it comes down to kill a process due to hwpoison,
a SIGBUS is delivered only if unmap has been successful.
Otherwise, a SIGKILL is delivered. And the reason for that is
to prevent the involved process from accessing the hwpoisoned
page again.

Since then a lot has changed, a hwpoisoned page is marked and
upon being re-accessed, the process will be killed immediately.
So let's take out the '!unmap_success' factor and try to deliver
SIGBUS if possible.

Signed-off-by: Jane Chu <jane.chu@oracle.com>
---
 mm/memory-failure.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 9e62a00b46dd..7fcf182abb96 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -519,19 +519,14 @@ void add_to_kill_ksm(struct task_struct *tsk, struct page *p,
  * Also when FAIL is set do a force kill because something went
  * wrong earlier.
  */
-static void kill_procs(struct list_head *to_kill, int forcekill, bool fail,
+static void kill_procs(struct list_head *to_kill, int forcekill,
 		unsigned long pfn, int flags)
 {
 	struct to_kill *tk, *next;
 
 	list_for_each_entry_safe(tk, next, to_kill, nd) {
 		if (forcekill) {
-			/*
-			 * In case something went wrong with munmapping
-			 * make sure the process doesn't catch the
-			 * signal and then access the memory. Just kill it.
-			 */
-			if (fail || tk->addr == -EFAULT) {
+			if (tk->addr == -EFAULT) {
 				pr_err("%#lx: forcibly killing %s:%d because of failure to unmap corrupted page\n",
 				       pfn, tk->tsk->comm, tk->tsk->pid);
 				do_send_sig_info(SIGKILL, SEND_SIG_PRIV,
@@ -1666,7 +1661,7 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
 	 */
 	forcekill = PageDirty(hpage) || (flags & MF_MUST_KILL) ||
 		    !unmap_success;
-	kill_procs(&tokill, forcekill, !unmap_success, pfn, flags);
+	kill_procs(&tokill, forcekill, pfn, flags);
 
 	return unmap_success;
 }
@@ -1730,7 +1725,7 @@ static void unmap_and_kill(struct list_head *to_kill, unsigned long pfn,
 		unmap_mapping_range(mapping, start, size, 0);
 	}
 
-	kill_procs(to_kill, flags & MF_MUST_KILL, false, pfn, flags);
+	kill_procs(to_kill, flags & MF_MUST_KILL, pfn, flags);
 }
 
 /*
-- 
2.39.3

Re: [PATCH 1/3] mm/memory-failure: try to send SIGBUS even if unmap failed

[Oscar Salvador]

On Wed, May 01, 2024 at 05:24:56PM -0600, Jane Chu wrote:
> For years when it comes down to kill a process due to hwpoison,
> a SIGBUS is delivered only if unmap has been successful.
> Otherwise, a SIGKILL is delivered. And the reason for that is
> to prevent the involved process from accessing the hwpoisoned
> page again.
> 
> Since then a lot has changed, a hwpoisoned page is marked and
> upon being re-accessed, the process will be killed immediately.
> So let's take out the '!unmap_success' factor and try to deliver
> SIGBUS if possible.

I am missing some details here.
An unmapped hwpoison page will trigger a fault and will return
VM_FAULT_HWPOISON all the way down and then deliver SIGBUS,
but if the page was not unmapped, how will this be catch upon
re-accessing? Will the system deliver a MCE event? 


-- 
Oscar Salvador
SUSE Labs

Re: [PATCH 1/3] mm/memory-failure: try to send SIGBUS even if unmap failed

[Jane Chu]

On 5/7/2024 2:02 AM, Oscar Salvador wrote:

> On Wed, May 01, 2024 at 05:24:56PM -0600, Jane Chu wrote:
>> For years when it comes down to kill a process due to hwpoison,
>> a SIGBUS is delivered only if unmap has been successful.
>> Otherwise, a SIGKILL is delivered. And the reason for that is
>> to prevent the involved process from accessing the hwpoisoned
>> page again.
>>
>> Since then a lot has changed, a hwpoisoned page is marked and
>> upon being re-accessed, the process will be killed immediately.
>> So let's take out the '!unmap_success' factor and try to deliver
>> SIGBUS if possible.
> I am missing some details here.
> An unmapped hwpoison page will trigger a fault and will return
> VM_FAULT_HWPOISON all the way down and then deliver SIGBUS,
> but if the page was not unmapped, how will this be catch upon
> re-accessing? Will the system deliver a MCE event?
>
I actually managed to hit the re-access case with an older version of 
Linux -

MCE occurred, but unmap failed,  no SIGBUS and test process re-access

the same address over and over (hence MCE after MCE), as the CPU

was unable to make forward progress.   In reality, this issue is fixed with

kill_accessing_processes().  The comment for this patch refers to 
comment made

about '!unmap_access' long time ago.

thanks,

-jane

Re: [PATCH 1/3] mm/memory-failure: try to send SIGBUS even if unmap failed

[Oscar Salvador]

On Tue, May 07, 2024 at 10:54:10AM -0700, Jane Chu wrote:
> I actually managed to hit the re-access case with an older version of Linux
> -
> 
> MCE occurred, but unmap failed,  no SIGBUS and test process re-access
> 
> the same address over and over (hence MCE after MCE), as the CPU
> 
> was unable to make forward progress.   In reality, this issue is fixed with
> 
> kill_accessing_processes().  The comment for this patch refers to comment
> made

So we get a faulty page and we try to unmap it from all processes that
might have it mapped in their pgtables.
Prior to this patch we would kill the processes right away and now we
deliver a SIGBUS.

Seems safe as upon-reaccesing kill_accessing_process() will be called
for already hwpoisoned pages.

I think the changelog could be made more explicit about this scenario
and state the role of kill_accessing_process more clear.

With that: Reviewed-by: Oscar Salvador <osalvador@suse.de>
 

-- 
Oscar Salvador
SUSE Labs

Re: [PATCH 1/3] mm/memory-failure: try to send SIGBUS even if unmap failed

[Jane Chu]

On 5/8/2024 5:06 AM, Oscar Salvador wrote:

> On Tue, May 07, 2024 at 10:54:10AM -0700, Jane Chu wrote:
>> I actually managed to hit the re-access case with an older version of Linux
>> -
>>
>> MCE occurred, but unmap failed,  no SIGBUS and test process re-access
>>
>> the same address over and over (hence MCE after MCE), as the CPU
>>
>> was unable to make forward progress.   In reality, this issue is fixed with
>>
>> kill_accessing_processes().  The comment for this patch refers to comment
>> made
> So we get a faulty page and we try to unmap it from all processes that
> might have it mapped in their pgtables.
> Prior to this patch we would kill the processes right away and now we
> deliver a SIGBUS.
>
> Seems safe as upon-reaccesing kill_accessing_process() will be called
> for already hwpoisoned pages.
>
> I think the changelog could be made more explicit about this scenario
> and state the role of kill_accessing_process more clear.
>
> With that: Reviewed-by: Oscar Salvador <osalvador@suse.de>
>   

I will revise the changelog and mention kill_accessing_process().

Thanks!

-jane

>

Re: [PATCH 1/3] mm/memory-failure: try to send SIGBUS even if unmap failed

[Miaohe Lin]

On 2024/5/2 7:24, Jane Chu wrote:
> For years when it comes down to kill a process due to hwpoison,
> a SIGBUS is delivered only if unmap has been successful.
> Otherwise, a SIGKILL is delivered. And the reason for that is
> to prevent the involved process from accessing the hwpoisoned
> page again.
> 
> Since then a lot has changed, a hwpoisoned page is marked and
> upon being re-accessed, the process will be killed immediately.
> So let's take out the '!unmap_success' factor and try to deliver
> SIGBUS if possible.
> 
> Signed-off-by: Jane Chu <jane.chu@oracle.com>
> ---
>  mm/memory-failure.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 9e62a00b46dd..7fcf182abb96 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -519,19 +519,14 @@ void add_to_kill_ksm(struct task_struct *tsk, struct page *p,
>   * Also when FAIL is set do a force kill because something went
>   * wrong earlier.

Since @fail is removed, above comment should be removed too.
Thanks.
.

Re: [PATCH 1/3] mm/memory-failure: try to send SIGBUS even if unmap failed

[Jane Chu]

On 5/8/2024 12:47 AM, Miaohe Lin wrote:

> On 2024/5/2 7:24, Jane Chu wrote:
>> For years when it comes down to kill a process due to hwpoison,
>> a SIGBUS is delivered only if unmap has been successful.
>> Otherwise, a SIGKILL is delivered. And the reason for that is
>> to prevent the involved process from accessing the hwpoisoned
>> page again.
>>
>> Since then a lot has changed, a hwpoisoned page is marked and
>> upon being re-accessed, the process will be killed immediately.
>> So let's take out the '!unmap_success' factor and try to deliver
>> SIGBUS if possible.
>>
>> Signed-off-by: Jane Chu <jane.chu@oracle.com>
>> ---
>>   mm/memory-failure.c | 13 ++++---------
>>   1 file changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index 9e62a00b46dd..7fcf182abb96 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -519,19 +519,14 @@ void add_to_kill_ksm(struct task_struct *tsk, struct page *p,
>>    * Also when FAIL is set do a force kill because something went
>>    * wrong earlier.
> Since @fail is removed, above comment should be removed too.
> Thanks.
> .

Good catch, will do.

Thanks!

-jane

Re: [PATCH 1/3] mm/memory-failure: try to send SIGBUS even if unmap failed

[Miaohe Lin]

On 2024/5/2 7:24, Jane Chu wrote:
> For years when it comes down to kill a process due to hwpoison,
> a SIGBUS is delivered only if unmap has been successful.
> Otherwise, a SIGKILL is delivered. And the reason for that is
> to prevent the involved process from accessing the hwpoisoned
> page again.
> 
> Since then a lot has changed, a hwpoisoned page is marked and
> upon being re-accessed, the process will be killed immediately.
> So let's take out the '!unmap_success' factor and try to deliver
> SIGBUS if possible.
> 
> Signed-off-by: Jane Chu <jane.chu@oracle.com>
> ---
>  mm/memory-failure.c | 13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 9e62a00b46dd..7fcf182abb96 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -519,19 +519,14 @@ void add_to_kill_ksm(struct task_struct *tsk, struct page *p,
>   * Also when FAIL is set do a force kill because something went
>   * wrong earlier.
>   */
> -static void kill_procs(struct list_head *to_kill, int forcekill, bool fail,
> +static void kill_procs(struct list_head *to_kill, int forcekill,
>  		unsigned long pfn, int flags)
>  {
>  	struct to_kill *tk, *next;
>  
>  	list_for_each_entry_safe(tk, next, to_kill, nd) {
>  		if (forcekill) {
> -			/*
> -			 * In case something went wrong with munmapping
> -			 * make sure the process doesn't catch the
> -			 * signal and then access the memory. Just kill it.
> -			 */
> -			if (fail || tk->addr == -EFAULT) {
> +			if (tk->addr == -EFAULT) {
>  				pr_err("%#lx: forcibly killing %s:%d because of failure to unmap corrupted page\n",
>  				       pfn, tk->tsk->comm, tk->tsk->pid);
>  				do_send_sig_info(SIGKILL, SEND_SIG_PRIV,
> @@ -1666,7 +1661,7 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
>  	 */

There is comment above the forcekill saying:

    When there was a problem unmapping earlier use a more force-full
uncatchable kill to prevent any accesses to the poisoned memory.

This might need to be changed too.
Thanks.
.

>  	forcekill = PageDirty(hpage) || (flags & MF_MUST_KILL) ||
>  		    !unmap_success;
> -	kill_procs(&tokill, forcekill, !unmap_success, pfn, flags);
> +	kill_procs(&tokill, forcekill, pfn, flags);
>  
>  	return unmap_success;
>  }
> @@ -1730,7 +1725,7 @@ static void unmap_and_kill(struct list_head *to_kill, unsigned long pfn,
>  		unmap_mapping_range(mapping, start, size, 0);
>  	}
>  
> -	kill_procs(to_kill, flags & MF_MUST_KILL, false, pfn, flags);
> +	kill_procs(to_kill, flags & MF_MUST_KILL, pfn, flags);
>  }
>  
>  /*
> 

Re: [PATCH 1/3] mm/memory-failure: try to send SIGBUS even if unmap failed

[Jane Chu]

On 5/8/2024 7:54 PM, Miaohe Lin wrote:

> On 2024/5/2 7:24, Jane Chu wrote:
>> For years when it comes down to kill a process due to hwpoison,
>> a SIGBUS is delivered only if unmap has been successful.
>> Otherwise, a SIGKILL is delivered. And the reason for that is
>> to prevent the involved process from accessing the hwpoisoned
>> page again.
>>
>> Since then a lot has changed, a hwpoisoned page is marked and
>> upon being re-accessed, the process will be killed immediately.
>> So let's take out the '!unmap_success' factor and try to deliver
>> SIGBUS if possible.
>>
>> Signed-off-by: Jane Chu <jane.chu@oracle.com>
>> ---
>>   mm/memory-failure.c | 13 ++++---------
>>   1 file changed, 4 insertions(+), 9 deletions(-)
>>
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index 9e62a00b46dd..7fcf182abb96 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -519,19 +519,14 @@ void add_to_kill_ksm(struct task_struct *tsk, struct page *p,
>>    * Also when FAIL is set do a force kill because something went
>>    * wrong earlier.
>>    */
>> -static void kill_procs(struct list_head *to_kill, int forcekill, bool fail,
>> +static void kill_procs(struct list_head *to_kill, int forcekill,
>>   		unsigned long pfn, int flags)
>>   {
>>   	struct to_kill *tk, *next;
>>   
>>   	list_for_each_entry_safe(tk, next, to_kill, nd) {
>>   		if (forcekill) {
>> -			/*
>> -			 * In case something went wrong with munmapping
>> -			 * make sure the process doesn't catch the
>> -			 * signal and then access the memory. Just kill it.
>> -			 */
>> -			if (fail || tk->addr == -EFAULT) {
>> +			if (tk->addr == -EFAULT) {
>>   				pr_err("%#lx: forcibly killing %s:%d because of failure to unmap corrupted page\n",
>>   				       pfn, tk->tsk->comm, tk->tsk->pid);
>>   				do_send_sig_info(SIGKILL, SEND_SIG_PRIV,
>> @@ -1666,7 +1661,7 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
>>   	 */
> There is comment above the forcekill saying:
>
>      When there was a problem unmapping earlier use a more force-full
> uncatchable kill to prevent any accesses to the poisoned memory.
>
> This might need to be changed too.

Yes, will do.

thanks!

-jane

> Thanks.
> .
>
>>   	forcekill = PageDirty(hpage) || (flags & MF_MUST_KILL) ||
>>   		    !unmap_success;
>> -	kill_procs(&tokill, forcekill, !unmap_success, pfn, flags);
>> +	kill_procs(&tokill, forcekill, pfn, flags);
>>   
>>   	return unmap_success;
>>   }
>> @@ -1730,7 +1725,7 @@ static void unmap_and_kill(struct list_head *to_kill, unsigned long pfn,
>>   		unmap_mapping_range(mapping, start, size, 0);
>>   	}
>>   
>> -	kill_procs(to_kill, flags & MF_MUST_KILL, false, pfn, flags);
>> +	kill_procs(to_kill, flags & MF_MUST_KILL, pfn, flags);
>>   }
>>   
>>   /*
>>
>

[PATCH 2/3] mm/madvise: Add MF_ACTION_REQUIRED to madvise(MADV_HWPOISON)

[Jane Chu]

The soft hwpoison injector via madvise(MADV_HWPOISON) operates in
a synchrous way in a sense, the injector is also a process under
test, and should it have the poisoned page mapped in its address
space, it should legitimately get killed as much as in a real UE
situation.

Signed-off-by: Jane Chu <jane.chu@oracle.com>
---
 mm/madvise.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/madvise.c b/mm/madvise.c
index 1a073fcc4c0c..eaeae5252c02 100644
--- a/mm/madvise.c
+++ b/mm/madvise.c
@@ -1127,7 +1127,7 @@ static int madvise_inject_error(int behavior,
 		} else {
 			pr_info("Injecting memory failure for pfn %#lx at process virtual address %#lx\n",
 				 pfn, start);
-			ret = memory_failure(pfn, MF_COUNT_INCREASED | MF_SW_SIMULATED);
+			ret = memory_failure(pfn, MF_ACTION_REQUIRED | MF_COUNT_INCREASED | MF_SW_SIMULATED);
 			if (ret == -EOPNOTSUPP)
 				ret = 0;
 		}
-- 
2.39.3

Re: [PATCH 2/3] mm/madvise: Add MF_ACTION_REQUIRED to madvise(MADV_HWPOISON)

[Miaohe Lin]

On 2024/5/2 7:24, Jane Chu wrote:
> The soft hwpoison injector via madvise(MADV_HWPOISON) operates in
> a synchrous way in a sense, the injector is also a process under
> test, and should it have the poisoned page mapped in its address
> space, it should legitimately get killed as much as in a real UE
> situation.

Will it be better to add a method to set MF_ACTION_REQUIRED explicitly when inject soft hwpoison?
Thanks.
.

> 
> Signed-off-by: Jane Chu <jane.chu@oracle.com>
> ---
>  mm/madvise.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/madvise.c b/mm/madvise.c
> index 1a073fcc4c0c..eaeae5252c02 100644
> --- a/mm/madvise.c
> +++ b/mm/madvise.c
> @@ -1127,7 +1127,7 @@ static int madvise_inject_error(int behavior,
>  		} else {
>  			pr_info("Injecting memory failure for pfn %#lx at process virtual address %#lx\n",
>  				 pfn, start);
> -			ret = memory_failure(pfn, MF_COUNT_INCREASED | MF_SW_SIMULATED);
> +			ret = memory_failure(pfn, MF_ACTION_REQUIRED | MF_COUNT_INCREASED | MF_SW_SIMULATED);
>  			if (ret == -EOPNOTSUPP)
>  				ret = 0;
>  		}
> 

Re: [PATCH 2/3] mm/madvise: Add MF_ACTION_REQUIRED to madvise(MADV_HWPOISON)

[Jane Chu]

On 5/5/2024 12:02 AM, Miaohe Lin wrote:

> On 2024/5/2 7:24, Jane Chu wrote:
>> The soft hwpoison injector via madvise(MADV_HWPOISON) operates in
>> a synchrous way in a sense, the injector is also a process under
>> test, and should it have the poisoned page mapped in its address
>> space, it should legitimately get killed as much as in a real UE
>> situation.
> Will it be better to add a method to set MF_ACTION_REQUIRED explicitly when inject soft hwpoison?
> Thanks.

So the first question is: Is there a need to preserve the existing 
behavior of  madvise(MADV_HWPOISON)?

The madvise(2) man page says -

        *MADV_HWPOISON *(since Linux 2.6.32)
               Poison the pages in the range specified by/addr/  and/length/
               and handle subsequent references to those pages like a
               hardware memory corruption.  This operation is available
               only for privileged (*CAP_SYS_ADMIN*) processes.  This
               operation may result in the calling process receiving a
               *SIGBUS *and the page being unmapped.

               This feature is intended for testing of memory error-
               handling code; it is available only if the kernel was
               configured with*CONFIG_MEMORY_FAILURE*.

And the impression from my reading is that: there doesn't seem to be a need.

A couple observations -
- The man page states that the calling process may receive a SIGBUS and the page being unmapped.
But the existing behavior is no SIGBUS unless MCE early kill is elected, so it doesn't quite match
the man page.
- There is 'hwpoison-inject' which behaves similar to the existing madvise(MADV_HWPOISON), that is,
soft inject without MF_ACTION_REQUIRED flag.

thanks,
-jane

> .
>
>> Signed-off-by: Jane Chu <jane.chu@oracle.com>
>> ---
>>   mm/madvise.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/madvise.c b/mm/madvise.c
>> index 1a073fcc4c0c..eaeae5252c02 100644
>> --- a/mm/madvise.c
>> +++ b/mm/madvise.c
>> @@ -1127,7 +1127,7 @@ static int madvise_inject_error(int behavior,
>>   		} else {
>>   			pr_info("Injecting memory failure for pfn %#lx at process virtual address %#lx\n",
>>   				 pfn, start);
>> -			ret = memory_failure(pfn, MF_COUNT_INCREASED | MF_SW_SIMULATED);
>> +			ret = memory_failure(pfn, MF_ACTION_REQUIRED | MF_COUNT_INCREASED | MF_SW_SIMULATED);
>>   			if (ret == -EOPNOTSUPP)
>>   				ret = 0;
>>   		}
>>

Re: [PATCH 2/3] mm/madvise: Add MF_ACTION_REQUIRED to madvise(MADV_HWPOISON)

[Miaohe Lin]

On 2024/5/7 3:54, Jane Chu wrote:
> On 5/5/2024 12:02 AM, Miaohe Lin wrote:
> 
>> On 2024/5/2 7:24, Jane Chu wrote:
>>> The soft hwpoison injector via madvise(MADV_HWPOISON) operates in
>>> a synchrous way in a sense, the injector is also a process under
>>> test, and should it have the poisoned page mapped in its address
>>> space, it should legitimately get killed as much as in a real UE
>>> situation.
>> Will it be better to add a method to set MF_ACTION_REQUIRED explicitly when inject soft hwpoison?
>> Thanks.
> 
> So the first question is: Is there a need to preserve the existing behavior of  madvise(MADV_HWPOISON)?
> 
> The madvise(2) man page says -
> 
>        *MADV_HWPOISON *(since Linux 2.6.32)
>               Poison the pages in the range specified by/addr/  and/length/
>               and handle subsequent references to those pages like a
>               hardware memory corruption.  This operation is available
>               only for privileged (*CAP_SYS_ADMIN*) processes.  This
>               operation may result in the calling process receiving a
>               *SIGBUS *and the page being unmapped.
> 
>               This feature is intended for testing of memory error-
>               handling code; it is available only if the kernel was
>               configured with*CONFIG_MEMORY_FAILURE*.
> 
> And the impression from my reading is that: there doesn't seem to be a need.
> 
> A couple observations -
> - The man page states that the calling process may receive a SIGBUS and the page being unmapped.
> But the existing behavior is no SIGBUS unless MCE early kill is elected, so it doesn't quite match
> the man page.
> - There is 'hwpoison-inject' which behaves similar to the existing madvise(MADV_HWPOISON), that is,
> soft inject without MF_ACTION_REQUIRED flag.
> 

I tend to agree with you. It might be a good idea to add MF_ACTION_REQUIRED to madvise(MADV_HWPOISON).
Thanks.
.

[PATCH 3/3] mm/memory-failure: send SIGBUS in the event of thp split fail

[Jane Chu]

When handle hwpoison in a GUP longterm pin'ed thp page,
try_to_split_thp_page() will fail. And at this point, there is little else
the kernel could do except sending a SIGBUS to the user process, thus
give it a chance to recover.

Signed-off-by: Jane Chu <jane.chu@oracle.com>
---
 mm/memory-failure.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 7fcf182abb96..67f4d24a98e7 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -2168,6 +2168,37 @@ static int memory_failure_dev_pagemap(unsigned long pfn, int flags,
 	return rc;
 }
 
+/*
+ * The calling condition is as such: thp split failed, page might have
+ * been GUP longterm pinned, not much can be done for recovery.
+ * But a SIGBUS should be delivered with vaddr provided so that the user
+ * application has a chance to recover. Also, application processes'
+ * election for MCE early killed will be honored.
+ */
+static int kill_procs_now(struct page *p, unsigned long pfn, int flags,
+			struct page *hpage)
+{
+	struct folio *folio = page_folio(hpage);
+	LIST_HEAD(tokill);
+	int res = -EHWPOISON;
+
+	/* deal with user pages only */
+	if (PageReserved(p) || PageSlab(p) || PageTable(p) || PageOffline(p))
+		res = -EBUSY;
+	if (!(PageLRU(hpage) || PageHuge(p)))
+		res = -EBUSY;
+
+	if (res == -EHWPOISON) {
+		collect_procs(folio, p, &tokill, flags & MF_ACTION_REQUIRED);
+		kill_procs(&tokill, true, pfn, flags);
+	}
+
+	if (flags & MF_COUNT_INCREASED)
+		put_page(p);
+
+	return res;
+}
+
 /**
  * memory_failure - Handle memory failure of a page.
  * @pfn: Page Number of the corrupted page
@@ -2297,6 +2328,11 @@ int memory_failure(unsigned long pfn, int flags)
 		 */
 		SetPageHasHWPoisoned(hpage);
 		if (try_to_split_thp_page(p) < 0) {
+			if (flags & MF_ACTION_REQUIRED) {
+				pr_err("%#lx: thp split failed\n", pfn);
+				res = kill_procs_now(p, pfn, flags, hpage);
+				goto unlock_mutex;
+			}
 			res = action_result(pfn, MF_MSG_UNSPLIT_THP, MF_IGNORED);
 			goto unlock_mutex;
 		}
-- 
2.39.3

Re: [PATCH 3/3] mm/memory-failure: send SIGBUS in the event of thp split fail

[Miaohe Lin]

On 2024/5/2 7:24, Jane Chu wrote:
> When handle hwpoison in a GUP longterm pin'ed thp page,
> try_to_split_thp_page() will fail. And at this point, there is little else
> the kernel could do except sending a SIGBUS to the user process, thus
> give it a chance to recover.
> 
> Signed-off-by: Jane Chu <jane.chu@oracle.com>

Thanks for your patch. Some comments below.

> ---
>  mm/memory-failure.c | 36 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 7fcf182abb96..67f4d24a98e7 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -2168,6 +2168,37 @@ static int memory_failure_dev_pagemap(unsigned long pfn, int flags,
>  	return rc;
>  }
>  
> +/*
> + * The calling condition is as such: thp split failed, page might have
> + * been GUP longterm pinned, not much can be done for recovery.
> + * But a SIGBUS should be delivered with vaddr provided so that the user
> + * application has a chance to recover. Also, application processes'
> + * election for MCE early killed will be honored.
> + */
> +static int kill_procs_now(struct page *p, unsigned long pfn, int flags,
> +			struct page *hpage)
> +{
> +	struct folio *folio = page_folio(hpage);
> +	LIST_HEAD(tokill);
> +	int res = -EHWPOISON;
> +
> +	/* deal with user pages only */
> +	if (PageReserved(p) || PageSlab(p) || PageTable(p) || PageOffline(p))
> +		res = -EBUSY;
> +	if (!(PageLRU(hpage) || PageHuge(p)))
> +		res = -EBUSY;

Above checks seems unneeded. We already know it's thp?

> +
> +	if (res == -EHWPOISON) {
> +		collect_procs(folio, p, &tokill, flags & MF_ACTION_REQUIRED);
> +		kill_procs(&tokill, true, pfn, flags);
> +	}
> +
> +	if (flags & MF_COUNT_INCREASED)
> +		put_page(p);

This if block is broken. put_page() has been done when try_to_split_thp_page() fails?

> +

action_result is missing?

> +	return res;
> +}
> +
>  /**
>   * memory_failure - Handle memory failure of a page.
>   * @pfn: Page Number of the corrupted page
> @@ -2297,6 +2328,11 @@ int memory_failure(unsigned long pfn, int flags)
>  		 */
>  		SetPageHasHWPoisoned(hpage);
>  		if (try_to_split_thp_page(p) < 0) {

Should hwpoison_filter() be called in this case?

> +			if (flags & MF_ACTION_REQUIRED) {
> +				pr_err("%#lx: thp split failed\n", pfn);
> +				res = kill_procs_now(p, pfn, flags, hpage);

Can we use hwpoison_user_mappings() directly here?

Thanks.
.

> +				goto unlock_mutex;
> +			}
>  			res = action_result(pfn, MF_MSG_UNSPLIT_THP, MF_IGNORED);
>  			goto unlock_mutex;
>  		}
> 

Re: [PATCH 3/3] mm/memory-failure: send SIGBUS in the event of thp split fail

[Jane Chu]

On 5/5/2024 12:00 AM, Miaohe Lin wrote:

> On 2024/5/2 7:24, Jane Chu wrote:
>> When handle hwpoison in a GUP longterm pin'ed thp page,
>> try_to_split_thp_page() will fail. And at this point, there is little else
>> the kernel could do except sending a SIGBUS to the user process, thus
>> give it a chance to recover.
>>
>> Signed-off-by: Jane Chu <jane.chu@oracle.com>
> Thanks for your patch. Some comments below.
>
>> ---
>>   mm/memory-failure.c | 36 ++++++++++++++++++++++++++++++++++++
>>   1 file changed, 36 insertions(+)
>>
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index 7fcf182abb96..67f4d24a98e7 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -2168,6 +2168,37 @@ static int memory_failure_dev_pagemap(unsigned long pfn, int flags,
>>   	return rc;
>>   }
>>   
>> +/*
>> + * The calling condition is as such: thp split failed, page might have
>> + * been GUP longterm pinned, not much can be done for recovery.
>> + * But a SIGBUS should be delivered with vaddr provided so that the user
>> + * application has a chance to recover. Also, application processes'
>> + * election for MCE early killed will be honored.
>> + */
>> +static int kill_procs_now(struct page *p, unsigned long pfn, int flags,
>> +			struct page *hpage)
>> +{
>> +	struct folio *folio = page_folio(hpage);
>> +	LIST_HEAD(tokill);
>> +	int res = -EHWPOISON;
>> +
>> +	/* deal with user pages only */
>> +	if (PageReserved(p) || PageSlab(p) || PageTable(p) || PageOffline(p))
>> +		res = -EBUSY;
>> +	if (!(PageLRU(hpage) || PageHuge(p)))
>> +		res = -EBUSY;
> Above checks seems unneeded. We already know it's thp?

Agreed.

I  lifted these checks from hwpoison_user_mapping() with a hope to make 
kill_procs_now() more generic,

such as, potentially replacing kill_accessing_processes() for 
re-accessing hwpoisoned page.

But I backed out at last, due to concerns that my tests might not have 
covered sufficient number of scenarios.

>
>> +
>> +	if (res == -EHWPOISON) {
>> +		collect_procs(folio, p, &tokill, flags & MF_ACTION_REQUIRED);
>> +		kill_procs(&tokill, true, pfn, flags);
>> +	}
>> +
>> +	if (flags & MF_COUNT_INCREASED)
>> +		put_page(p);
> This if block is broken. put_page() has been done when try_to_split_thp_page() fails?

put_page() has not been done if try_to_split_thp_page() fails, and I 
think it should.

I will revise the code so that put_page() is called regardless 
MF_ACTION_REQUIRED is set or not.

>
>> +
> action_result is missing?

Indeed,  action_result() isn't always called, referring to the 
re-accessing hwpoison scenarios.

In this case, I think the reason  is that, we just killed the process 
and there is nothing

else to do or to report.

>
>> +	return res;
>> +}
>> +
>>   /**
>>    * memory_failure - Handle memory failure of a page.
>>    * @pfn: Page Number of the corrupted page
>> @@ -2297,6 +2328,11 @@ int memory_failure(unsigned long pfn, int flags)
>>   		 */
>>   		SetPageHasHWPoisoned(hpage);
>>   		if (try_to_split_thp_page(p) < 0) {
> Should hwpoison_filter() be called in this case?
Yes, it should. I will add the hwpoison_filter check.
>
>> +			if (flags & MF_ACTION_REQUIRED) {
>> +				pr_err("%#lx: thp split failed\n", pfn);
>> +				res = kill_procs_now(p, pfn, flags, hpage);
> Can we use hwpoison_user_mappings() directly here?

I thought about using hwpoison_user_mappings() with an extra flag, but 
gave up in the end.

Because most of the code there are not needed, such as the checks you 
mentioned above,

and umapping etc.

thanks!

-jane

>
> Thanks.
> .
>
>> +				goto unlock_mutex;
>> +			}
>>   			res = action_result(pfn, MF_MSG_UNSPLIT_THP, MF_IGNORED);
>>   			goto unlock_mutex;
>>   		}
>>

Re: [PATCH 3/3] mm/memory-failure: send SIGBUS in the event of thp split fail

[Miaohe Lin]

On 2024/5/7 4:26, Jane Chu wrote:
> On 5/5/2024 12:00 AM, Miaohe Lin wrote:
> 
>> On 2024/5/2 7:24, Jane Chu wrote:
>>> When handle hwpoison in a GUP longterm pin'ed thp page,
>>> try_to_split_thp_page() will fail. And at this point, there is little else
>>> the kernel could do except sending a SIGBUS to the user process, thus
>>> give it a chance to recover.
>>>
>>> Signed-off-by: Jane Chu <jane.chu@oracle.com>
>> Thanks for your patch. Some comments below.
>>
>>> ---
>>>   mm/memory-failure.c | 36 ++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 36 insertions(+)
>>>
>>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>>> index 7fcf182abb96..67f4d24a98e7 100644
>>> --- a/mm/memory-failure.c
>>> +++ b/mm/memory-failure.c
>>> @@ -2168,6 +2168,37 @@ static int memory_failure_dev_pagemap(unsigned long pfn, int flags,
>>>       return rc;
>>>   }
>>>   +/*
>>> + * The calling condition is as such: thp split failed, page might have
>>> + * been GUP longterm pinned, not much can be done for recovery.
>>> + * But a SIGBUS should be delivered with vaddr provided so that the user
>>> + * application has a chance to recover. Also, application processes'
>>> + * election for MCE early killed will be honored.
>>> + */
>>> +static int kill_procs_now(struct page *p, unsigned long pfn, int flags,
>>> +            struct page *hpage)
>>> +{
>>> +    struct folio *folio = page_folio(hpage);
>>> +    LIST_HEAD(tokill);
>>> +    int res = -EHWPOISON;
>>> +
>>> +    /* deal with user pages only */
>>> +    if (PageReserved(p) || PageSlab(p) || PageTable(p) || PageOffline(p))
>>> +        res = -EBUSY;
>>> +    if (!(PageLRU(hpage) || PageHuge(p)))
>>> +        res = -EBUSY;
>> Above checks seems unneeded. We already know it's thp?
> 
> Agreed.
> 
> I  lifted these checks from hwpoison_user_mapping() with a hope to make kill_procs_now() more generic,
> 
> such as, potentially replacing kill_accessing_processes() for re-accessing hwpoisoned page.
> 
> But I backed out at last, due to concerns that my tests might not have covered sufficient number of scenarios.
> 
>>
>>> +
>>> +    if (res == -EHWPOISON) {
>>> +        collect_procs(folio, p, &tokill, flags & MF_ACTION_REQUIRED);
>>> +        kill_procs(&tokill, true, pfn, flags);
>>> +    }
>>> +
>>> +    if (flags & MF_COUNT_INCREASED)
>>> +        put_page(p);
>> This if block is broken. put_page() has been done when try_to_split_thp_page() fails?
> 
> put_page() has not been done if try_to_split_thp_page() fails, and I think it should.

In try_to_split_thp_page(), if split_huge_page fails, i.e. ret != 0, put_page() is called. See below:

static int try_to_split_thp_page(struct page *page)
{
	int ret;

	lock_page(page);
	ret = split_huge_page(page);
	unlock_page(page);

	if (unlikely(ret))
		put_page(page);
	^^^^^^^^^^^^^^^^^^^^^^^
	return ret;
}

Or am I miss something?

> 
> I will revise the code so that put_page() is called regardless MF_ACTION_REQUIRED is set or not.
> 
>>
>>> +
>> action_result is missing?
> 
> Indeed,  action_result() isn't always called, referring to the re-accessing hwpoison scenarios.
> 
> In this case, I think the reason  is that, we just killed the process and there is nothing
> 
> else to do or to report.
> 
>>
>>> +    return res;
>>> +}
>>> +
>>>   /**
>>>    * memory_failure - Handle memory failure of a page.
>>>    * @pfn: Page Number of the corrupted page
>>> @@ -2297,6 +2328,11 @@ int memory_failure(unsigned long pfn, int flags)
>>>            */
>>>           SetPageHasHWPoisoned(hpage);
>>>           if (try_to_split_thp_page(p) < 0) {
>> Should hwpoison_filter() be called in this case?
> Yes, it should. I will add the hwpoison_filter check.
>>
>>> +            if (flags & MF_ACTION_REQUIRED) {

Only in MF_ACTION_REQUIRED case, SIGBUS is sent to processes when thp split failed. Any reson under it?

Thanks.
.

Re: [PATCH 3/3] mm/memory-failure: send SIGBUS in the event of thp split fail

[Jane Chu]

On 5/8/2024 1:08 AM, Miaohe Lin wrote:

> On 2024/5/7 4:26, Jane Chu wrote:
>> On 5/5/2024 12:00 AM, Miaohe Lin wrote:
>>
>>> On 2024/5/2 7:24, Jane Chu wrote:
>>>> When handle hwpoison in a GUP longterm pin'ed thp page,
>>>> try_to_split_thp_page() will fail. And at this point, there is little else
>>>> the kernel could do except sending a SIGBUS to the user process, thus
>>>> give it a chance to recover.
>>>>
>>>> Signed-off-by: Jane Chu <jane.chu@oracle.com>
>>> Thanks for your patch. Some comments below.
>>>
>>>> ---
>>>>    mm/memory-failure.c | 36 ++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 36 insertions(+)
>>>>
>>>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>>>> index 7fcf182abb96..67f4d24a98e7 100644
>>>> --- a/mm/memory-failure.c
>>>> +++ b/mm/memory-failure.c
>>>> @@ -2168,6 +2168,37 @@ static int memory_failure_dev_pagemap(unsigned long pfn, int flags,
>>>>        return rc;
>>>>    }
>>>>    +/*
>>>> + * The calling condition is as such: thp split failed, page might have
>>>> + * been GUP longterm pinned, not much can be done for recovery.
>>>> + * But a SIGBUS should be delivered with vaddr provided so that the user
>>>> + * application has a chance to recover. Also, application processes'
>>>> + * election for MCE early killed will be honored.
>>>> + */
>>>> +static int kill_procs_now(struct page *p, unsigned long pfn, int flags,
>>>> +            struct page *hpage)
>>>> +{
>>>> +    struct folio *folio = page_folio(hpage);
>>>> +    LIST_HEAD(tokill);
>>>> +    int res = -EHWPOISON;
>>>> +
>>>> +    /* deal with user pages only */
>>>> +    if (PageReserved(p) || PageSlab(p) || PageTable(p) || PageOffline(p))
>>>> +        res = -EBUSY;
>>>> +    if (!(PageLRU(hpage) || PageHuge(p)))
>>>> +        res = -EBUSY;
>>> Above checks seems unneeded. We already know it's thp?
>> Agreed.
>>
>> I  lifted these checks from hwpoison_user_mapping() with a hope to make kill_procs_now() more generic,
>>
>> such as, potentially replacing kill_accessing_processes() for re-accessing hwpoisoned page.
>>
>> But I backed out at last, due to concerns that my tests might not have covered sufficient number of scenarios.
>>
>>>> +
>>>> +    if (res == -EHWPOISON) {
>>>> +        collect_procs(folio, p, &tokill, flags & MF_ACTION_REQUIRED);
>>>> +        kill_procs(&tokill, true, pfn, flags);
>>>> +    }
>>>> +
>>>> +    if (flags & MF_COUNT_INCREASED)
>>>> +        put_page(p);
>>> This if block is broken. put_page() has been done when try_to_split_thp_page() fails?
>> put_page() has not been done if try_to_split_thp_page() fails, and I think it should.
> In try_to_split_thp_page(), if split_huge_page fails, i.e. ret != 0, put_page() is called. See below:
>
> static int try_to_split_thp_page(struct page *page)
> {
> 	int ret;
>
> 	lock_page(page);
> 	ret = split_huge_page(page);
> 	unlock_page(page);
>
> 	if (unlikely(ret))
> 		put_page(page);
> 	^^^^^^^^^^^^^^^^^^^^^^^
> 	return ret;
> }
>
> Or am I miss something?

I think you caught a bug in my code, thanks!

How about moving put_page() outside try_to_split_thp_page() ?

>
>> I will revise the code so that put_page() is called regardless MF_ACTION_REQUIRED is set or not.
>>
>>>> +
>>> action_result is missing?
>> Indeed,  action_result() isn't always called, referring to the re-accessing hwpoison scenarios.
>>
>> In this case, I think the reason  is that, we just killed the process and there is nothing
>>
>> else to do or to report.
>>
>>>> +    return res;
>>>> +}
>>>> +
>>>>    /**
>>>>     * memory_failure - Handle memory failure of a page.
>>>>     * @pfn: Page Number of the corrupted page
>>>> @@ -2297,6 +2328,11 @@ int memory_failure(unsigned long pfn, int flags)
>>>>             */
>>>>            SetPageHasHWPoisoned(hpage);
>>>>            if (try_to_split_thp_page(p) < 0) {
>>> Should hwpoison_filter() be called in this case?
>> Yes, it should. I will add the hwpoison_filter check.
>>>> +            if (flags & MF_ACTION_REQUIRED) {
> Only in MF_ACTION_REQUIRED case, SIGBUS is sent to processes when thp split failed. Any reson under it?

I took a clue from kill_accessing_process() which is invoked only if 
MF_ACTION_REQUIRED is set.

The usual code path for delivery signal is

if page-is-dirty or MF_MUST_KILL-is-set or umap-failed, then

- send SIGKILL if vaddr is -EFAULT

- send SIGBUS with BUS_MCEERR_AR if MF_ACTION_REQUIRED is set

- send SIGBUS with BUS_MCEERR_AO if MF_ACTION_REQUIRED is not set and 
process elected for MCE-early-kill

So, if kill_procs_now() is invoked only if MF_ACTION_REQUIRED (as it is 
in the patch), one can argue that

the MCE-early-kill request is not honored which deviates from the 
existing behavior.

Perhaps I should remove the

+ if (flags & MF_ACTION_REQUIRED) {

check.

thanks!

-jane



>
> Thanks.
> .

Re: [PATCH 3/3] mm/memory-failure: send SIGBUS in the event of thp split fail

[Miaohe Lin]

On 2024/5/9 1:45, Jane Chu wrote:
> On 5/8/2024 1:08 AM, Miaohe Lin wrote:
> 
>> On 2024/5/7 4:26, Jane Chu wrote:
>>> On 5/5/2024 12:00 AM, Miaohe Lin wrote:
>>>
>>>> On 2024/5/2 7:24, Jane Chu wrote:
>>>>> When handle hwpoison in a GUP longterm pin'ed thp page,
>>>>> try_to_split_thp_page() will fail. And at this point, there is little else
>>>>> the kernel could do except sending a SIGBUS to the user process, thus
>>>>> give it a chance to recover.
>>>>>
>>>>> Signed-off-by: Jane Chu <jane.chu@oracle.com>
>>>> Thanks for your patch. Some comments below.
>>>>
>>>>> ---
>>>>>    mm/memory-failure.c | 36 ++++++++++++++++++++++++++++++++++++
>>>>>    1 file changed, 36 insertions(+)
>>>>>
>>>>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>>>>> index 7fcf182abb96..67f4d24a98e7 100644
>>>>> --- a/mm/memory-failure.c
>>>>> +++ b/mm/memory-failure.c
>>>>> @@ -2168,6 +2168,37 @@ static int memory_failure_dev_pagemap(unsigned long pfn, int flags,
>>>>>        return rc;
>>>>>    }
>>>>>    +/*
>>>>> + * The calling condition is as such: thp split failed, page might have
>>>>> + * been GUP longterm pinned, not much can be done for recovery.
>>>>> + * But a SIGBUS should be delivered with vaddr provided so that the user
>>>>> + * application has a chance to recover. Also, application processes'
>>>>> + * election for MCE early killed will be honored.
>>>>> + */
>>>>> +static int kill_procs_now(struct page *p, unsigned long pfn, int flags,
>>>>> +            struct page *hpage)
>>>>> +{
>>>>> +    struct folio *folio = page_folio(hpage);
>>>>> +    LIST_HEAD(tokill);
>>>>> +    int res = -EHWPOISON;
>>>>> +
>>>>> +    /* deal with user pages only */
>>>>> +    if (PageReserved(p) || PageSlab(p) || PageTable(p) || PageOffline(p))
>>>>> +        res = -EBUSY;
>>>>> +    if (!(PageLRU(hpage) || PageHuge(p)))
>>>>> +        res = -EBUSY;
>>>> Above checks seems unneeded. We already know it's thp?
>>> Agreed.
>>>
>>> I  lifted these checks from hwpoison_user_mapping() with a hope to make kill_procs_now() more generic,
>>>
>>> such as, potentially replacing kill_accessing_processes() for re-accessing hwpoisoned page.
>>>
>>> But I backed out at last, due to concerns that my tests might not have covered sufficient number of scenarios.
>>>
>>>>> +
>>>>> +    if (res == -EHWPOISON) {
>>>>> +        collect_procs(folio, p, &tokill, flags & MF_ACTION_REQUIRED);
>>>>> +        kill_procs(&tokill, true, pfn, flags);
>>>>> +    }
>>>>> +
>>>>> +    if (flags & MF_COUNT_INCREASED)
>>>>> +        put_page(p);
>>>> This if block is broken. put_page() has been done when try_to_split_thp_page() fails?
>>> put_page() has not been done if try_to_split_thp_page() fails, and I think it should.
>> In try_to_split_thp_page(), if split_huge_page fails, i.e. ret != 0, put_page() is called. See below:
>>
>> static int try_to_split_thp_page(struct page *page)
>> {
>>     int ret;
>>
>>     lock_page(page);
>>     ret = split_huge_page(page);
>>     unlock_page(page);
>>
>>     if (unlikely(ret))
>>         put_page(page);
>>     ^^^^^^^^^^^^^^^^^^^^^^^
>>     return ret;
>> }
>>
>> Or am I miss something?
> 
> I think you caught a bug in my code, thanks!
> 
> How about moving put_page() outside try_to_split_thp_page() ?

If you want to send SIGBUS in the event of thp split fail, it might be required to do so.
I think kill_procs_now() needs extra thp refcnt to do its work.

> 
>>
>>> I will revise the code so that put_page() is called regardless MF_ACTION_REQUIRED is set or not.
>>>
>>>>> +
>>>> action_result is missing?
>>> Indeed,  action_result() isn't always called, referring to the re-accessing hwpoison scenarios.
>>>
>>> In this case, I think the reason  is that, we just killed the process and there is nothing
>>>
>>> else to do or to report.
>>>
>>>>> +    return res;
>>>>> +}
>>>>> +
>>>>>    /**
>>>>>     * memory_failure - Handle memory failure of a page.
>>>>>     * @pfn: Page Number of the corrupted page
>>>>> @@ -2297,6 +2328,11 @@ int memory_failure(unsigned long pfn, int flags)
>>>>>             */
>>>>>            SetPageHasHWPoisoned(hpage);
>>>>>            if (try_to_split_thp_page(p) < 0) {
>>>> Should hwpoison_filter() be called in this case?
>>> Yes, it should. I will add the hwpoison_filter check.
>>>>> +            if (flags & MF_ACTION_REQUIRED) {
>> Only in MF_ACTION_REQUIRED case, SIGBUS is sent to processes when thp split failed. Any reson under it?
> 
> I took a clue from kill_accessing_process() which is invoked only if MF_ACTION_REQUIRED is set.
> 
> The usual code path for delivery signal is
> 
> if page-is-dirty or MF_MUST_KILL-is-set or umap-failed, then
> 
> - send SIGKILL if vaddr is -EFAULT
> 
> - send SIGBUS with BUS_MCEERR_AR if MF_ACTION_REQUIRED is set
> 
> - send SIGBUS with BUS_MCEERR_AO if MF_ACTION_REQUIRED is not set and process elected for MCE-early-kill
> 
> So, if kill_procs_now() is invoked only if MF_ACTION_REQUIRED (as it is in the patch), one can argue that
> 
> the MCE-early-kill request is not honored which deviates from the existing behavior.
> 
> Perhaps I should remove the
> 
> + if (flags & MF_ACTION_REQUIRED) {

I tend to agree MCE-early-kill request should be honored when try to kill process.
Thanks.
.

Re: [PATCH 3/3] mm/memory-failure: send SIGBUS in the event of thp split fail

[Jane Chu]

On 5/9/2024 1:30 AM, Miaohe Lin wrote:
> On 2024/5/9 1:45, Jane Chu wrote:
>> On 5/8/2024 1:08 AM, Miaohe Lin wrote:
>>
>>> On 2024/5/7 4:26, Jane Chu wrote:
>>>> On 5/5/2024 12:00 AM, Miaohe Lin wrote:
>>>>
>>>>> On 2024/5/2 7:24, Jane Chu wrote:
>>>>>> When handle hwpoison in a GUP longterm pin'ed thp page,
>>>>>> try_to_split_thp_page() will fail. And at this point, there is little else
>>>>>> the kernel could do except sending a SIGBUS to the user process, thus
>>>>>> give it a chance to recover.
>>>>>>
>>>>>> Signed-off-by: Jane Chu <jane.chu@oracle.com>
>>>>> Thanks for your patch. Some comments below.
>>>>>
>>>>>> ---
>>>>>>     mm/memory-failure.c | 36 ++++++++++++++++++++++++++++++++++++
>>>>>>     1 file changed, 36 insertions(+)
>>>>>>
>>>>>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>>>>>> index 7fcf182abb96..67f4d24a98e7 100644
>>>>>> --- a/mm/memory-failure.c
>>>>>> +++ b/mm/memory-failure.c
>>>>>> @@ -2168,6 +2168,37 @@ static int memory_failure_dev_pagemap(unsigned long pfn, int flags,
>>>>>>         return rc;
>>>>>>     }
>>>>>>     +/*
>>>>>> + * The calling condition is as such: thp split failed, page might have
>>>>>> + * been GUP longterm pinned, not much can be done for recovery.
>>>>>> + * But a SIGBUS should be delivered with vaddr provided so that the user
>>>>>> + * application has a chance to recover. Also, application processes'
>>>>>> + * election for MCE early killed will be honored.
>>>>>> + */
>>>>>> +static int kill_procs_now(struct page *p, unsigned long pfn, int flags,
>>>>>> +            struct page *hpage)
>>>>>> +{
>>>>>> +    struct folio *folio = page_folio(hpage);
>>>>>> +    LIST_HEAD(tokill);
>>>>>> +    int res = -EHWPOISON;
>>>>>> +
>>>>>> +    /* deal with user pages only */
>>>>>> +    if (PageReserved(p) || PageSlab(p) || PageTable(p) || PageOffline(p))
>>>>>> +        res = -EBUSY;
>>>>>> +    if (!(PageLRU(hpage) || PageHuge(p)))
>>>>>> +        res = -EBUSY;
>>>>> Above checks seems unneeded. We already know it's thp?
>>>> Agreed.
>>>>
>>>> I  lifted these checks from hwpoison_user_mapping() with a hope to make kill_procs_now() more generic,
>>>>
>>>> such as, potentially replacing kill_accessing_processes() for re-accessing hwpoisoned page.
>>>>
>>>> But I backed out at last, due to concerns that my tests might not have covered sufficient number of scenarios.
>>>>
>>>>>> +
>>>>>> +    if (res == -EHWPOISON) {
>>>>>> +        collect_procs(folio, p, &tokill, flags & MF_ACTION_REQUIRED);
>>>>>> +        kill_procs(&tokill, true, pfn, flags);
>>>>>> +    }
>>>>>> +
>>>>>> +    if (flags & MF_COUNT_INCREASED)
>>>>>> +        put_page(p);
>>>>> This if block is broken. put_page() has been done when try_to_split_thp_page() fails?
>>>> put_page() has not been done if try_to_split_thp_page() fails, and I think it should.
>>> In try_to_split_thp_page(), if split_huge_page fails, i.e. ret != 0, put_page() is called. See below:
>>>
>>> static int try_to_split_thp_page(struct page *page)
>>> {
>>>      int ret;
>>>
>>>      lock_page(page);
>>>      ret = split_huge_page(page);
>>>      unlock_page(page);
>>>
>>>      if (unlikely(ret))
>>>          put_page(page);
>>>      ^^^^^^^^^^^^^^^^^^^^^^^
>>>      return ret;
>>> }
>>>
>>> Or am I miss something?
>> I think you caught a bug in my code, thanks!
>>
>> How about moving put_page() outside try_to_split_thp_page() ?
> If you want to send SIGBUS in the event of thp split fail, it might be required to do so.
> I think kill_procs_now() needs extra thp refcnt to do its work.

Agreed.  I added an boolean to try_to_split_thp_page(),the boolean 
indicates whether to put_page().

In case of kill_procs_now(), put_page() is called afterwards.

>
>>>> I will revise the code so that put_page() is called regardless MF_ACTION_REQUIRED is set or not.
>>>>
>>>>>> +
>>>>> action_result is missing?
>>>> Indeed,  action_result() isn't always called, referring to the re-accessing hwpoison scenarios.
>>>>
>>>> In this case, I think the reason  is that, we just killed the process and there is nothing
>>>>
>>>> else to do or to report.
>>>>
>>>>>> +    return res;
>>>>>> +}
>>>>>> +
>>>>>>     /**
>>>>>>      * memory_failure - Handle memory failure of a page.
>>>>>>      * @pfn: Page Number of the corrupted page
>>>>>> @@ -2297,6 +2328,11 @@ int memory_failure(unsigned long pfn, int flags)
>>>>>>              */
>>>>>>             SetPageHasHWPoisoned(hpage);
>>>>>>             if (try_to_split_thp_page(p) < 0) {
>>>>> Should hwpoison_filter() be called in this case?
>>>> Yes, it should. I will add the hwpoison_filter check.
>>>>>> +            if (flags & MF_ACTION_REQUIRED) {
>>> Only in MF_ACTION_REQUIRED case, SIGBUS is sent to processes when thp split failed. Any reson under it?
>> I took a clue from kill_accessing_process() which is invoked only if MF_ACTION_REQUIRED is set.
>>
>> The usual code path for delivery signal is
>>
>> if page-is-dirty or MF_MUST_KILL-is-set or umap-failed, then
>>
>> - send SIGKILL if vaddr is -EFAULT
>>
>> - send SIGBUS with BUS_MCEERR_AR if MF_ACTION_REQUIRED is set
>>
>> - send SIGBUS with BUS_MCEERR_AO if MF_ACTION_REQUIRED is not set and process elected for MCE-early-kill
>>
>> So, if kill_procs_now() is invoked only if MF_ACTION_REQUIRED (as it is in the patch), one can argue that
>>
>> the MCE-early-kill request is not honored which deviates from the existing behavior.
>>
>> Perhaps I should remove the
>>
>> + if (flags & MF_ACTION_REQUIRED) {
> I tend to agree MCE-early-kill request should be honored when try to kill process.
> Thanks.
> .

Thanks,

-jane

>

Re: [PATCH 3/3] mm/memory-failure: send SIGBUS in the event of thp split fail

[Miaohe Lin]

On 2024/5/9 23:34, Jane Chu wrote:
> 
> On 5/9/2024 1:30 AM, Miaohe Lin wrote:
>> On 2024/5/9 1:45, Jane Chu wrote:
>>> On 5/8/2024 1:08 AM, Miaohe Lin wrote:
>>>
>>>> On 2024/5/7 4:26, Jane Chu wrote:
>>>>> On 5/5/2024 12:00 AM, Miaohe Lin wrote:
>>>>>
>>>>>> On 2024/5/2 7:24, Jane Chu wrote:
>>>>>>> When handle hwpoison in a GUP longterm pin'ed thp page,
>>>>>>> try_to_split_thp_page() will fail. And at this point, there is little else
>>>>>>> the kernel could do except sending a SIGBUS to the user process, thus
>>>>>>> give it a chance to recover.
>>>>>>>
>>>>>>> Signed-off-by: Jane Chu <jane.chu@oracle.com>
>>>>>> Thanks for your patch. Some comments below.
>>>>>>
>>>>>>> ---
>>>>>>>     mm/memory-failure.c | 36 ++++++++++++++++++++++++++++++++++++
>>>>>>>     1 file changed, 36 insertions(+)
>>>>>>>
>>>>>>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>>>>>>> index 7fcf182abb96..67f4d24a98e7 100644
>>>>>>> --- a/mm/memory-failure.c
>>>>>>> +++ b/mm/memory-failure.c
>>>>>>> @@ -2168,6 +2168,37 @@ static int memory_failure_dev_pagemap(unsigned long pfn, int flags,
>>>>>>>         return rc;
>>>>>>>     }
>>>>>>>     +/*
>>>>>>> + * The calling condition is as such: thp split failed, page might have
>>>>>>> + * been GUP longterm pinned, not much can be done for recovery.
>>>>>>> + * But a SIGBUS should be delivered with vaddr provided so that the user
>>>>>>> + * application has a chance to recover. Also, application processes'
>>>>>>> + * election for MCE early killed will be honored.
>>>>>>> + */
>>>>>>> +static int kill_procs_now(struct page *p, unsigned long pfn, int flags,
>>>>>>> +            struct page *hpage)
>>>>>>> +{
>>>>>>> +    struct folio *folio = page_folio(hpage);
>>>>>>> +    LIST_HEAD(tokill);
>>>>>>> +    int res = -EHWPOISON;
>>>>>>> +
>>>>>>> +    /* deal with user pages only */
>>>>>>> +    if (PageReserved(p) || PageSlab(p) || PageTable(p) || PageOffline(p))
>>>>>>> +        res = -EBUSY;
>>>>>>> +    if (!(PageLRU(hpage) || PageHuge(p)))
>>>>>>> +        res = -EBUSY;
>>>>>> Above checks seems unneeded. We already know it's thp?
>>>>> Agreed.
>>>>>
>>>>> I  lifted these checks from hwpoison_user_mapping() with a hope to make kill_procs_now() more generic,
>>>>>
>>>>> such as, potentially replacing kill_accessing_processes() for re-accessing hwpoisoned page.
>>>>>
>>>>> But I backed out at last, due to concerns that my tests might not have covered sufficient number of scenarios.
>>>>>
>>>>>>> +
>>>>>>> +    if (res == -EHWPOISON) {
>>>>>>> +        collect_procs(folio, p, &tokill, flags & MF_ACTION_REQUIRED);
>>>>>>> +        kill_procs(&tokill, true, pfn, flags);
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    if (flags & MF_COUNT_INCREASED)
>>>>>>> +        put_page(p);
>>>>>> This if block is broken. put_page() has been done when try_to_split_thp_page() fails?
>>>>> put_page() has not been done if try_to_split_thp_page() fails, and I think it should.
>>>> In try_to_split_thp_page(), if split_huge_page fails, i.e. ret != 0, put_page() is called. See below:
>>>>
>>>> static int try_to_split_thp_page(struct page *page)
>>>> {
>>>>      int ret;
>>>>
>>>>      lock_page(page);
>>>>      ret = split_huge_page(page);
>>>>      unlock_page(page);
>>>>
>>>>      if (unlikely(ret))
>>>>          put_page(page);
>>>>      ^^^^^^^^^^^^^^^^^^^^^^^
>>>>      return ret;
>>>> }
>>>>
>>>> Or am I miss something?
>>> I think you caught a bug in my code, thanks!
>>>
>>> How about moving put_page() outside try_to_split_thp_page() ?
>> If you want to send SIGBUS in the event of thp split fail, it might be required to do so.
>> I think kill_procs_now() needs extra thp refcnt to do its work.
> 
> Agreed.  I added an boolean to try_to_split_thp_page(),the boolean indicates whether to put_page().

IMHO, it might be too complicated to add an extra boolean to indicate whether to put_page(). It might be
more straightforward to always put_page outside try_to_split_thp_page?
Thanks.
.

Re: [PATCH 3/3] mm/memory-failure: send SIGBUS in the event of thp split fail

[Jane Chu]

On 5/9/2024 7:59 PM, Miaohe Lin wrote:
> On 2024/5/9 23:34, Jane Chu wrote:
>> On 5/9/2024 1:30 AM, Miaohe Lin wrote:
>>> On 2024/5/9 1:45, Jane Chu wrote:
>>>> On 5/8/2024 1:08 AM, Miaohe Lin wrote:
>>>>
>>>>> On 2024/5/7 4:26, Jane Chu wrote:
>>>>>> On 5/5/2024 12:00 AM, Miaohe Lin wrote:
>>>>>>
>>>>>>> On 2024/5/2 7:24, Jane Chu wrote:
>>>>>>>> When handle hwpoison in a GUP longterm pin'ed thp page,
>>>>>>>> try_to_split_thp_page() will fail. And at this point, there is little else
>>>>>>>> the kernel could do except sending a SIGBUS to the user process, thus
>>>>>>>> give it a chance to recover.
>>>>>>>>
>>>>>>>> Signed-off-by: Jane Chu <jane.chu@oracle.com>
>>>>>>> Thanks for your patch. Some comments below.
>>>>>>>
>>>>>>>> ---
>>>>>>>>      mm/memory-failure.c | 36 ++++++++++++++++++++++++++++++++++++
>>>>>>>>      1 file changed, 36 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>>>>>>>> index 7fcf182abb96..67f4d24a98e7 100644
>>>>>>>> --- a/mm/memory-failure.c
>>>>>>>> +++ b/mm/memory-failure.c
>>>>>>>> @@ -2168,6 +2168,37 @@ static int memory_failure_dev_pagemap(unsigned long pfn, int flags,
>>>>>>>>          return rc;
>>>>>>>>      }
>>>>>>>>      +/*
>>>>>>>> + * The calling condition is as such: thp split failed, page might have
>>>>>>>> + * been GUP longterm pinned, not much can be done for recovery.
>>>>>>>> + * But a SIGBUS should be delivered with vaddr provided so that the user
>>>>>>>> + * application has a chance to recover. Also, application processes'
>>>>>>>> + * election for MCE early killed will be honored.
>>>>>>>> + */
>>>>>>>> +static int kill_procs_now(struct page *p, unsigned long pfn, int flags,
>>>>>>>> +            struct page *hpage)
>>>>>>>> +{
>>>>>>>> +    struct folio *folio = page_folio(hpage);
>>>>>>>> +    LIST_HEAD(tokill);
>>>>>>>> +    int res = -EHWPOISON;
>>>>>>>> +
>>>>>>>> +    /* deal with user pages only */
>>>>>>>> +    if (PageReserved(p) || PageSlab(p) || PageTable(p) || PageOffline(p))
>>>>>>>> +        res = -EBUSY;
>>>>>>>> +    if (!(PageLRU(hpage) || PageHuge(p)))
>>>>>>>> +        res = -EBUSY;
>>>>>>> Above checks seems unneeded. We already know it's thp?
>>>>>> Agreed.
>>>>>>
>>>>>> I  lifted these checks from hwpoison_user_mapping() with a hope to make kill_procs_now() more generic,
>>>>>>
>>>>>> such as, potentially replacing kill_accessing_processes() for re-accessing hwpoisoned page.
>>>>>>
>>>>>> But I backed out at last, due to concerns that my tests might not have covered sufficient number of scenarios.
>>>>>>
>>>>>>>> +
>>>>>>>> +    if (res == -EHWPOISON) {
>>>>>>>> +        collect_procs(folio, p, &tokill, flags & MF_ACTION_REQUIRED);
>>>>>>>> +        kill_procs(&tokill, true, pfn, flags);
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    if (flags & MF_COUNT_INCREASED)
>>>>>>>> +        put_page(p);
>>>>>>> This if block is broken. put_page() has been done when try_to_split_thp_page() fails?
>>>>>> put_page() has not been done if try_to_split_thp_page() fails, and I think it should.
>>>>> In try_to_split_thp_page(), if split_huge_page fails, i.e. ret != 0, put_page() is called. See below:
>>>>>
>>>>> static int try_to_split_thp_page(struct page *page)
>>>>> {
>>>>>       int ret;
>>>>>
>>>>>       lock_page(page);
>>>>>       ret = split_huge_page(page);
>>>>>       unlock_page(page);
>>>>>
>>>>>       if (unlikely(ret))
>>>>>           put_page(page);
>>>>>       ^^^^^^^^^^^^^^^^^^^^^^^
>>>>>       return ret;
>>>>> }
>>>>>
>>>>> Or am I miss something?
>>>> I think you caught a bug in my code, thanks!
>>>>
>>>> How about moving put_page() outside try_to_split_thp_page() ?
>>> If you want to send SIGBUS in the event of thp split fail, it might be required to do so.
>>> I think kill_procs_now() needs extra thp refcnt to do its work.
>> Agreed.  I added an boolean to try_to_split_thp_page(),the boolean indicates whether to put_page().
> IMHO, it might be too complicated to add an extra boolean to indicate whether to put_page(). It might be
> more straightforward to always put_page outside try_to_split_thp_page?

Looks okay to me, let's see.  Will send out v2 in a while.

thanks,

-jane

> Thanks.
> .
>

Re: [PATCH 3/3] mm/memory-failure: send SIGBUS in the event of thp split fail

[Miaohe Lin]

On 2024/5/2 7:24, Jane Chu wrote:
> When handle hwpoison in a GUP longterm pin'ed thp page,
> try_to_split_thp_page() will fail. And at this point, there is little else
> the kernel could do except sending a SIGBUS to the user process, thus
> give it a chance to recover.

It seems the user process will still receive SIGBUS via kill_accessing_process()
when (re-)access thp later. So they should have a chance to recover already.
Or am I miss something?

Thanks.
.

Re: [PATCH 3/3] mm/memory-failure: send SIGBUS in the event of thp split fail

[Jane Chu]

On 5/8/2024 2:03 AM, Miaohe Lin wrote:

> On 2024/5/2 7:24, Jane Chu wrote:
>> When handle hwpoison in a GUP longterm pin'ed thp page,
>> try_to_split_thp_page() will fail. And at this point, there is little else
>> the kernel could do except sending a SIGBUS to the user process, thus
>> give it a chance to recover.
> It seems the user process will still receive SIGBUS via kill_accessing_process()
> when (re-)access thp later. So they should have a chance to recover already.
> Or am I miss something?

The concern is about real UE consumption in which case, it's desirable 
to kill the process ASAP without having to relying on subsequent 
access.  Also to honor processes' MCE-early-kill request. 
kill_accessing_process() is very conservative in that, it doesn't check 
other processes that have the poisoned page mapped.

thanks,

-jane

>
> Thanks.
> .
>
>

Re: [PATCH 3/3] mm/memory-failure: send SIGBUS in the event of thp split fail

[Miaohe Lin]

On 2024/5/9 0:56, Jane Chu wrote:
> On 5/8/2024 2:03 AM, Miaohe Lin wrote:
> 
>> On 2024/5/2 7:24, Jane Chu wrote:
>>> When handle hwpoison in a GUP longterm pin'ed thp page,
>>> try_to_split_thp_page() will fail. And at this point, there is little else
>>> the kernel could do except sending a SIGBUS to the user process, thus
>>> give it a chance to recover.
>> It seems the user process will still receive SIGBUS via kill_accessing_process()
>> when (re-)access thp later. So they should have a chance to recover already.
>> Or am I miss something?
> 
> The concern is about real UE consumption in which case, it's desirable to kill the process ASAP without having to relying on subsequent access.  Also to honor processes' MCE-early-kill request. kill_accessing_process() is very conservative in that, it doesn't check other processes that have the poisoned page mapped.

I see. Thanks for your explanation.
Thanks.
.

> 
> thanks,
> 
> -jane
> 
>>
>> Thanks.
>> .
>>
>>
> .