[PATCH v2] mm: clean up hwpoison page cache page in fault path

[PATCH v2] mm: clean up hwpoison page cache page in fault path

[Rik van Riel]

Sometimes the page offlining code can leave behind a hwpoisoned clean
page cache page. This can lead to programs being killed over and over
and over again as they fault in the hwpoisoned page, get killed, and
then get re-spawned by whatever wanted to run them.

This is particularly embarrassing when the page was offlined due to
having too many corrected memory errors. Now we are killing tasks
due to them trying to access memory that probably isn't even corrupted.

This problem can be avoided by invalidating the page from the page
fault handler, which already has a branch for dealing with these
kinds of pages. With this patch we simply pretend the page fault
was successful if the page was invalidated, return to userspace,
incur another page fault, read in the file from disk (to a new
memory page), and then everything works again.

Signed-off-by: Rik van Riel <riel@surriel.com>
Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>
---
v2: fix compiler warning found by kernel test robot

 mm/memory.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index c125c4969913..55270ea2a7c7 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3871,11 +3871,16 @@ static vm_fault_t __do_fault(struct vm_fault *vmf)
 		return ret;
 
 	if (unlikely(PageHWPoison(vmf->page))) {
-		if (ret & VM_FAULT_LOCKED)
+		vm_fault_t poisonret = VM_FAULT_HWPOISON;
+		if (ret & VM_FAULT_LOCKED) {
+			/* Retry if a clean page was removed from the cache. */
+			if (invalidate_inode_page(vmf->page))
+				poisonret = 0;
 			unlock_page(vmf->page);
+		}
 		put_page(vmf->page);
 		vmf->page = NULL;
-		return VM_FAULT_HWPOISON;
+		return poisonret;
 	}
 
 	if (unlikely(!(ret & VM_FAULT_LOCKED)))
-- 
2.34.1



-- 
All rights reversed.

Re: [PATCH v2] mm: clean up hwpoison page cache page in fault path

[Andrew Morton]

> Subject: [PATCH v2] mm: clean up hwpoison page cache page in fault path

At first scan I thought this was a code cleanup.

I think I'll do s/clean up/invalidate/.

On Sat, 12 Feb 2022 21:37:40 -0500 Rik van Riel <riel@surriel.com> wrote:

> Sometimes the page offlining code can leave behind a hwpoisoned clean
> page cache page.

Is this correct behaviour?

> This can lead to programs being killed over and over
> and over again as they fault in the hwpoisoned page, get killed, and
> then get re-spawned by whatever wanted to run them.
> 
> This is particularly embarrassing when the page was offlined due to
> having too many corrected memory errors. Now we are killing tasks
> due to them trying to access memory that probably isn't even corrupted.
> 
> This problem can be avoided by invalidating the page from the page
> fault handler, which already has a branch for dealing with these
> kinds of pages. With this patch we simply pretend the page fault
> was successful if the page was invalidated, return to userspace,
> incur another page fault, read in the file from disk (to a new
> memory page), and then everything works again.

Is this worth a cc:stable?

Re: [PATCH v2] mm: clean up hwpoison page cache page in fault path

[Rik van Riel]

[-- Attachment #1: Type: text/plain, Size: 1770 bytes --]

On Mon, 2022-02-14 at 15:24 -0800, Andrew Morton wrote:
> 
> > Subject: [PATCH v2] mm: clean up hwpoison page cache page in fault
> > path
> 
> At first scan I thought this was a code cleanup.
> 
> I think I'll do s/clean up/invalidate/.
> 
OK, that sounds good.

> On Sat, 12 Feb 2022 21:37:40 -0500 Rik van Riel <riel@surriel.com>
> wrote:
> 
> > Sometimes the page offlining code can leave behind a hwpoisoned
> > clean
> > page cache page.
> 
> Is this correct behaviour?

It is not desirable, and the soft page offlining code
tries to invalidate the page, but I don't think overhauling
the way we lock and refcount page cache pages just to make
offlining them more reliable would be worthwhile, when we
already have a branch in the page fault handler to deal with
these pages, anyway.

> > This can lead to programs being killed over and over
> > and over again as they fault in the hwpoisoned page, get killed,
> > and
> > then get re-spawned by whatever wanted to run them.
> > 
> > This is particularly embarrassing when the page was offlined due to
> > having too many corrected memory errors. Now we are killing tasks
> > due to them trying to access memory that probably isn't even
> > corrupted.
> > 
> > This problem can be avoided by invalidating the page from the page
> > fault handler, which already has a branch for dealing with these
> > kinds of pages. With this patch we simply pretend the page fault
> > was successful if the page was invalidated, return to userspace,
> > incur another page fault, read in the file from disk (to a new
> > memory page), and then everything works again.
> 
> Is this worth a cc:stable?

Maybe. I don't know how far back this issue goes...

-- 
All Rights Reversed.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 484 bytes --]

