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@xxxxxxxxxx>
Cc: Waiman Long <longman@xxxxxxxxxx>
Cc: Baoquan He <bhe@xxxxxxxxxx>
Cc: Christoph von Recklinghausen <crecklin@xxxxxxxxxx>
Cc: Don Dutile <ddutile@xxxxxxxxxx>
Cc: Herton R. Krzesinski <herton@xxxxxxxxxx>
Cc: David Rientjes <rientjes@xxxxxxxxxx>
Cc: Michal Hocko <mhocko@xxxxxxxx>
Cc: Andrea Arcangeli <aarcange@xxxxxxxxxx>
Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
Cc: <tglx@xxxxxxxxxxxxx>
Cc: <mingo@xxxxxxxxxx>
Cc: <dvhart@xxxxxxxxxxxxx>
Cc: <dave@xxxxxxxxxxxx>
Cc: <andrealmeid@xxxxxxxxxxxxx>
Cc: <peterz@xxxxxxxxxxxxx>
Co-developed-by: Joel Savitz <jsavitz@xxxxxxxxxx>
Signed-off-by: Joel Savitz <jsavitz@xxxxxxxxxx>
Signed-off-by: Nico Pache <npache@xxxxxxxxxx>
---
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
