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.

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

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.

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

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

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.

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

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