Messages in this thread | | | Subject | Re: [PATCH v5] mm/gup: check page hwposion status for coredump. | From | David Hildenbrand <> | Date | Wed, 31 Mar 2021 08:44:53 +0200 |
| |
On 31.03.21 06:32, HORIGUCHI NAOYA(堀口 直也) wrote: > On Wed, Mar 31, 2021 at 10:43:36AM +0800, Aili Yao wrote: >> On Wed, 31 Mar 2021 01:52:59 +0000 HORIGUCHI NAOYA(堀口 直也) <naoya.horiguchi@nec.com> wrote: >>> On Fri, Mar 26, 2021 at 03:22:49PM +0100, David Hildenbrand wrote: >>>> On 26.03.21 15:09, David Hildenbrand wrote: >>>>> On 22.03.21 12:33, Aili Yao wrote: >>>>>> When we do coredump for user process signal, this may be one SIGBUS signal >>>>>> with BUS_MCEERR_AR or BUS_MCEERR_AO code, which means this signal is >>>>>> resulted from ECC memory fail like SRAR or SRAO, we expect the memory >>>>>> recovery work is finished correctly, then the get_dump_page() will not >>>>>> return the error page as its process pte is set invalid by >>>>>> memory_failure(). >>>>>> >>>>>> But memory_failure() may fail, and the process's related pte may not be >>>>>> correctly set invalid, for current code, we will return the poison page, >>>>>> get it dumped, and then lead to system panic as its in kernel code. >>>>>> >>>>>> So check the hwpoison status in get_dump_page(), and if TRUE, return NULL. >>>>>> >>>>>> There maybe other scenario that is also better to check hwposion status >>>>>> and not to panic, so make a wrapper for this check, Thanks to David's >>>>>> suggestion(<david@redhat.com>). >>>>>> >>>>>> Link: https://lkml.kernel.org/r/20210319104437.6f30e80d@alex-virtual-machine >>>>>> Signed-off-by: Aili Yao <yaoaili@kingsoft.com> >>>>>> Cc: David Hildenbrand <david@redhat.com> >>>>>> Cc: Matthew Wilcox <willy@infradead.org> >>>>>> Cc: Naoya Horiguchi <naoya.horiguchi@nec.com> >>>>>> Cc: Oscar Salvador <osalvador@suse.de> >>>>>> Cc: Mike Kravetz <mike.kravetz@oracle.com> >>>>>> Cc: Aili Yao <yaoaili@kingsoft.com> >>>>>> Cc: stable@vger.kernel.org >>>>>> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> >>>>>> --- >>>>>> mm/gup.c | 4 ++++ >>>>>> mm/internal.h | 20 ++++++++++++++++++++ >>>>>> 2 files changed, 24 insertions(+) >>>>>> >>>>>> diff --git a/mm/gup.c b/mm/gup.c >>>>>> index e4c224c..6f7e1aa 100644 >>>>>> --- a/mm/gup.c >>>>>> +++ b/mm/gup.c >>>>>> @@ -1536,6 +1536,10 @@ struct page *get_dump_page(unsigned long addr) >>>>>> FOLL_FORCE | FOLL_DUMP | FOLL_GET); >>>>>> if (locked) >>>>>> mmap_read_unlock(mm); >>>>> >>>>> Thinking again, wouldn't we get -EFAULT from __get_user_pages_locked() >>>>> when stumbling over a hwpoisoned page? >>>>> >>>>> See __get_user_pages_locked()->__get_user_pages()->faultin_page(): >>>>> >>>>> handle_mm_fault()->vm_fault_to_errno(), which translates >>>>> VM_FAULT_HWPOISON to -EFAULT, unless FOLL_HWPOISON is set (-> -EHWPOISON) >>>>> >>>>> ? >>> >>> We could get -EFAULT, but sometimes not (depends on how memory_failure() fails). >>> >>> If we failed to unmap, the page table is not converted to hwpoison entry, >>> so __get_user_pages_locked() get the hwpoisoned page. >>> >>> If we successfully unmapped but failed in truncate_error_page() for example, >>> the processes mapping the page would get -EFAULT as expected. But even in >>> this case, other processes could reach the error page via page cache and >>> __get_user_pages_locked() for them could return the hwpoisoned page. >>> >>>> >>>> Or doesn't that happen as you describe "But memory_failure() may fail, and >>>> the process's related pte may not be correctly set invalid" -- but why does >>>> that happen? >>> >>> Simply because memory_failure() doesn't handle some page types like ksm page >>> and zero page. Or maybe shmem thp also belongs to this class.
Thanks for that info!
>>> >>>> >>>> On a similar thought, should get_user_pages() never return a page that has >>>> HWPoison set? E.g., check also for existing PTEs if the page is hwpoisoned? >>> >>> Make sense to me. Maybe inserting hwpoison check into follow_page_pte() and >>> follow_huge_pmd() would work well. >> >> I think we should take more care to broadcast the hwpoison check to other cases, >> SIGBUS coredump is such a case that it is supposed to not touch the poison page, >> and if we return NULL for this, the coredump process will get a successful finish. >> >> Other cases may also meet the requirements like coredump, but we need to identify it, >> that's the poison check wrapper's purpose. If not, we may break the integrity of the >> related action, which may be no better than panic. > > If you worry about regression and would like to make this new behavior conditional, > we could use FOLL_HWPOISON to specify that the caller is hwpoison-aware so that > any !FOLL_HWPOISON caller ignores the hwpoison check and works as it does now. > This approach looks to me helpful because it would encourage developers touching > gup code to pay attention to FOLL_HWPOISON code.
FOLL_HWPOISON might be the right start, indeed.
-- Thanks,
David / dhildenb
| |