linux-kernel - Re: [PATCH 3.12 78/78] mm: let mm_find

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] [day] [month] [year] [list]

Message-ID: <alpine.LSU.2.11.1501121448200.1589@eggly.anvils>
Date:	Mon, 12 Jan 2015 15:13:29 -0800 (PST)
From:	Hugh Dickins <hughd@...gle.com>
To:	Jiri Slaby <jslaby@...e.cz>
cc:	"Kirill A. Shutemov" <kirill@...temov.name>,
	Hugh Dickins <hughd@...gle.com>, stable@...r.kernel.org,
	"Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
	linux-kernel@...r.kernel.org,
	Konstantin Khlebnikov <koct9i@...il.com>,
	Mel Gorman <mgorman@...e.de>, Bob Liu <bob.liu@...cle.com>,
	Christoph Lameter <cl@...two.org>,
	Dave Jones <davej@...hat.com>,
	David Rientjes <rientjes@...gle.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH 3.12 78/78] mm: let mm_find_pmd fix buggy race with THP
 fault

On Mon, 12 Jan 2015, Kirill A. Shutemov wrote:
> On Mon, Jan 12, 2015 at 11:01:46AM +0100, Jiri Slaby wrote:
> > On 01/10/2015, 06:01 AM, Hugh Dickins wrote:
> > > On Fri, 9 Jan 2015, Jiri Slaby wrote:
> > > 
> > >> From: Hugh Dickins <hughd@...gle.com>
> > >>
> > >> 3.12-stable review patch.  If anyone has any objections, please let me know.
> > >>
> > >> ===============
> > >>
> > >> commit f72e7dcdd25229446b102e587ef2f826f76bff28 upstream.
> > ...
> > > Fine for this to go in, but there is one catch, which I discovered when
> > > backporting to v3.11: it needed one more hunk.  I haven't checked your
> > > base tree, but if this applies then I believe you need it - most of the
> > > time no problem, but it can case page migration to fail to find a
> > > migration entry it inserted earlier, then BUG_ON(!PageLocked(p)) in
> > > migration_entry_to_page() soon after.  Here's what I wrote back then:
> > > 
> > > Note on rebase to v3.11: added a hunk to replace the use of mm_find_pmd()
> > > in page_check_address_pmd().  This call had been similarly replaced by
> > > the time of my v3.16 commit, in Kirill Shutemov's v3.15 b5a8cad376ee
> > > ("thp: close race between split and zap huge pages"): which we do not
> > > need as such, since it's fixing v3.13 117b0791ac42 ("mm, thp: move ptl
> > > taking inside page_check_address_pmd()"), from a split page-table-lock
> > > series we are not backporting.  But without this additional hunk, rmap
> > > sometimes broke when the new semantic for mm_find_pmd() was used here.
> > > 
> > > (Adding Kirill to Cc: shouldn't he have been Cc'ed already?)
> > > 
> > > Hugh
> > 
> > Thanks, I see. So the diff between the hunk below and 117b0791ac42 are
> > two things:
> > 
> > > --- a/mm/huge_memory.c
> > > +++ b/mm/huge_memory.c
> > > @@ -1584,12 +1584,20 @@ pmd_t *page_check_address_pmd(struct page *page,
> > >  			      unsigned long address,
> > >  			      enum page_check_address_pmd_flag flag)
> > >  {
> > > +	pgd_t *pgd;
> > > +	pud_t *pud;
> > >  	pmd_t *pmd, *ret = NULL;
> > >  
> > >  	if (address & ~HPAGE_PMD_MASK)
> > >  		goto out;
> > >  
> > > -	pmd = mm_find_pmd(mm, address);
> > > +	pgd = pgd_offset(mm, address);
> > > +	if (!pgd_present(*pgd))
> > > +		goto out;
> > > +	pud = pud_offset(pgd, address);
> > > +	if (!pud_present(*pud))
> > > +		goto out;
> > > +	pmd = pmd_offset(pud, address);
> > >  	if (!pmd)
> > >  		goto out;
> > 
> > This check is removed by 117b0791ac42. Can actually pmd returned from
> > pmd_offset be NULL?
> 
> [ I believe, you mean by b5a8cad376ee, right? ]
> 
> No, pmd cannot be NULL here, if pud is present and valid (pud_page_vaddr()
> is not NULL).

Right, the !pmd test after pmd_offset is just stupid: I thought I was
copying a standard version of that sequence from somewhere, and blindly
duplicating the stupid test; but looking back now, suspect I was the
one introducing that stupidity.  It doesn't do anything wrong, but
it's misleadingly stupid and better removed.

> 
> > 
> > >  	if (pmd_none(*pmd))
> > 
> > pmd_none() is replaced by !pmd_present().
> 
> Both pmd_none() and !pmd_present() would work. pmd_none() can be slightly
> faster.

Right, you can use whichever you feel like.

> 
> > My question is: is it OK to take the backport of 117b0791ac42 attached
> > (to stay with what upstream has)?
> 
> The commit message would be totally misleading, since the fixed bug is not
> present in v3.12. It's better to fold the patch into "mm: let mm_find_pmd
> fix buggy race with THP".

Agreed, it would be inappropriate to incorporate Kirill's changelog:
better just to append the text I supplied pointing to his commits.

Hugh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/