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

[Yu Zhao <yuzhao@google.com>]

On Thu, Sep 26, 2019 at 08:26:46PM -0700, John Hubbard wrote:
> On 9/26/19 3:20 AM, Kirill A. Shutemov wrote:
> > On Wed, Sep 25, 2019 at 04:26:54PM -0600, Yu Zhao wrote:
> >> On Wed, Sep 25, 2019 at 10:25:30AM +0200, Peter Zijlstra wrote:
> >>> On Tue, Sep 24, 2019 at 05:24:58PM -0600, Yu Zhao wrote:
> ...
> >>> I'm thinking this patch make stuff rather fragile.. Should we instead
> >>> stick the barrier in set_p*d_at() instead? Or rather, make that store a
> >>> store-release?
> >>
> >> I prefer it this way too, but I suspected the majority would be
> >> concerned with the performance implications, especially those
> >> looping set_pte_at()s in mm/huge_memory.c.
> > 
> > We can rename current set_pte_at() to __set_pte_at() or something and
> > leave it in places where barrier is not needed. The new set_pte_at()( will
> > be used in the rest of the places with the barrier inside.
> 
> +1, sounds nice. I was unhappy about the wide-ranging changes that would have
> to be maintained. So this seems much better.

Just to be clear that doing so will add unnecessary barriers to one
of the two paths that share set_pte_at().

> > 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. 

I was under impression switching back from interrupt context is a
full barrier (otherwise wouldn't we be vulnerable to some side
channel attacks?), so the reader side wouldn't need explicit rmb.