Re: [mm 4.15-rc8] Random oopses under memory pressure.

[Kirill A. Shutemov]

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