Re: [PATCH v2] mm: clean up hwpoison page cache page in fault path

[HORIGUCHI NAOYA(堀口 直也)]

On Mon, Feb 14, 2022 at 08:37:26PM -0500, Rik van Riel wrote:
> On Mon, 2022-02-14 at 15:24 -0800, Andrew Morton wrote:
> > 
> > > Subject: [PATCH v2] mm: clean up hwpoison page cache page in fault
> > > path
> > 
> > At first scan I thought this was a code cleanup.
> > 
> > I think I'll do s/clean up/invalidate/.
> > 
> OK, that sounds good.
> 
> > On Sat, 12 Feb 2022 21:37:40 -0500 Rik van Riel <riel@surriel.com>
> > wrote:
> > 
> > > Sometimes the page offlining code can leave behind a hwpoisoned
> > > clean
> > > page cache page.
> > 
> > Is this correct behaviour?
> 
> It is not desirable, and the soft page offlining code
> tries to invalidate the page, but I don't think overhauling
> the way we lock and refcount page cache pages just to make
> offlining them more reliable would be worthwhile, when we
> already have a branch in the page fault handler to deal with
> these pages, anyway.

I don't have any idea about how this kind of page is left on page
cache after page offlining.  But I agree with the suggested change.

> 
> > > This can lead to programs being killed over and over
> > > and over again as they fault in the hwpoisoned page, get killed,
> > > and
> > > then get re-spawned by whatever wanted to run them.
> > > 
> > > This is particularly embarrassing when the page was offlined due to
> > > having too many corrected memory errors. Now we are killing tasks
> > > due to them trying to access memory that probably isn't even
> > > corrupted.
> > > 
> > > This problem can be avoided by invalidating the page from the page
> > > fault handler, which already has a branch for dealing with these
> > > kinds of pages. With this patch we simply pretend the page fault
> > > was successful if the page was invalidated, return to userspace,
> > > incur another page fault, read in the file from disk (to a new
> > > memory page), and then everything works again.
> > 
> > Is this worth a cc:stable?
> 
> Maybe. I don't know how far back this issue goes...

This issue should be orthogonal with recent changes on hwpoison, and
the base code targetted by this patch is unchanged since 2016 (4.10-rc1),
so this patch is simply applicable to most of the maintained stable trees
(maybe except 4.9.z).

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

Thanks,
Naoya Horiguchi

Re: [PATCH v2] mm: clean up hwpoison page cache page in fault path

[David Hildenbrand]

On 13.02.22 03:37, Rik van Riel wrote:
> Sometimes the page offlining code can leave behind a hwpoisoned clean
> page cache page. This can lead to programs being killed over and over
> and over again as they fault in the hwpoisoned page, get killed, and
> then get re-spawned by whatever wanted to run them.

Hi Rik,

am I correct that you are only talking about soft offlining as triggered
from mm/memory-failure.c, not page offlining as in memory offlining
mm/memory_hotunplug.c ?

Maybe you can clarify that in the patch description, it made me nervous
for a second :)

-- 
Thanks,

David / dhildenb

Re: [PATCH v2] mm: clean up hwpoison page cache page in fault path

[Oscar Salvador]

On Sat, Feb 12, 2022 at 09:37:40PM -0500, Rik van Riel wrote:
> Sometimes the page offlining code can leave behind a hwpoisoned clean
> page cache page. This can lead to programs being killed over and over
> and over again as they fault in the hwpoisoned page, get killed, and
> then get re-spawned by whatever wanted to run them.

Hi Rik,

Do you know how that exactly happens? We should not be really leaving
anything behind, and soft-offline (not hard) code works with the premise
of only poisoning a page in case it was contained, so I am wondering
what is going on here.

