On Mon, Jul 24, 2017 at 09:23:32AM +0200, Michal Hocko wrote:
> From: Michal Hocko <mho...@suse.com>
>
> David has noticed that the oom killer might kill additional tasks while
> the exiting oom victim hasn't terminated yet because the oom_reaper marks
> the curent victim MMF_OOM_SKIP too early when mm->mm_users dropped down
> to 0. The race is as follows
>
> oom_reap_task do_exit
> exit_mm
> __oom_reap_task_mm
> mmput
> __mmput
> mmget_not_zero # fails
> exit_mmap # frees memory
> set_bit(MMF_OOM_SKIP)
>
> The victim is still visible to the OOM killer until it is unhashed.
I think this is a very minor problem, in the worst case you get a
false positive oom kill, and it requires a race condition for it to
happen. I wouldn't add mmap_sem in exit_mmap just for this considering
the mmget_not_zero is already enough to leave exit_mmap alone.
Could you first clarify these points then I'll understand better what
the above is about:
1) if exit_mmap runs for a long time with terabytes of RAM with
mmap_sem held for writing like your patch does, wouldn't then
oom_reap_task_mm fail the same way after a few tries on
down_read_trylock? Despite your patch got applied? Isn't that
simply moving the failure that leads to set_bit(MMF_OOM_SKIP) from
mmget_not_zero to down_read_trylock?
2) why isn't __oom_reap_task_mm returning different retvals in case
mmget_not_zero fails? What is the point to schedule_timeout
and retry MAX_OOM_REAP_RETRIES times if mmget_not_zero caused it to
return null as it can't do anything about such task anymore? Why
are we scheduling those RETRIES times if mm_users is 0?
3) if exit_mmap is freeing lots of memory already, why should there be
another OOM immediately? I thought oom reaper only was needed when
the task on the right column couldn't reach the final mmput to set
mm_users to 0. Why exactly is a problem that MMF_OOM_SKIP gets set
on the mm, if exit_mmap is already guaranteed to be running? Why
isn't the oom reaper happy to just stop in such case and wait it to
complete? exit_mmap doesn't even take the mmap_sem and it's running
in R state, how would it block in a way that requires the OOM
reaper to free memory from another process to complete?
4) how is it safe to overwrite a VM_FAULT_RETRY that returns without
mmap_sem and then the arch code will release the mmap_sem despite
it was already released by handle_mm_fault? Anonymous memory faults
aren't common to return VM_FAULT_RETRY but an userfault
can. Shouldn't there be a block that prevents overwriting if
VM_FAULT_RETRY is set below? (not only VM_FAULT_ERROR)
if (unlikely((current->flags & PF_KTHREAD) && !(ret & VM_FAULT_ERROR)
&& test_bit(MMF_UNSTABLE, &vma->vm_mm->flags)))
ret = VM_FAULT_SIGBUS;
Thanks,
Andrea
