Messages in this thread | | | Date | Tue, 8 Mar 2022 19:48:11 -0500 | Subject | Re: [PATCH v4] mm/oom_kill.c: futex: Don't OOM reap a process with a futex robust list | From | Waiman Long <> |
| |
On 3/8/22 19:25, Nico Pache wrote: > The pthread struct is allocated on PRIVATE|ANONYMOUS memory [1] which can > be targeted by the oom reaper. This mapping is also 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 clear the memory of the futex robust list before the > exit path has handled the futex death. > > Prevent the OOM reaper from concurrently reaping the mappings if the dying > process contains a robust_list. If the dying task_struct does not contain > a pointer in tsk->robust_list, we can assume there was either never one > setup for this task struct, or futex_cleanup has properly handled the > futex death and we can safely reap this memory. > > Reproducer: https://gitlab.com/jsavitz/oom_futex_reproducer > > [1] https://elixir.bootlin.com/glibc/latest/source/nptl/allocatestack.c#L370 > > Fixes: 212925802454 ("mm: oom: let oom_reap_task and exit_mmap run concurrently") > Cc: Rafael Aquini <aquini@redhat.com> > Cc: Waiman Long <longman@redhat.com> > Cc: Baoquan He <bhe@redhat.com> > Cc: Christoph von Recklinghausen <crecklin@redhat.com> > Cc: Don Dutile <ddutile@redhat.com> > Cc: Herton R. Krzesinski <herton@redhat.com> > Cc: David Rientjes <rientjes@google.com> > Cc: Michal Hocko <mhocko@suse.com> > Cc: Andrea Arcangeli <aarcange@redhat.com> > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: <tglx@linutronix.de> > Cc: <mingo@redhat.com> > Cc: <dvhart@infradead.org> > Cc: <dave@stgolabs.net> > Cc: <andrealmeid@collabora.com> > Cc: <peterz@infradead.org> > Co-developed-by: Joel Savitz <jsavitz@redhat.com> > Signed-off-by: Joel Savitz <jsavitz@redhat.com> > Signed-off-by: Nico Pache <npache@redhat.com> > --- > mm/oom_kill.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c > index 989f35a2bbb1..37af902494d8 100644 > --- a/mm/oom_kill.c > +++ b/mm/oom_kill.c > @@ -587,6 +587,25 @@ static bool oom_reap_task_mm(struct task_struct *tsk, struct mm_struct *mm) > goto out_unlock; > } > > + /* Don't reap a process holding a robust_list as the pthread > + * struct is allocated in userspace using PRIVATE | ANONYMOUS > + * memory which when reaped before futex_cleanup() can leave > + * the waiting process stuck. > + */ > +#ifdef CONFIG_FUTEX > + bool robust = false; > + > + robust = tsk->robust_list != NULL; > +#ifdef CONFIG_COMPAT > + robust |= tsk->compat_robust_list != NULL; > +#endif > + if (robust) { > + trace_skip_task_reaping(tsk->pid); > + pr_info("oom_reaper: skipping task as it contains a robust list"); > + goto out_finish; > + } > +#endif > + > trace_start_task_reaping(tsk->pid); > > /* failed to reap part of the address space. Try again later */
I believe it will be easier to read if you define a helper function like
#ifdef CONFIG_FUTEX static inline bool has_robust_list(struct task_struct *tsk) { bool robust = !!tsk->robust_list;
#ifdef CONFIG_COMPAT robust |= !!tsk->compat_robust_list #endif return robust; } #else static inline bool has_robust_list(struct task_struct *tsk) { return false; } #endif
Then you don't need any #if/endif in oom_reap_task_mm().
Cheers, Longman
| |