In-use pagecache pages are migrated away, and the actual page is
contained, and for clean ones, we already do the invalidate_inode_page()
and then contain it in case we succeed.

One scenario I can imagine this can happen is if by the time we call
page_handle_poison(), someone has taken another refcount on the page,
and the put_page() does not really free it, but I am not sure that
can happen.

-- 
Oscar Salvador
SUSE Labs

Re: [PATCH v2] mm: clean up hwpoison page cache page in fault path

[Rik van Riel]

[-- Attachment #1: Type: text/plain, Size: 810 bytes --]

On Tue, 2022-02-15 at 12:22 +0100, David Hildenbrand wrote:
> On 13.02.22 03:37, Rik van Riel wrote:
> > Sometimes the page offlining code can leave behind a hwpoisoned
> > clean
> > page cache page. This can lead to programs being killed over and
> > over
> > and over again as they fault in the hwpoisoned page, get killed,
> > and
> > then get re-spawned by whatever wanted to run them.
> 
> Hi Rik,
> 
> am I correct that you are only talking about soft offlining as
> triggered
> from mm/memory-failure.c, not page offlining as in memory offlining
> mm/memory_hotunplug.c ?

That is correct in that I am talking only about memory-failure.c,
however the code in memory-failure.c has both hard and soft
offlining modes, and I suppose this patch covers both?

-- 
All Rights Reversed.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] mm: clean up hwpoison page cache page in fault path

[Rik van Riel]

[-- Attachment #1: Type: text/plain, Size: 1498 bytes --]

On Tue, 2022-02-15 at 13:51 +0100, Oscar Salvador wrote:
> On Sat, Feb 12, 2022 at 09:37:40PM -0500, Rik van Riel wrote:
> > Sometimes the page offlining code can leave behind a hwpoisoned
> > clean
> > page cache page. This can lead to programs being killed over and
> > over
> > and over again as they fault in the hwpoisoned page, get killed,
> > and
> > then get re-spawned by whatever wanted to run them.
> 
> Hi Rik,
> 
> Do you know how that exactly happens? We should not be really leaving
> anything behind, and soft-offline (not hard) code works with the
> premise
> of only poisoning a page in case it was contained, so I am wondering
> what is going on here.
> 
> In-use pagecache pages are migrated away, and the actual page is
> contained, and for clean ones, we already do the
> invalidate_inode_page()
> and then contain it in case we succeed.

I do not know the exact failure case, since I have never
caught a system in the act of leaking one of these pages.

I just know I have seen this issue on systems where the
"soft_offline: %#lx: invalidated\n" printk was the only
offline method leaving any message in the kernel log.

However, there are a few code paths through the soft
offlining code path that don't seem to have any printks,
so I am not sure exactly where things went wrong.

I only really found the aftermath, and tested this patch
by loading it as a kernel live patch module on some of
those systems.

-- 
All Rights Reversed.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2] mm: clean up hwpoison page cache page in fault path

[David Hildenbrand]

On 15.02.22 16:01, Rik van Riel wrote:
> On Tue, 2022-02-15 at 12:22 +0100, David Hildenbrand wrote:
>> On 13.02.22 03:37, Rik van Riel wrote:
>>> Sometimes the page offlining code can leave behind a hwpoisoned
>>> clean
>>> page cache page. This can lead to programs being killed over and
>>> over
>>> and over again as they fault in the hwpoisoned page, get killed,
>>> and
>>> then get re-spawned by whatever wanted to run them.
>>
>> Hi Rik,
>>
>> am I correct that you are only talking about soft offlining as
>> triggered
>> from mm/memory-failure.c, not page offlining as in memory offlining
>> mm/memory_hotunplug.c ?
> 
> That is correct in that I am talking only about memory-failure.c,
> however the code in memory-failure.c has both hard and soft
> offlining modes, and I suppose this patch covers both?
> 

Right, for hwpoison handling there is hard and soft offlining of pages
... maybe "hwpoison page offlining" would be clearer, not sure.

Thanks for clarifying!

-- 
Thanks,

David / dhildenb

Re: [PATCH v2] mm: clean up hwpoison page cache page in fault path

[Miaohe Lin]

