On Mon 24-06-19 14:26:31, Shakeel Butt wrote:
> The commit ef08e3b4981a ("[PATCH] cpusets: confine oom_killer to
> mem_exclusive cpuset") introduces a heuristic where a potential
> oom-killer victim is skipped if the intersection of the potential victim
> and the current (the process triggered the oom) is empty based on the
> reason that killing such victim most probably will not help the current
> allocating process. However the commit 7887a3da753e ("[PATCH] oom:
> cpuset hint") changed the heuristic to just decrease the oom_badness
> scores of such potential victim based on the reason that the cpuset of
> such processes might have changed and previously they might have
> allocated memory on mems where the current allocating process can
> allocate from.
>
> Unintentionally commit 7887a3da753e ("[PATCH] oom: cpuset hint")
> introduced a side effect as the oom_badness is also exposed to the
> user space through /proc/[pid]/oom_score, so, readers with different
> cpusets can read different oom_score of th same process.
>
> Later the commit 6cf86ac6f36b ("oom: filter tasks not sharing the same
> cpuset") fixed the side effect introduced by 7887a3da753e by moving the
> cpuset intersection back to only oom-killer context and out of
> oom_badness. However the combination of the commit ab290adbaf8f ("oom:
> make oom_unkillable_task() helper function") and commit 26ebc984913b
> ("oom: /proc/<pid>/oom_score treat kernel thread honestly")
> unintentionally brought back the cpuset intersection check into the
> oom_badness calculation function.
Thanks for this excursion into the history. I think it is very useful.
> Other than doing cpuset/mempolicy intersection from oom_badness, the
> memcg oom context is also doing cpuset/mempolicy intersection which is
> quite wrong and is caught by syzcaller with the following report:
>
> kasan: CONFIG_KASAN_INLINE enabled
> kasan: GPF could be caused by NULL-ptr deref or user memory access
> general protection fault: 0000 [#1] PREEMPT SMP KASAN
> CPU: 0 PID: 28426 Comm: syz-executor.5 Not tainted 5.2.0-rc3-next-20190607
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> RIP: 0010:__read_once_size include/linux/compiler.h:194 [inline]
> RIP: 0010:has_intersects_mems_allowed mm/oom_kill.c:84 [inline]
> RIP: 0010:oom_unkillable_task mm/oom_kill.c:168 [inline]
> RIP: 0010:oom_unkillable_task+0x180/0x400 mm/oom_kill.c:155
> Code: c1 ea 03 80 3c 02 00 0f 85 80 02 00 00 4c 8b a3 10 07 00 00 48 b8 00
> 00 00 00 00 fc ff df 4d 8d 74 24 10 4c 89 f2 48 c1 ea 03 <80> 3c 02 00 0f
> 85 67 02 00 00 49 8b 44 24 10 4c 8d a0 68 fa ff ff
> RSP: 0018:ffff888000127490 EFLAGS: 00010a03
> RAX: dffffc0000000000 RBX: ffff8880a4cd5438 RCX: ffffffff818dae9c
> RDX: 100000000c3cc602 RSI: ffffffff818dac8d RDI: 0000000000000001
> RBP: ffff8880001274d0 R08: ffff888000086180 R09: ffffed1015d26be0
> R10: ffffed1015d26bdf R11: ffff8880ae935efb R12: 8000000061e63007
> R13: 0000000000000000 R14: 8000000061e63017 R15: 1ffff11000024ea6
> FS: 00005555561f5940(0000) GS:ffff8880ae800000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000607304 CR3: 000000009237e000 CR4: 00000000001426f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000600
> Call Trace:
> oom_evaluate_task+0x49/0x520 mm/oom_kill.c:321
> mem_cgroup_scan_tasks+0xcc/0x180 mm/memcontrol.c:1169
> select_bad_process mm/oom_kill.c:374 [inline]
> out_of_memory mm/oom_kill.c:1088 [inline]
> out_of_memory+0x6b2/0x1280 mm/oom_kill.c:1035
> mem_cgroup_out_of_memory+0x1ca/0x230 mm/memcontrol.c:1573
> mem_cgroup_oom mm/memcontrol.c:1905 [inline]
> try_charge+0xfbe/0x1480 mm/memcontrol.c:2468
> mem_cgroup_try_charge+0x24d/0x5e0 mm/memcontrol.c:6073
> mem_cgroup_try_charge_delay+0x1f/0xa0 mm/memcontrol.c:6088
> do_huge_pmd_wp_page_fallback+0x24f/0x1680 mm/huge_memory.c:1201
> do_huge_pmd_wp_page+0x7fc/0x2160 mm/huge_memory.c:1359
> wp_huge_pmd mm/memory.c:3793 [inline]
> __handle_mm_fault+0x164c/0x3eb0 mm/memory.c:4006
> handle_mm_fault+0x3b7/0xa90 mm/memory.c:4053
> do_user_addr_fault arch/x86/mm/fault.c:1455 [inline]
> __do_page_fault+0x5ef/0xda0 arch/x86/mm/fault.c:1521
> do_page_fault+0x71/0x57d arch/x86/mm/fault.c:1552
> page_fault+0x1e/0x30 arch/x86/entry/entry_64.S:1156
> RIP: 0033:0x400590
> Code: 06 e9 49 01 00 00 48 8b 44 24 10 48 0b 44 24 28 75 1f 48 8b 14 24 48
> 8b 7c 24 20 be 04 00 00 00 e8 f5 56 00 00 48 8b 74 24 08 <89> 06 e9 1e 01
> 00 00 48 8b 44 24 08 48 8b 14 24 be 04 00 00 00 8b
> RSP: 002b:00007fff7bc49780 EFLAGS: 00010206
> RAX: 0000000000000001 RBX: 0000000000760000 RCX: 0000000000000000
> RDX: 0000000000000000 RSI: 000000002000cffc RDI: 0000000000000001
> RBP: fffffffffffffffe R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000075 R11: 0000000000000246 R12: 0000000000760008
> R13: 00000000004c55f2 R14: 0000000000000000 R15: 00007fff7bc499b0
> Modules linked in:
> ---[ end trace a65689219582ffff ]---
> RIP: 0010:__read_once_size include/linux/compiler.h:194 [inline]
> RIP: 0010:has_intersects_mems_allowed mm/oom_kill.c:84 [inline]
> RIP: 0010:oom_unkillable_task mm/oom_kill.c:168 [inline]
> RIP: 0010:oom_unkillable_task+0x180/0x400 mm/oom_kill.c:155
> Code: c1 ea 03 80 3c 02 00 0f 85 80 02 00 00 4c 8b a3 10 07 00 00 48 b8 00
> 00 00 00 00 fc ff df 4d 8d 74 24 10 4c 89 f2 48 c1 ea 03 <80> 3c 02 00 0f
> 85 67 02 00 00 49 8b 44 24 10 4c 8d a0 68 fa ff ff
> RSP: 0018:ffff888000127490 EFLAGS: 00010a03
> RAX: dffffc0000000000 RBX: ffff8880a4cd5438 RCX: ffffffff818dae9c
> RDX: 100000000c3cc602 RSI: ffffffff818dac8d RDI: 0000000000000001
> RBP: ffff8880001274d0 R08: ffff888000086180 R09: ffffed1015d26be0
> R10: ffffed1015d26bdf R11: ffff8880ae935efb R12: 8000000061e63007
> R13: 0000000000000000 R14: 8000000061e63017 R15: 1ffff11000024ea6
> FS: 00005555561f5940(0000) GS:ffff8880ae800000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000001b2f823000 CR3: 000000009237e000 CR4: 00000000001426f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000600
>
> The fix is to decouple the cpuset/mempolicy intersection check from
> oom_unkillable_task() and make sure cpuset/mempolicy intersection check
> is only done in the global oom context.
Thanks for the changelog update. This looks really great to me.
> Reported-by: syzbot+d0fc9d3c166bc5e4a94b@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Shakeel Butt <shakeelb@xxxxxxxxxx>
I think that VM_BUG_ON in has_intersects_mems_allowed is over protective
and it makes the rest of the code a bit more convoluted than necessary.
Is there any reason we just do the check and return true there? Btw.
has_intersects_mems_allowed sounds like a misnomer to me. It suggests
to be a more generic function while it has some memcg implications which
are not trivial to spot without digging deeper. I would go with
oom_cpuset_eligible or something along those lines.
Anyway
Acked-by: Michal Hocko <mhocko@xxxxxxxx>
> ---
> Changelog since v2:
> - Further divided the patch into two patches.
> - More cleaned version.
>
> Changelog since v1:
> - Divide the patch into two patches.
>
> fs/proc/base.c | 3 +--
> include/linux/oom.h | 1 -
> mm/oom_kill.c | 51 ++++++++++++++++++++++++++-------------------
> 3 files changed, 30 insertions(+), 25 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 5eacce5e924a..57b7a0d75ef5 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -532,8 +532,7 @@ static int proc_oom_score(struct seq_file *m, struct pid_namespace *ns,
> unsigned long totalpages = totalram_pages() + total_swap_pages;
> unsigned long points = 0;
>
> - points = oom_badness(task, NULL, totalpages) *
> - 1000 / totalpages;
> + points = oom_badness(task, totalpages) * 1000 / totalpages;
> seq_printf(m, "%lu\n", points);
>
> return 0;
> diff --git a/include/linux/oom.h b/include/linux/oom.h
> index b75104690311..c696c265f019 100644
> --- a/include/linux/oom.h
> +++ b/include/linux/oom.h
> @@ -108,7 +108,6 @@ static inline vm_fault_t check_stable_address_space(struct mm_struct *mm)
> bool __oom_reap_task_mm(struct mm_struct *mm);
>
> extern unsigned long oom_badness(struct task_struct *p,
> - const nodemask_t *nodemask,
> unsigned long totalpages);
>
> extern bool out_of_memory(struct oom_control *oc);
> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index e0cdcbd58b0b..9f91cb7036fb 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -64,6 +64,11 @@ int sysctl_oom_dump_tasks = 1;
> */
> DEFINE_MUTEX(oom_lock);
>
> +static inline bool is_memcg_oom(struct oom_control *oc)
> +{
> + return oc->memcg != NULL;
> +}
> +
> #ifdef CONFIG_NUMA
> /**
> * has_intersects_mems_allowed() - check task eligiblity for kill
> @@ -73,12 +78,18 @@ DEFINE_MUTEX(oom_lock);
> * Task eligibility is determined by whether or not a candidate task, @tsk,
> * shares the same mempolicy nodes as current if it is bound by such a policy
> * and whether or not it has the same set of allowed cpuset nodes.
> + *
> + * Only call in the global oom context (i.e. not in memcg oom). This function
> + * is assuming 'current' has triggered the oom-killer.
> */
> static bool has_intersects_mems_allowed(struct task_struct *start,
> - const nodemask_t *mask)
> + struct oom_control *oc)
> {
> struct task_struct *tsk;
> bool ret = false;
> + const nodemask_t *mask = oc->nodemask;
> +
> + VM_BUG_ON(is_memcg_oom(oc));
>
> rcu_read_lock();
> for_each_thread(start, tsk) {
> @@ -106,7 +117,7 @@ static bool has_intersects_mems_allowed(struct task_struct *start,
> }
> #else
> static bool has_intersects_mems_allowed(struct task_struct *tsk,
> - const nodemask_t *mask)
> + struct oom_control *oc)
> {
> return true;
> }
> @@ -146,24 +157,13 @@ static inline bool is_sysrq_oom(struct oom_control *oc)
> return oc->order == -1;
> }
>
> -static inline bool is_memcg_oom(struct oom_control *oc)
> -{
> - return oc->memcg != NULL;
> -}
> -
> /* return true if the task is not adequate as candidate victim task. */
> -static bool oom_unkillable_task(struct task_struct *p,
> - const nodemask_t *nodemask)
> +static bool oom_unkillable_task(struct task_struct *p)
> {
> if (is_global_init(p))
> return true;
> if (p->flags & PF_KTHREAD)
> return true;
> -
> - /* p may not have freeable memory in nodemask */
> - if (!has_intersects_mems_allowed(p, nodemask))
> - return true;
> -
> return false;
> }
>
> @@ -190,19 +190,17 @@ static bool is_dump_unreclaim_slabs(void)
> * oom_badness - heuristic function to determine which candidate task to kill
> * @p: task struct of which task we should calculate
> * @totalpages: total present RAM allowed for page allocation
> - * @nodemask: nodemask passed to page allocator for mempolicy ooms
> *
> * The heuristic for determining which task to kill is made to be as simple and
> * predictable as possible. The goal is to return the highest value for the
> * task consuming the most memory to avoid subsequent oom failures.
> */
> -unsigned long oom_badness(struct task_struct *p,
> - const nodemask_t *nodemask, unsigned long totalpages)
> +unsigned long oom_badness(struct task_struct *p, unsigned long totalpages)
> {
> long points;
> long adj;
>
> - if (oom_unkillable_task(p, nodemask))
> + if (oom_unkillable_task(p))
> return 0;
>
> p = find_lock_task_mm(p);
> @@ -313,7 +311,11 @@ static int oom_evaluate_task(struct task_struct *task, void *arg)
> struct oom_control *oc = arg;
> unsigned long points;
>
> - if (oom_unkillable_task(task, oc->nodemask))
> + if (oom_unkillable_task(task))
> + goto next;
> +
> + /* p may not have freeable memory in nodemask */
> + if (!is_memcg_oom(oc) && !has_intersects_mems_allowed(task, oc))
> goto next;
>
> /*
> @@ -337,7 +339,7 @@ static int oom_evaluate_task(struct task_struct *task, void *arg)
> goto select;
> }
>
> - points = oom_badness(task, oc->nodemask, oc->totalpages);
> + points = oom_badness(task, oc->totalpages);
> if (!points || points < oc->chosen_points)
> goto next;
>
> @@ -385,7 +387,11 @@ static int dump_task(struct task_struct *p, void *arg)
> struct oom_control *oc = arg;
> struct task_struct *task;
>
> - if (oom_unkillable_task(p, oc->nodemask))
> + if (oom_unkillable_task(p))
> + return 0;
> +
> + /* p may not have freeable memory in nodemask */
> + if (!is_memcg_oom(oc) && !has_intersects_mems_allowed(p, oc))
> return 0;
>
> task = find_lock_task_mm(p);
> @@ -1085,7 +1091,8 @@ bool out_of_memory(struct oom_control *oc)
> check_panic_on_oom(oc, constraint);
>
> if (!is_memcg_oom(oc) && sysctl_oom_kill_allocating_task &&
> - current->mm && !oom_unkillable_task(current, oc->nodemask) &&
> + current->mm && !oom_unkillable_task(current) &&
> + has_intersects_mems_allowed(current, oc) &&
> current->signal->oom_score_adj != OOM_SCORE_ADJ_MIN) {
> get_task_struct(current);
> oc->chosen = current;
> --
> 2.22.0.410.gd8fdbe21b5-goog
--
Michal Hocko
SUSE Labs
