Re: [PATCH v6] mm, oom: Fix race when selecting process to kill

[dserrg <dserrg@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5193 bytes --]

(sorry for html)

Why do we even bother with locking?
Why not just merge my original patch? (The link is in Vladimir's message)
It provides much more elegant (and working!) solution for this problem.
David, how did you miss it in the first place?

Oh.. and by the way. I was hitting the same bug in other
while_each_thread loops in oom_kill.c. Anyway, goodluck ;)
14 нояб. 2013 г. 2:18 пользователь "Sameer Nanda" <snanda@chromium.org>
написал:

> The selection of the process to be killed happens in two spots:
> first in select_bad_process and then a further refinement by
> looking for child processes in oom_kill_process. Since this is
> a two step process, it is possible that the process selected by
> select_bad_process may get a SIGKILL just before oom_kill_process
> executes. If this were to happen, __unhash_process deletes this
> process from the thread_group list. This results in oom_kill_process
> getting stuck in an infinite loop when traversing the thread_group
> list of the selected process.
>
> Fix this race by adding a pid_alive check for the selected process
> with tasklist_lock held in oom_kill_process.
>
> Signed-off-by: Sameer Nanda <snanda@chromium.org>
> ---
>  include/linux/sched.h |  5 +++++
>  mm/oom_kill.c         | 34 +++++++++++++++++++++-------------
>  2 files changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index e27baee..8975dbb 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -2156,6 +2156,11 @@ extern bool current_is_single_threaded(void);
>  #define do_each_thread(g, t) \
>         for (g = t = &init_task ; (g = t = next_task(g)) != &init_task ; )
> do
>
> +/*
> + * Careful: while_each_thread is not RCU safe. Callers should hold
> + * read_lock(tasklist_lock) across while_each_thread loops.
> + */
> +
>  #define while_each_thread(g, t) \
>         while ((t = next_thread(t)) != g)
>
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 6738c47..0d1f804 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -412,31 +412,33 @@ void oom_kill_process(struct task_struct *p, gfp_t
> gfp_mask, int order,
>         static DEFINE_RATELIMIT_STATE(oom_rs, DEFAULT_RATELIMIT_INTERVAL,
>                                               DEFAULT_RATELIMIT_BURST);
>
> +       if (__ratelimit(&oom_rs))
> +               dump_header(p, gfp_mask, order, memcg, nodemask);
> +
> +       task_lock(p);
> +       pr_err("%s: Kill process %d (%s) score %d or sacrifice child\n",
> +               message, task_pid_nr(p), p->comm, points);
> +       task_unlock(p);
> +
> +       read_lock(&tasklist_lock);
> +
>         /*
>          * If the task is already exiting, don't alarm the sysadmin or kill
>          * its children or threads, just set TIF_MEMDIE so it can die
> quickly
>          */
> -       if (p->flags & PF_EXITING) {
> +       if (p->flags & PF_EXITING || !pid_alive(p)) {
>                 set_tsk_thread_flag(p, TIF_MEMDIE);
>                 put_task_struct(p);
> +               read_unlock(&tasklist_lock);
>                 return;
>         }
>
> -       if (__ratelimit(&oom_rs))
> -               dump_header(p, gfp_mask, order, memcg, nodemask);
> -
> -       task_lock(p);
> -       pr_err("%s: Kill process %d (%s) score %d or sacrifice child\n",
> -               message, task_pid_nr(p), p->comm, points);
> -       task_unlock(p);
> -
>         /*
>          * If any of p's children has a different mm and is eligible for
> kill,
>          * the one with the highest oom_badness() score is sacrificed for
> its
>          * parent.  This attempts to lose the minimal amount of work done
> while
>          * still freeing memory.
>          */
> -       read_lock(&tasklist_lock);
>         do {
>                 list_for_each_entry(child, &t->children, sibling) {
>                         unsigned int child_points;
> @@ -456,12 +458,17 @@ void oom_kill_process(struct task_struct *p, gfp_t
> gfp_mask, int order,
>                         }
>                 }
>         } while_each_thread(p, t);
> -       read_unlock(&tasklist_lock);
>
> -       rcu_read_lock();
>         p = find_lock_task_mm(victim);
> +
> +       /*
> +        * Since while_each_thread is currently not RCU safe, this unlock
> of
> +        * tasklist_lock may need to be moved further down if any
> additional
> +        * while_each_thread loops get added to this function.
> +        */
> +       read_unlock(&tasklist_lock);
> +
>         if (!p) {
> -               rcu_read_unlock();
>                 put_task_struct(victim);
>                 return;
>         } else if (victim != p) {
> @@ -487,6 +494,7 @@ void oom_kill_process(struct task_struct *p, gfp_t
> gfp_mask, int order,
>          * That thread will now get access to memory reserves since it has
> a
>          * pending fatal signal.
>          */
> +       rcu_read_lock();
>         for_each_process(p)
>                 if (p->mm == mm && !same_thread_group(p, victim) &&
>                     !(p->flags & PF_KTHREAD)) {
> --
> 1.8.4.1
>
>

[-- Attachment #2: Type: text/html, Size: 5852 bytes --]