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
