On Fri, Jan 19, 2018 at 12:07:47PM +0000, Michal Hocko wrote:
> > >From 861f68c555b87fd6c0ccc3428ace91b7e185b73a Mon Sep 17 00:00:00 2001
> > From: "Kirill A. Shutemov" <kirill.shutemov@xxxxxxxxxxxxxxx>
> > Date: Thu, 18 Jan 2018 18:24:07 +0300
> > Subject: [PATCH] mm, page_vma_mapped: Drop faulty pointer arithmetics in
> > check_pte()
> >
> > Tetsuo reported random crashes under memory pressure on 32-bit x86
> > system and tracked down to change that introduced
> > page_vma_mapped_walk().
> >
> > The root cause of the issue is the faulty pointer math in check_pte().
> > As ->pte may point to an arbitrary page we have to check that they are
> > belong to the section before doing math. Otherwise it may lead to weird
> > results.
> >
> > It wasn't noticed until now as mem_map[] is virtually contiguous on flatmem or
> > vmemmap sparsemem. Pointer arithmetic just works against all 'struct page'
> > pointers. But with classic sparsemem, it doesn't.
>
> it doesn't because each section memap is allocated separately and so
> consecutive pfns crossing two sections might have struct pages at
> completely unrelated addresses.
Okay, I'll amend it.
> > Let's restructure code a bit and replace pointer arithmetic with
> > operations on pfns.
> >
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
> > Reported-by: Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx>
> > Fixes: ace71a19cec5 ("mm: introduce page_vma_mapped_walk()")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@xxxxxxxxxxxxxxx>
>
> The patch makes sense but there is one more thing to fix here.
>
> [...]
> > static bool check_pte(struct page_vma_mapped_walk *pvmw)
> > {
> > + unsigned long pfn;
> > +
> > if (pvmw->flags & PVMW_MIGRATION) {
> > #ifdef CONFIG_MIGRATION
> > swp_entry_t entry;
> > @@ -41,37 +61,34 @@ static bool check_pte(struct page_vma_mapped_walk *pvmw)
> >
> > if (!is_migration_entry(entry))
> > return false;
> > - if (migration_entry_to_page(entry) - pvmw->page >=
> > - hpage_nr_pages(pvmw->page)) {
> > - return false;
> > - }
> > - if (migration_entry_to_page(entry) < pvmw->page)
> > - return false;
> > +
> > + pfn = migration_entry_to_pfn(entry);
> > #else
> > WARN_ON_ONCE(1);
> > #endif
> > - } else {
>
> now you allow to pass through with uninitialized pfn. We used to return
> true in that case so we should probably keep it in this WARN_ON_ONCE
> case. Please note that I haven't studied this particular case and the
> ifdef is definitely not an act of art but that is a separate topic.
Good catch. Thanks.
I think returning true here is wrong as we don't validate in any way what
is mapped there. I'll put "return false;".
And I take a look if we can drop the #ifdef.
--
Kirill A. Shutemov
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
