Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 10 May 2021 11:13:28 +0800
From:   Aili Yao <yaoaili@kingsoft.com>
To:     "akpm@linux-foundation.org" <akpm@linux-foundation.org>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>
CC:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "yangfeng1@kingsoft.com" <yangfeng1@kingsoft.com>,
        "sunhao2@kingsoft.com" <sunhao2@kingsoft.com>,
        <yaoaili126@gmail.com>
Subject: Re: [PATCH v7] mm/gup: check page hwpoison status for memory
 recovery failures.
Message-ID: <20210510111328.1eef98f0@alex-virtual-machine>
In-Reply-To: <20210406104123.451ee3c3@alex-virtual-machine>
References: <20a0d078-f49d-54d6-9f04-f6b41dd51e5f@redhat.com>
        <20210318044600.GJ3420@casper.infradead.org>
        <20210318133412.12078eb7@alex-virtual-machine>
        <20210319104437.6f30e80d@alex-virtual-machine>
        <20210320003516.GC3420@casper.infradead.org>
        <20210322193318.377c9ce9@alex-virtual-machine>
        <afeac310-c6aa-f9d8-6c90-e7e7f21ddf9a@redhat.com>
        <f316ca3b-6f09-c51d-9661-66171f14ee33@redhat.com>
        <20210331015258.GB22060@hori.linux.bs1.fc.nec.co.jp>
        <20210331104303.145aea53@alex-virtual-machine>
        <20210331043231.GA26013@hori.linux.bs1.fc.nec.co.jp>
        <9f49c415-a75a-1ea3-b80c-5ba512331de6@redhat.com>
        <20210406102346.3890ceb2@alex-virtual-machine>
        <20210406104123.451ee3c3@alex-virtual-machine>
Organization: kingsoft
MIME-Version: 1.0
Content-Type: text/plain; charset="US-ASCII"
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 6 Apr 2021 10:41:23 +0800
Aili Yao <yaoaili@kingsoft.com> wrote:

> When we call get_user_pages() to pin user page in memory, there may be
> hwpoison page, currently, we just handle the normal case that memory
> recovery jod is correctly finished, and we will not return the hwpoison
> page to callers, but for other cases like memory recovery fails and the
> user process related pte is not correctly set invalid, we will still
> return the hwpoison page, and may touch it and lead to panic.
> 
> In gup.c, for normal page, after we call follow_page_mask(), we will
> return the related page pointer; or like another hwpoison case with pte
> invalid, it will return NULL. For NULL, we will handle it in if (!page)
> branch. In this patch, we will filter out the hwpoison page in
> follow_page_mask() and return error code for recovery failure cases.
> 
> We will check the page hwpoison status as soon as possible and avoid doing
> followed normal procedure and try not to grab related pages.
> 
> Changes since v6:
> - Fix wrong page pointer check in follow_trans_huge_pmd();
> 
> Signed-off-by: Aili Yao <yaoaili@kingsoft.com>

After considering more, I still insist that we should take more care to treat the case that kernel touched  user hwpoison pages.

Usually when process touched the hwpoison page, it will be killed, and no other bad consequence will happen. while in a case, kernel touches
the user process hwpoison page, It should be one more serious issue than the user process's access, the error return nor Null return is
not sufficient and reasonable for me.

There will be some doubt places, 
1) should the user process be killed first? 
2) if error returned, the process can correctly process it?  It seems there is no way the process can finish that. 
3) if NULL returned, job continue again? the data integrity will be guaranteed?  I doubted that.

There are so many arguements about the CopyIn patch from intel that the process should killed or not...
And this gup module seems more compicated and have the same question. 
When error returned or NULL return from gup, in some way and some scenario, this will make the hw issue even more complicated.

Yes, there is a flag FOLL_HWPOISON, but this flag seems be designed for different return values that the process can accept,
it is not designed to decide kernel to panic or not;

Currently I have no other scenario like coredump case. And I havn't one good way to fix this leak in a graceful way.

Maybe, For some scenario, the kernel recovery thing for user process is wrong and not deserved.

I have a lot of other works to do and have not much time to continue on this thread, And I insist my original post.

There should be a better option for this issue, when the better option is ready, this patch can and should be reverted.

Thanks!
Aili Yao