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

[John Hubbard <jhubbard@nvidia.com>]

On 10/2/19 2:24 AM, Jan Kara wrote:
> 
> OK, so you are concerned that the page table walk of pgd->p4d->pud->pmd
> will get prefetched by the CPU before interrupts are disabled, somewhere in
> the middle of the walk IPI flushing TLBs on the cpu will happen allowing
> munmap() on another CPU to proceed and free page tables so the rest of the
> walk will happen on freed and possibly reused pages. Do I understand you
> right?
> 

Yes, that's it.

thanks,
-- 
John Hubbard
NVIDIA


> Realistically, I don't think this can happen as I'd expect the CPU to throw
> away the speculation state on interrupt. But that's just my expectation and
> I agree that I don't see anything in Documentation/memory-barriers.txt that
> would prevent what you are concerned about. Let's ask Paul :)
> 
> Paul, we are discussing here a possible races between
> mm/gup.c:__get_user_pages_fast() and mm/mmap.c:unmap_region(). The first
> has a code like:
> 
> local_irq_save(flags);
> load pgd from current->mm
> load p4d from pgd
> load pud from p4d
> load pmd from pud
> ...
> local_irq_restore(flags);
> 
> while the second has code like:
> 
> unmap_region()
>    walk pgd
>      walk p4d
>        walk pud
>          walk pmd
>            clear ptes
>            flush tlb
>    free page tables
> 
> Now the serialization between these two relies on the fact that flushing
> TLBs from unmap_region() requires IPI to be served on each CPU so in naive
> understanding unmap_region() shouldn't be able to get to 'free unused page
> tables' part until __get_user_pages_fast() enables interrupts again. Now as
> John points out we don't see anything in Documentation/memory-barriers.txt
> that would actually guarantee this. So do we really need something like
> smp_rmb() after disabling interrupts in __get_user_pages_fast() or is the
> race John is concerned about impossible? Thanks!
> 
>> This one has a problem, because Documentation/memory-barriers.txt points out:
>>
>> INTERRUPT DISABLING FUNCTIONS
>> -----------------------------
>>
>> Functions that disable interrupts (ACQUIRE equivalent) and enable interrupts
>> (RELEASE equivalent) will act as compiler barriers only.  So if memory or I/O
>> barriers are required in such a situation, they must be provided from some
>> other means.
>>
>>
>> ...and so I'm suggesting that we need something approximately like this:
>>
>> diff --git a/mm/gup.c b/mm/gup.c
>> index 23a9f9c9d377..1678d50a2d8b 100644
>> --- a/mm/gup.c
>> +++ b/mm/gup.c
>> @@ -2415,7 +2415,9 @@ int get_user_pages_fast(unsigned long start, int nr_pages,
>>          if (IS_ENABLED(CONFIG_HAVE_FAST_GUP) &&
>>              gup_fast_permitted(start, end)) {
>>                  local_irq_disable();
>> +               smp_mb();
>>                  gup_pgd_range(addr, end, gup_flags, pages, &nr);
>> +               smp_mb();
>>                  local_irq_enable();
>>                  ret = nr;
> 
> 								Honza
>