On 2022/2/15 20:51, Oscar Salvador wrote:
> On Sat, Feb 12, 2022 at 09:37:40PM -0500, Rik van Riel wrote:
>> Sometimes the page offlining code can leave behind a hwpoisoned clean
>> page cache page. This can lead to programs being killed over and over
>> and over again as they fault in the hwpoisoned page, get killed, and
>> then get re-spawned by whatever wanted to run them.
> 
> Hi Rik,
> 
> Do you know how that exactly happens? We should not be really leaving
> anything behind, and soft-offline (not hard) code works with the premise
> of only poisoning a page in case it was contained, so I am wondering
> what is going on here.
> 
> In-use pagecache pages are migrated away, and the actual page is
> contained, and for clean ones, we already do the invalidate_inode_page()
> and then contain it in case we succeed.
> 

IIUC, this could not happen when soft-offlining a pagecache page. They're either
invalidated or migrated away and then we set PageHWPoison.
I think this may happen on a clean pagecache page when it's isolated. So it's !PageLRU.
And identify_page_state treats it as me_unknown because it's non reserved, slab, swapcache
and so on ...(see error_states for details). Or am I miss anything?

Thanks.

> One scenario I can imagine this can happen is if by the time we call
> page_handle_poison(), someone has taken another refcount on the page,
> and the put_page() does not really free it, but I am not sure that
> can happen.
> 

Re: [PATCH v2] mm: clean up hwpoison page cache page in fault path

[Oscar Salvador]

On Wed, Feb 16, 2022 at 10:13:14AM +0800, Miaohe Lin wrote:
> IIUC, this could not happen when soft-offlining a pagecache page. They're either
> invalidated or migrated away and then we set PageHWPoison.
> I think this may happen on a clean pagecache page when it's isolated. So it's !PageLRU.
> And identify_page_state treats it as me_unknown because it's non reserved, slab, swapcache
> and so on ...(see error_states for details). Or am I miss anything?

But the path you are talking about is when we do have a non-recoverable
error, so memory_failure() path.
AFAIU, Rik talks about pages with corrected errors, and that is
soft_offline().

-- 
Oscar Salvador
SUSE Labs

Re: [PATCH v2] mm: clean up hwpoison page cache page in fault path

[Oscar Salvador]

On Sat, Feb 12, 2022 at 09:37:40PM -0500, Rik van Riel wrote:
> Sometimes the page offlining code can leave behind a hwpoisoned clean
> page cache page. This can lead to programs being killed over and over
> and over again as they fault in the hwpoisoned page, get killed, and
> then get re-spawned by whatever wanted to run them.
> 
> This is particularly embarrassing when the page was offlined due to
> having too many corrected memory errors. Now we are killing tasks
> due to them trying to access memory that probably isn't even corrupted.
> 
> This problem can be avoided by invalidating the page from the page
> fault handler, which already has a branch for dealing with these
> kinds of pages. With this patch we simply pretend the page fault
> was successful if the page was invalidated, return to userspace,
> incur another page fault, read in the file from disk (to a new
> memory page), and then everything works again.
> 
> Signed-off-by: Rik van Riel <riel@surriel.com>
> Reviewed-by: Miaohe Lin <linmiaohe@huawei.com>

Although I would really loved to understand how we got there,
it fixes the problem, so:

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

-- 
Oscar Salvador
SUSE Labs

Re: [PATCH v2] mm: clean up hwpoison page cache page in fault path

[Miaohe Lin]

On 2022/2/21 20:54, Oscar Salvador wrote:
> On Wed, Feb 16, 2022 at 10:13:14AM +0800, Miaohe Lin wrote:
>> IIUC, this could not happen when soft-offlining a pagecache page. They're either
>> invalidated or migrated away and then we set PageHWPoison.
>> I think this may happen on a clean pagecache page when it's isolated. So it's !PageLRU.
>> And identify_page_state treats it as me_unknown because it's non reserved, slab, swapcache
>> and so on ...(see error_states for details). Or am I miss anything?
> 
> But the path you are talking about is when we do have a non-recoverable
> error, so memory_failure() path.
> AFAIU, Rik talks about pages with corrected errors, and that is
> soft_offline().

Oh, yes, Rik talks about pages with corrected errors. My mistake. Then I really
want to understand how we got there too. :)

Thanks.

>