[PATCH 0/4] A few fixup and cleanup patches for memory-failure

[PATCH 0/4] A few fixup and cleanup patches for memory-failure

[Miaohe Lin]

Hi everyone,
This series contains a few fixup patches to fix potential unexpected
return value, fix wrong swap entry type for hwpoisoned swapcache page
and so on. More details can be found in the respective changelogs.
Thanks!

Miaohe Lin (4):
  mm/swapfile: fix wrong swap entry type for hwpoisoned swapcache page
  mm: memory-failure: fix potential unexpected return value from
    unpoison_memory()
  mm: memory-failure: avoid false hwpoison page mapped error info
  mm: memory-failure: add PageOffline() check

 mm/memory-failure.c | 15 ++++++++++-----
 mm/swapfile.c       |  3 ++-
 2 files changed, 12 insertions(+), 6 deletions(-)

-- 
2.33.0

[PATCH 1/4] mm/swapfile: fix wrong swap entry type for hwpoisoned swapcache page

[Miaohe Lin]

Hwpoisoned dirty swap cache page is kept in the swap cache and there's
simple interception code in do_swap_page() to catch it. But when trying
to swapoff, unuse_pte() will wrongly install a general sense of "future
accesses are invalid" swap entry for hwpoisoned swap cache page due to
unaware of such type of page. The user will receive SIGBUS signal without
expected BUS_MCEERR_AR payload.

