lkml.org 
[lkml]   [2022]   [Mar]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v5] mm/oom_kill.c: futex: Close a race between do_exit and the oom_reaper
    From


    On 3/21/22 02:55, Michal Hocko wrote:
    > On Thu 17-03-22 21:36:21, Nico Pache wrote:
    >> The pthread struct is allocated on PRIVATE|ANONYMOUS memory [1] which can
    >> be targeted by the oom reaper. This mapping is used to store the futex
    >> robust list; the kernel does not keep a copy of the robust list and instead
    >> references a userspace address to maintain the robustness during a process
    >> death. A race can occur between exit_mm and the oom reaper that allows
    >> the oom reaper to free the memory of the futex robust list before the
    >> exit path has handled the futex death:
    >>
    >> CPU1 CPU2
    >> ------------------------------------------------------------------------
    >> page_fault
    >> out_of_memory
    >> do_exit "signal"
    >> wake_oom_reaper
    >> oom_reaper
    >> oom_reap_task_mm (invalidates mm)
    >> exit_mm
    >> exit_mm_release
    >> futex_exit_release
    >> futex_cleanup
    >> exit_robust_list
    >> get_user (EFAULT- can't access memory)
    >
    > I still think it is useful to be explicit about the consequences of the
    > EFAULT here. Did you want to mention that a failing get_user in this
    > path would result in a hang because nobody is woken up when the current
    > holder of the lock terminates.

    Sounds good! You make a good point-- We had that in all the other versions, but
    I forgot to include it in this commit log.
    >
    >> While in the oom reaper thread, we must handle the futex cleanup without
    >> sleeping. To achieve this, add the boolean `try` to futex_exit_begin().
    >> This will control weather or not we use a trylock. Also introduce
    >> try_futex_exit_release() which will utilize the trylock version of the
    >> futex_cleanup_begin(). Also call kthread_use_mm in this context to assure
    >> the get_user call in futex_cleanup() does not return with EFAULT.
    >
    > This alone is not sufficient. get_user can sleep in the #PF handler path
    > (e.g. by waiting for swap in). Or is there any guarantee that the page
    > is never swapped out? If we cannot rule out #PF then this is not a
    > viable way to address the problem I am afraid.>
    > Please also note that this all is done after mmap_lock has been already
    > taken so a page fault could deadlock on the mmap_lock.
    >
    I don't think we can guarantee that page is not swapped out. Good catch, I was
    concerned when I saw the 'might_fault' in get_user, but I wasn't fully sure of
    its consequences. I'm still learning the labyrinth that is the MM space, so
    thanks for the context!

    > The more I am thinking about this the more I am getting convinced that
    > we should rather approach this differently and skip over vmas which can
    > be holding the list. Have you considered this option?

    We've discussed it and it seems very doable, but we haven't attempted to
    implement it yet. I'll give it a shot and see what I can come up with!

    Thank you for your feedback and reviews :)
    -- Nico

    \
     
     \ /
      Last update: 2022-03-22 00:02    [W:2.356 / U:0.052 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site