Hi Al, On Sat, Sep 23, 2017 at 02:40:21PM +0100, Al Viro wrote: > Calling get_user_pages_fast() while holding ->mmap_sem is > asking for trouble: > > CPU1: r4k_flush_cache_sigtramp() > down_read(¤t->mm->mmap_sem); > > CPU2: (running thread with the same ->mm): sys_pkey_alloc() > down_write(¤t->mm->mmap_sem); > > CPU1: > pages = get_user_pages_fast(addr, 1, 0, &args.page); > which hits an absent page and goto slow. Then it goes on to > ret = get_user_pages_unlocked(start, (end - start) >> PAGE_SHIFT, > pages, write ? FOLL_WRITE : 0); > which does > return __get_user_pages_unlocked(current, current->mm, start, nr_pages, > pages, gup_flags | FOLL_TOUCH); > which does > down_read(&mm->mmap_sem); > ret = __get_user_pages_locked(tsk, mm, start, nr_pages, pages, NULL, > &locked, false, gup_flags); > > and we have a classical deadlock on recursive down_read() (thread 1: down_read() > gets the rwsem; thread 2: down_write() blocks waiting for thread 1 to release > it; thread 1: down_read() blocks waiting for thread 2 to get through down_write() > and eventual up_write(), which completes the deadlock). Hmm, indeed. Thanks for spotting that one. > Replacing pkey_alloc(2) with e.g. mmap(2) turns that from hard deadlock into > something killable, but while "with bad timing you might get process stuck > hard" is worse than "with bad timing you might get process stuck until you > kill -9 it", neither is a good thing. > > I'm not familiar enough with arch/mips guts to suggest any variant of solution; > replacing get_user_pages_fast() with get_user_pages_locked() would solve the > deadlock, but that loses the fast path; not taking ->mmap_sem there have > local_r4k_flush_cache_sigtramp() run without fcs_args->mm being locked, which > might or might not be a problem. Suggestions? I suspect my concern was the theoretical situation where: task1 hits an FPU branch instruction that needs emulating this creates a trampoline above the stack in user memory to execute the delay slot instruction, and calls r4k_flush_cache_sigtramp() task2 in same process changes the memory mapping somehow. SMP call to local_r4k_flush_cache_sigtramp() it may do cache op using the user virtual address when same mm active, or using kmap otherwise. For the kmap case (SMP call to CPU with different process executing) its holding a reference to the page so it should be fine. If the page was swapped out or otherwise copied and it icache flushed the old page, then I think the new page would get flushed when swapped back in so thats fine. Otherwise I can't imagine a situation where it'd misbehave unless userland explicitly remapped the memory and it accessed using the virtual address, in which case its asking for trouble. Therefore I suspect it should be safe to drop the mmap_sem, but thats OTOH. Have I missed anything important? Thanks James
Attachment:
signature.asc
Description: Digital signature