Fixes: 6b970599e807 ("mm: hwpoison: support recovery from ksm_might_need_to_copy()")
Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/swapfile.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 346e22b8ae97..02f6808e65bf 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -1767,7 +1767,8 @@ static int unuse_pte(struct vm_area_struct *vma, pmd_t *pmd,
 		swp_entry_t swp_entry;
 
 		dec_mm_counter(vma->vm_mm, MM_SWAPENTS);
-		if (hwposioned) {
+		/* Hwpoisoned swapcache page is also !PageUptodate. */
+		if (hwposioned || PageHWPoison(page)) {
 			swp_entry = make_hwpoison_entry(swapcache);
 			page = swapcache;
 		} else {
-- 
2.33.0

[PATCH 2/4] mm: memory-failure: fix potential unexpected return value from unpoison_memory()

[Miaohe Lin]

If unpoison_memory() fails to clear page hwpoisoned flag, return value
ret is expected to be -EBUSY. But when get_hwpoison_page() returns 1
and fails to clear page hwpoisoned flag due to races, return value will
be unexpected 1 leading to users being confused.

Fixes: bf181c582588 ("mm/hwpoison: fix unpoison_memory()")
Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/memory-failure.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 9ab97016877e..ac074f82f5b3 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -2546,11 +2546,11 @@ int unpoison_memory(unsigned long pfn)
 			unpoison_pr_info("Unpoison: failed to grab page %#lx\n",
 					 pfn, &unpoison_rs);
 	} else {
+		ret = -EBUSY;
 		if (PageHuge(p)) {
 			huge = true;
 			count = folio_free_raw_hwp(folio, false);
 			if (count == 0) {
-				ret = -EBUSY;
 				folio_put(folio);
 				goto unlock_mutex;
 			}
-- 
2.33.0

[PATCH 3/4] mm: memory-failure: avoid false hwpoison page mapped error info

[Miaohe Lin]

folio->_mapcount is overloaded in SLAB, so folio_mapped() has to be done
after folio_test_slab() is checked. Otherwise slab folio might be treated
as a mapped folio leading to false 'Someone maps the hwpoison page' error
info.

Fixes: 230ac719c500 ("mm/hwpoison: don't try to unpoison containment-failed pages")
Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/memory-failure.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index ac074f82f5b3..42e63b0ab5f7 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -2513,6 +2513,13 @@ int unpoison_memory(unsigned long pfn)
 		goto unlock_mutex;
 	}
 
+	if (folio_test_slab(folio) || PageTable(&folio->page) || folio_test_reserved(folio))
+		goto unlock_mutex;
+
+	/*
+	 * Note that folio->_mapcount is overloaded in SLAB, so the simple test
+	 * in folio_mapped() has to be done after folio_test_slab() is checked.
+	 */
 	if (folio_mapped(folio)) {
 		unpoison_pr_info("Unpoison: Someone maps the hwpoison page %#lx\n",
 				 pfn, &unpoison_rs);
@@ -2525,9 +2532,6 @@ int unpoison_memory(unsigned long pfn)
 		goto unlock_mutex;
 	}
 
-	if (folio_test_slab(folio) || PageTable(&folio->page) || folio_test_reserved(folio))
-		goto unlock_mutex;
-
 	ret = get_hwpoison_page(p, MF_UNPOISON);
 	if (!ret) {
 		if (PageHuge(p)) {
-- 
2.33.0

[PATCH 4/4] mm: memory-failure: add PageOffline() check

[Miaohe Lin]

Memory failure is not interested in logically offlined page. Skip this
type of pages.

Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
---
 mm/memory-failure.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 42e63b0ab5f7..ed79b69837de 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1559,7 +1559,7 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
 	 * Here we are interested only in user-mapped pages, so skip any
 	 * other types of pages.
 	 */
-	if (PageReserved(p) || PageSlab(p) || PageTable(p))
+	if (PageReserved(p) || PageSlab(p) || PageTable(p) || PageOffline(p))
 		return true;
 	if (!(PageLRU(hpage) || PageHuge(p)))
 		return true;
@@ -2513,7 +2513,8 @@ int unpoison_memory(unsigned long pfn)
 		goto unlock_mutex;
 	}
 
-	if (folio_test_slab(folio) || PageTable(&folio->page) || folio_test_reserved(folio))
+	if (folio_test_slab(folio) || PageTable(&folio->page) ||
+	    folio_test_reserved(folio) || PageOffline(&folio->page))
 		goto unlock_mutex;
 
 	/*
-- 
2.33.0

Re: [PATCH 1/4] mm/swapfile: fix wrong swap entry type for hwpoisoned swapcache page

[Matthew Wilcox]

On Sat, Jul 15, 2023 at 11:17:26AM +0800, Miaohe Lin wrote:
> Hwpoisoned dirty swap cache page is kept in the swap cache and there's
> simple interception code in do_swap_page() to catch it. But when trying
> to swapoff, unuse_pte() will wrongly install a general sense of "future
> accesses are invalid" swap entry for hwpoisoned swap cache page due to
> unaware of such type of page. The user will receive SIGBUS signal without
> expected BUS_MCEERR_AR payload.

Have you observed this, or do you just think it's true?

> +++ b/mm/swapfile.c
> @@ -1767,7 +1767,8 @@ static int unuse_pte(struct vm_area_struct *vma, pmd_t *pmd,
>  		swp_entry_t swp_entry;
>  
>  		dec_mm_counter(vma->vm_mm, MM_SWAPENTS);
> -		if (hwposioned) {
> +		/* Hwpoisoned swapcache page is also !PageUptodate. */
> +		if (hwposioned || PageHWPoison(page)) {

This line makes no sense to me.  How do we get here with PageHWPoison()
being true and hwposioned being false?

>  			swp_entry = make_hwpoison_entry(swapcache);
>  			page = swapcache;
>  		} else {
> -- 
> 2.33.0
> 
> 

Re: [PATCH 3/4] mm: memory-failure: avoid false hwpoison page mapped error info

[Matthew Wilcox]

On Sat, Jul 15, 2023 at 11:17:28AM +0800, Miaohe Lin wrote:
> folio->_mapcount is overloaded in SLAB, so folio_mapped() has to be done
> after folio_test_slab() is checked. Otherwise slab folio might be treated
> as a mapped folio leading to false 'Someone maps the hwpoison page' error
> info.
> 
> Fixes: 230ac719c500 ("mm/hwpoison: don't try to unpoison containment-failed pages")
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Re: [PATCH 1/4] mm/swapfile: fix wrong swap entry type for hwpoisoned swapcache page

[Miaohe Lin]

On 2023/7/15 11:50, Matthew Wilcox wrote:
> On Sat, Jul 15, 2023 at 11:17:26AM +0800, Miaohe Lin wrote:
>> Hwpoisoned dirty swap cache page is kept in the swap cache and there's
>> simple interception code in do_swap_page() to catch it. But when trying
>> to swapoff, unuse_pte() will wrongly install a general sense of "future
>> accesses are invalid" swap entry for hwpoisoned swap cache page due to
>> unaware of such type of page. The user will receive SIGBUS signal without
>> expected BUS_MCEERR_AR payload.
> 
> Have you observed this, or do you just think it's true?
> 
>> +++ b/mm/swapfile.c
>> @@ -1767,7 +1767,8 @@ static int unuse_pte(struct vm_area_struct *vma, pmd_t *pmd,
>>  		swp_entry_t swp_entry;
>>  
>>  		dec_mm_counter(vma->vm_mm, MM_SWAPENTS);
>> -		if (hwposioned) {
>> +		/* Hwpoisoned swapcache page is also !PageUptodate. */
>> +		if (hwposioned || PageHWPoison(page)) {
> 
> This line makes no sense to me.  How do we get here with PageHWPoison()
> being true and hwposioned being false?

hwposioned will be true iff ksm_might_need_to_copy returns -EHWPOISON.
And there's PageUptodate check in ksm_might_need_to_copy before we can return -EHWPOISON:

  ksm_might_need_to_copy
    if (!PageUptodate(page))
      return page;		/* let do_swap_page report the error */
    ^^^
    Will return here because hwpoisoned swapcache page is !PageUptodate(cleared via me_swapcache_dirty()).

Or am I miss something?

Thanks.

Re: [PATCH 1/4] mm/swapfile: fix wrong swap entry type for hwpoisoned swapcache page

[Matthew Wilcox]

On Mon, Jul 17, 2023 at 10:33:14AM +0800, Miaohe Lin wrote:
> On 2023/7/15 11:50, Matthew Wilcox wrote:
> > On Sat, Jul 15, 2023 at 11:17:26AM +0800, Miaohe Lin wrote:
> >> Hwpoisoned dirty swap cache page is kept in the swap cache and there's
> >> simple interception code in do_swap_page() to catch it. But when trying
> >> to swapoff, unuse_pte() will wrongly install a general sense of "future
> >> accesses are invalid" swap entry for hwpoisoned swap cache page due to
> >> unaware of such type of page. The user will receive SIGBUS signal without
> >> expected BUS_MCEERR_AR payload.
> > 
> > Have you observed this, or do you just think it's true?
> > 
> >> +++ b/mm/swapfile.c
> >> @@ -1767,7 +1767,8 @@ static int unuse_pte(struct vm_area_struct *vma, pmd_t *pmd,
> >>  		swp_entry_t swp_entry;
> >>  
> >>  		dec_mm_counter(vma->vm_mm, MM_SWAPENTS);
> >> -		if (hwposioned) {
> >> +		/* Hwpoisoned swapcache page is also !PageUptodate. */
> >> +		if (hwposioned || PageHWPoison(page)) {
> > 
> > This line makes no sense to me.  How do we get here with PageHWPoison()
> > being true and hwposioned being false?
> 
> hwposioned will be true iff ksm_might_need_to_copy returns -EHWPOISON.
> And there's PageUptodate check in ksm_might_need_to_copy before we can return -EHWPOISON:
> 
>   ksm_might_need_to_copy
>     if (!PageUptodate(page))
>       return page;		/* let do_swap_page report the error */
>     ^^^
>     Will return here because hwpoisoned swapcache page is !PageUptodate(cleared via me_swapcache_dirty()).
> 
> Or am I miss something?

Ah!  So we don't even get to calling copy_mc_to_kernel().  That seems
like a bug in ksm_might_need_to_copy(), don't you think?  Maybe this
would be a better fix:

+	if (PageHWPoison(page))
+		return ERR_PTR(-EHWPOISON);
	if (!PageUptodate(page))
		return page;

Re: [PATCH 1/4] mm/swapfile: fix wrong swap entry type for hwpoisoned swapcache page

[Miaohe Lin]

On 2023/7/17 10:53, Matthew Wilcox wrote:
> On Mon, Jul 17, 2023 at 10:33:14AM +0800, Miaohe Lin wrote:
>> On 2023/7/15 11:50, Matthew Wilcox wrote:
>>> On Sat, Jul 15, 2023 at 11:17:26AM +0800, Miaohe Lin wrote:
>>>> Hwpoisoned dirty swap cache page is kept in the swap cache and there's
>>>> simple interception code in do_swap_page() to catch it. But when trying
>>>> to swapoff, unuse_pte() will wrongly install a general sense of "future
>>>> accesses are invalid" swap entry for hwpoisoned swap cache page due to
>>>> unaware of such type of page. The user will receive SIGBUS signal without
>>>> expected BUS_MCEERR_AR payload.
>>>
>>> Have you observed this, or do you just think it's true?
>>>
>>>> +++ b/mm/swapfile.c
>>>> @@ -1767,7 +1767,8 @@ static int unuse_pte(struct vm_area_struct *vma, pmd_t *pmd,
>>>>  		swp_entry_t swp_entry;
>>>>  
>>>>  		dec_mm_counter(vma->vm_mm, MM_SWAPENTS);
>>>> -		if (hwposioned) {
>>>> +		/* Hwpoisoned swapcache page is also !PageUptodate. */
>>>> +		if (hwposioned || PageHWPoison(page)) {
>>>
>>> This line makes no sense to me.  How do we get here with PageHWPoison()
>>> being true and hwposioned being false?
>>
>> hwposioned will be true iff ksm_might_need_to_copy returns -EHWPOISON.
>> And there's PageUptodate check in ksm_might_need_to_copy before we can return -EHWPOISON:
>>
>>   ksm_might_need_to_copy
>>     if (!PageUptodate(page))
>>       return page;		/* let do_swap_page report the error */
>>     ^^^
>>     Will return here because hwpoisoned swapcache page is !PageUptodate(cleared via me_swapcache_dirty()).
>>
>> Or am I miss something?
> 
> Ah!  So we don't even get to calling copy_mc_to_kernel().  That seems
> like a bug in ksm_might_need_to_copy(), don't you think?  Maybe this

I'm sorry but could you please explain what the bug is?

> would be a better fix:
> 
> +	if (PageHWPoison(page))
> +		return ERR_PTR(-EHWPOISON);

Looks reasonable. We can further avoid accessing contents of hwpoisoned swapcache pages which are
PageHWPoison() && PageUptodate() at first place.

Thanks.

> 	if (!PageUptodate(page))
> 		return page;
> 
> .
> 

Re: [PATCH 2/4] mm: memory-failure: fix potential unexpected return value from unpoison_memory()

[Naoya Horiguchi]

On Sat, Jul 15, 2023 at 11:17:27AM +0800, Miaohe Lin wrote:
> If unpoison_memory() fails to clear page hwpoisoned flag, return value
> ret is expected to be -EBUSY. But when get_hwpoison_page() returns 1
> and fails to clear page hwpoisoned flag due to races, return value will
> be unexpected 1 leading to users being confused.

Thank you for fixing this.

> 
> Fixes: bf181c582588 ("mm/hwpoison: fix unpoison_memory()")
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  mm/memory-failure.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 9ab97016877e..ac074f82f5b3 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -2546,11 +2546,11 @@ int unpoison_memory(unsigned long pfn)
>  			unpoison_pr_info("Unpoison: failed to grab page %#lx\n",
>  					 pfn, &unpoison_rs);
>  	} else {
> +		ret = -EBUSY;

It seems be a code smell to me that the variable "ret" is used not only to
save the return value of unpoison_memory(), but also to save the return
value from get_hwpoison_page(). So I think that it might be better to use
another auto-variable solely to save the return value of get_hwpoison_page.
Then ret has the initial value -EBUSY at this point and no need to
reinitialize it.

Thanks,
Naoya Horiguchi

>  		if (PageHuge(p)) {
>  			huge = true;
>  			count = folio_free_raw_hwp(folio, false);
>  			if (count == 0) {
> -				ret = -EBUSY;
>  				folio_put(folio);
>  				goto unlock_mutex;
>  			}
> -- 
> 2.33.0
> 
> 
> 

Re: [PATCH 3/4] mm: memory-failure: avoid false hwpoison page mapped error info

[Naoya Horiguchi]

On Sat, Jul 15, 2023 at 11:17:28AM +0800, Miaohe Lin wrote:
> folio->_mapcount is overloaded in SLAB, so folio_mapped() has to be done
> after folio_test_slab() is checked. Otherwise slab folio might be treated
> as a mapped folio leading to false 'Someone maps the hwpoison page' error
> info.
> 
> Fixes: 230ac719c500 ("mm/hwpoison: don't try to unpoison containment-failed pages")
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>

Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>

Re: [PATCH 4/4] mm: memory-failure: add PageOffline() check

[Naoya Horiguchi]

On Sat, Jul 15, 2023 at 11:17:29AM +0800, Miaohe Lin wrote:
> Memory failure is not interested in logically offlined page. Skip this
> type of pages.
> 
> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> ---
>  mm/memory-failure.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 42e63b0ab5f7..ed79b69837de 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -1559,7 +1559,7 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
>  	 * Here we are interested only in user-mapped pages, so skip any
>  	 * other types of pages.
>  	 */
> -	if (PageReserved(p) || PageSlab(p) || PageTable(p))
> +	if (PageReserved(p) || PageSlab(p) || PageTable(p) || PageOffline(p))

hwpoison_user_mappings() is called after some checks are done, so I'm not
sure that it's the right place to check PageOffline().
We could check it before setting PageHWPoison() as we do at the beginning of
memory_failure() around pfn_to_online_page().  Does it make sense?

Thanks,
Naoya Horiguchi

>  		return true;
>  	if (!(PageLRU(hpage) || PageHuge(p)))
>  		return true;
> @@ -2513,7 +2513,8 @@ int unpoison_memory(unsigned long pfn)
>  		goto unlock_mutex;
>  	}
>  
> -	if (folio_test_slab(folio) || PageTable(&folio->page) || folio_test_reserved(folio))
> +	if (folio_test_slab(folio) || PageTable(&folio->page) ||
> +	    folio_test_reserved(folio) || PageOffline(&folio->page))
>  		goto unlock_mutex;
>  
>  	/*
> -- 
> 2.33.0
> 
> 
> 

Re: [PATCH 4/4] mm: memory-failure: add PageOffline() check

[Miaohe Lin]

On 2023/7/20 9:09, Naoya Horiguchi wrote:
> On Sat, Jul 15, 2023 at 11:17:29AM +0800, Miaohe Lin wrote:
>> Memory failure is not interested in logically offlined page. Skip this
>> type of pages.
>>
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>> ---
>>  mm/memory-failure.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index 42e63b0ab5f7..ed79b69837de 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -1559,7 +1559,7 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
>>  	 * Here we are interested only in user-mapped pages, so skip any
>>  	 * other types of pages.
>>  	 */
>> -	if (PageReserved(p) || PageSlab(p) || PageTable(p))
>> +	if (PageReserved(p) || PageSlab(p) || PageTable(p) || PageOffline(p))
> 
> hwpoison_user_mappings() is called after some checks are done, so I'm not
> sure that it's the right place to check PageOffline().

hwpoison_user_mappings() is called after the "if (!PageLRU(p) && !PageWriteback(p))" check in memory_failure().
So the page can't also be PageReserved(p) or PageSlab(p) or PageTable(p) here? I think the check here just wants
to make things clear that only user-mapped pages are interested. Or am I miss something?

Thanks Naoya.

Re: [PATCH 2/4] mm: memory-failure: fix potential unexpected return value from unpoison_memory()

[Miaohe Lin]

On 2023/7/20 7:48, Naoya Horiguchi wrote:
> On Sat, Jul 15, 2023 at 11:17:27AM +0800, Miaohe Lin wrote:
>> If unpoison_memory() fails to clear page hwpoisoned flag, return value
>> ret is expected to be -EBUSY. But when get_hwpoison_page() returns 1
>> and fails to clear page hwpoisoned flag due to races, return value will
>> be unexpected 1 leading to users being confused.
> 
> Thank you for fixing this.
> 
>>
>> Fixes: bf181c582588 ("mm/hwpoison: fix unpoison_memory()")
>> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
>> ---
>>  mm/memory-failure.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
>> index 9ab97016877e..ac074f82f5b3 100644
>> --- a/mm/memory-failure.c
>> +++ b/mm/memory-failure.c
>> @@ -2546,11 +2546,11 @@ int unpoison_memory(unsigned long pfn)
>>  			unpoison_pr_info("Unpoison: failed to grab page %#lx\n",
>>  					 pfn, &unpoison_rs);
>>  	} else {
>> +		ret = -EBUSY;
> 
> It seems be a code smell to me that the variable "ret" is used not only to
> save the return value of unpoison_memory(), but also to save the return
> value from get_hwpoison_page(). So I think that it might be better to use
> another auto-variable solely to save the return value of get_hwpoison_page.
> Then ret has the initial value -EBUSY at this point and no need to
> reinitialize it.
> 

This should be a nice improvement. Will do it in v2.

Thanks Naoya.

Re: [PATCH 4/4] mm: memory-failure: add PageOffline() check

[Naoya Horiguchi]

On Thu, Jul 20, 2023 at 04:42:04PM +0800, Miaohe Lin wrote:
> On 2023/7/20 9:09, Naoya Horiguchi wrote:
> > On Sat, Jul 15, 2023 at 11:17:29AM +0800, Miaohe Lin wrote:
> >> Memory failure is not interested in logically offlined page. Skip this
> >> type of pages.
> >>
> >> Signed-off-by: Miaohe Lin <linmiaohe@huawei.com>
> >> ---
> >>  mm/memory-failure.c | 5 +++--
> >>  1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> >> index 42e63b0ab5f7..ed79b69837de 100644
> >> --- a/mm/memory-failure.c
> >> +++ b/mm/memory-failure.c
> >> @@ -1559,7 +1559,7 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
> >>  	 * Here we are interested only in user-mapped pages, so skip any
> >>  	 * other types of pages.
> >>  	 */
> >> -	if (PageReserved(p) || PageSlab(p) || PageTable(p))
> >> +	if (PageReserved(p) || PageSlab(p) || PageTable(p) || PageOffline(p))
> > 
> > hwpoison_user_mappings() is called after some checks are done, so I'm not
> > sure that it's the right place to check PageOffline().
> 
> hwpoison_user_mappings() is called after the "if (!PageLRU(p) && !PageWriteback(p))" check in memory_failure().
> So the page can't also be PageReserved(p) or PageSlab(p) or PageTable(p) here? I think the check here just wants
> to make things clear that only user-mapped pages are interested. Or am I miss something?

No, you're right,
So this "if (PageReserved(p) || PageSlab(p) || PageTable(p) || PageOffline(p))"
can be considered as checking potential deviation.
OK, so the patch is fine.

Acked-by: Naoya Horiguchi <naoya.horiguchi@nec.com>

Re: [PATCH 1/4] mm/swapfile: fix wrong swap entry type for hwpoisoned swapcache page

[Miaohe Lin]

On 2023/7/17 10:53, Matthew Wilcox wrote:
> On Mon, Jul 17, 2023 at 10:33:14AM +0800, Miaohe Lin wrote:
>> On 2023/7/15 11:50, Matthew Wilcox wrote:
>>> On Sat, Jul 15, 2023 at 11:17:26AM +0800, Miaohe Lin wrote:
>>>> Hwpoisoned dirty swap cache page is kept in the swap cache and there's
>>>> simple interception code in do_swap_page() to catch it. But when trying
>>>> to swapoff, unuse_pte() will wrongly install a general sense of "future
>>>> accesses are invalid" swap entry for hwpoisoned swap cache page due to
>>>> unaware of such type of page. The user will receive SIGBUS signal without
>>>> expected BUS_MCEERR_AR payload.
>>>
>>> Have you observed this, or do you just think it's true?
>>>
>>>> +++ b/mm/swapfile.c
>>>> @@ -1767,7 +1767,8 @@ static int unuse_pte(struct vm_area_struct *vma, pmd_t *pmd,
>>>>  		swp_entry_t swp_entry;
>>>>  
>>>>  		dec_mm_counter(vma->vm_mm, MM_SWAPENTS);
>>>> -		if (hwposioned) {
>>>> +		/* Hwpoisoned swapcache page is also !PageUptodate. */
>>>> +		if (hwposioned || PageHWPoison(page)) {
>>>
>>> This line makes no sense to me.  How do we get here with PageHWPoison()
>>> being true and hwposioned being false?
>>
>> hwposioned will be true iff ksm_might_need_to_copy returns -EHWPOISON.
>> And there's PageUptodate check in ksm_might_need_to_copy before we can return -EHWPOISON:
>>
>>   ksm_might_need_to_copy
>>     if (!PageUptodate(page))
>>       return page;		/* let do_swap_page report the error */
>>     ^^^
>>     Will return here because hwpoisoned swapcache page is !PageUptodate(cleared via me_swapcache_dirty()).
>>
>> Or am I miss something?
> 
> Ah!  So we don't even get to calling copy_mc_to_kernel().  That seems
> like a bug in ksm_might_need_to_copy(), don't you think?  Maybe this
> would be a better fix:
> 
> +	if (PageHWPoison(page))
> +		return ERR_PTR(-EHWPOISON);

I'm preparing posting the v2. But I found this won't fix the issue if CONFIG_KSM is disabled. So the correct fix
should be something like below?

diff --git a/mm/ksm.c b/mm/ksm.c
index 97a9627116fa..74804158ee02 100644
--- a/mm/ksm.c
+++ b/mm/ksm.c
@@ -2794,6 +2794,8 @@ struct page *ksm_might_need_to_copy(struct page *page,
                        anon_vma->root == vma->anon_vma->root) {
                return page;            /* still no need to copy it */
        }
+       if (PageHWPoison(page))
+               return ERR_PTR(-EHWPOISON);
        if (!PageUptodate(page))
                return page;            /* let do_swap_page report the error */

diff --git a/mm/swapfile.c b/mm/swapfile.c
index 346e22b8ae97..3dcc9d92689c 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -1744,7 +1744,7 @@ static int unuse_pte(struct vm_area_struct *vma, pmd_t *pmd,
        struct page *swapcache;
        spinlock_t *ptl;
        pte_t *pte, new_pte, old_pte;
-       bool hwposioned = false;
+       bool hwposioned = PageHWPoison(page);
        int ret = 1;

        swapcache = page;


Thanks.