Re: [PATCH v3 3/4] mm: don't expose non-hugetlb page to fast gup prematurely

From: Yu Zhao <yuzhao@google.com>
To: Michal Hocko <mhocko@kernel.org>
Cc: "John Hubbard" <jhubbard@nvidia.com>,
	"Kirill A. Shutemov" <kirill@shutemov.name>,
	"Peter Zijlstra" <peterz@infradead.org>,
	"Andrew Morton" <akpm@linux-foundation.org>,
	"Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>,
	"Ingo Molnar" <mingo@redhat.com>,
	"Arnaldo Carvalho de Melo" <acme@kernel.org>,
	"Alexander Shishkin" <alexander.shishkin@linux.intel.com>,
	"Jiri Olsa" <jolsa@redhat.com>,
	"Namhyung Kim" <namhyung@kernel.org>,
	"Vlastimil Babka" <vbabka@suse.cz>,
	"Hugh Dickins" <hughd@google.com>,
	"Jérôme Glisse" <jglisse@redhat.com>,
	"Andrea Arcangeli" <aarcange@redhat.com>,
	"Aneesh Kumar K . V" <aneesh.kumar@linux.ibm.com>,
	"David Rientjes" <rientjes@google.com>,
	"Matthew Wilcox" <willy@infradead.org>,
	"Lance Roy" <ldr709@gmail.com>,
	"Ralph Campbell" <rcampbell@nvidia.com>,
	"Jason Gunthorpe" <jgg@ziepe.ca>,
	"Dave Airlie" <airlied@redhat.com>,
	"Thomas Hellstrom" <thellstrom@vmware.com>,
	"Souptick Joarder" <jrdr.linux@gmail.com>,
	"Mel Gorman" <mgorman@suse.de>, "Jan Kara" <jack@suse.cz>,
	"Mike Kravetz" <mike.kravetz@oracle.com>,
	"Huang Ying" <ying.huang@intel.com>,
	"Aaron Lu" <ziqian.lzq@antfin.com>,
	"Omar Sandoval" <osandov@fb.com>,
	"Thomas Gleixner" <tglx@linutronix.de>,
	"Vineeth Remanan Pillai" <vpillai@digitalocean.com>,
	"Daniel Jordan" <daniel.m.jordan@oracle.com>,
	"Mike Rapoport" <rppt@linux.ibm.com>,
	"Joel Fernandes" <joel@joelfernandes.org>,
	"Mark Rutland" <mark.rutland@arm.com>,
	"Alexander Duyck" <alexander.h.duyck@linux.intel.com>,
	"Pavel Tatashin" <pavel.tatashin@microsoft.com>,
	"David Hildenbrand" <david@redhat.com>,
	"Juergen Gross" <jgross@suse.com>,
	"Anthony Yznaga" <anthony.yznaga@oracle.com>,
	"Johannes Weiner" <hannes@cmpxchg.org>,
	"Darrick J . Wong" <darrick.wong@oracle.com>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [PATCH v3 3/4] mm: don't expose non-hugetlb page to fast gup prematurely
Date: Fri, 27 Sep 2019 12:31:16 -0600	[thread overview]
Message-ID: <20190927183116.GA216665@google.com> (raw)
In-Reply-To: <20190927123056.GE26848@dhcp22.suse.cz>

On Fri, Sep 27, 2019 at 02:33:00PM +0200, Michal Hocko wrote:
> On Thu 26-09-19 20:26:46, John Hubbard wrote:
> > On 9/26/19 3:20 AM, Kirill A. Shutemov wrote:
> > > BTW, have you looked at other levels of page table hierarchy. Do we have
> > > the same issue for PMD/PUD/... pages?
> > > 
> > 
> > Along the lines of "what other memory barriers might be missing for
> > get_user_pages_fast(), I'm also concerned that the synchronization between
> > get_user_pages_fast() and freeing the page tables might be technically broken,
> > due to missing memory barriers on the get_user_pages_fast() side. Details:
> > 
> > gup_fast() disables interrupts, but I think it also needs some sort of
> > memory barrier(s), in order to prevent reads of the page table (gup_pgd_range,
> > etc) from speculatively happening before the interrupts are disabled. 
> 
> Could you be more specific about the race scenario please? I thought
> that the unmap path will be serialized by the pte lock.

Yes, the unmap path is protected by ptl, but the fast gup isn't.
Please correct me if I'm wrong, John. This is the hypothetical race:

CPU 1 (gup)				CPU 2 (zap)
speculatively load a pmd val
					zap the pte table pointed by the pmd val
					flush tlb by ipi
<handle ipi>
					free the pte table
local_irq_disable()
use the stale pmd val
use-after-free the pte table
local_irq_enable()

I don't think it would happen because the interrupt context on CPU 1
would act as a full mb and enforce a reload of the pmd val. But I'm
not entirely sure.