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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180119123051.xd5orkoagxanp23d@black.fi.intel.com>
Date:   Fri, 19 Jan 2018 15:30:51 +0300
From:   "Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
To:     Michal Hocko <mhocko@...nel.org>
Cc:     "Kirill A. Shutemov" <kirill@...temov.name>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>,
        torvalds@...ux-foundation.org, akpm@...ux-foundation.org,
        hannes@...xchg.org, iamjoonsoo.kim@....com,
        mgorman@...hsingularity.net, tony.luck@...el.com, vbabka@...e.cz,
        aarcange@...hat.com, hillf.zj@...baba-inc.com, hughd@...gle.com,
        oleg@...hat.com, peterz@...radead.org, riel@...hat.com,
        srikar@...ux.vnet.ibm.com, vdavydov.dev@...il.com,
        mingo@...nel.org, linux-kernel@...r.kernel.org, linux-mm@...ck.org,
        x86@...nel.org
Subject: Re: [mm 4.15-rc8] Random oopses under memory pressure.

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@...ux.intel.com>
> > 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@...ux.intel.com>
> > Reported-by: Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
> > Fixes: ace71a19cec5 ("mm: introduce page_vma_mapped_walk()")
> > Cc: stable@...r.kernel.org
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@...ux.intel.com>
> 
> 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