From: Shakeel Butt <shakeelb@google.com>
To: Johannes Weiner <hannes@cmpxchg.org>,
Michal Hocko <mhocko@kernel.org>,
David Rientjes <rientjes@google.com>,
Andrew Morton <akpm@linux-foundation.org>,
Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
Roman Gushchin <guro@fb.com>,
Linus Torvalds <torvalds@linux-foundation.org>
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
Shakeel Butt <shakeelb@google.com>,
syzbot+7fbbfa368521945f0e3d@syzkaller.appspotmail.com,
Michal Hocko <mhocko@suse.com>,
stable@kernel.org
Subject: [PATCH v3 1/2] mm, oom: fix use-after-free in oom_kill_process
Date: Mon, 21 Jan 2019 13:58:49 -0800
Message-ID: <20190121215850.221745-1-shakeelb@google.com> (raw)
Syzbot instance running on upstream kernel found a use-after-free bug
in oom_kill_process. On further inspection it seems like the process
selected to be oom-killed has exited even before reaching
read_lock(&tasklist_lock) in oom_kill_process(). More specifically the
tsk->usage is 1 which is due to get_task_struct() in oom_evaluate_task()
and the put_task_struct within for_each_thread() frees the tsk and
for_each_thread() tries to access the tsk. The easiest fix is to do
get/put across the for_each_thread() on the selected task.
Now the next question is should we continue with the oom-kill as the
previously selected task has exited? However before adding more
complexity and heuristics, let's answer why we even look at the
children of oom-kill selected task? The select_bad_process() has already
selected the worst process in the system/memcg. Due to race, the
selected process might not be the worst at the kill time but does that
matter? The userspace can use the oom_score_adj interface to prefer
children to be killed before the parent. I looked at the history but it
seems like this is there before git history.
Reported-by: syzbot+7fbbfa368521945f0e3d@syzkaller.appspotmail.com
Fixes: 6b0c81b3be11 ("mm, oom: reduce dependency on tasklist_lock")
Signed-off-by: Shakeel Butt <shakeelb@google.com>
Reviewed-by: Roman Gushchin <guro@fb.com>
Acked-by: Michal Hocko <mhocko@suse.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: David Rientjes <rientjes@google.com>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>
Cc: stable@kernel.org
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
---
Changelog since v2:
- N/A
Changelog since v1:
- Improved the commit message and added the Reported-by and Fixes tags.
mm/oom_kill.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 0930b4365be7..1a007dae1e8f 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -981,6 +981,13 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
* still freeing memory.
*/
read_lock(&tasklist_lock);
+
+ /*
+ * The task 'p' might have already exited before reaching here. The
+ * put_task_struct() will free task_struct 'p' while the loop still try
+ * to access the field of 'p', so, get an extra reference.
+ */
+ get_task_struct(p);
for_each_thread(p, t) {
list_for_each_entry(child, &t->children, sibling) {
unsigned int child_points;
@@ -1000,6 +1007,7 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
}
}
}
+ put_task_struct(p);
read_unlock(&tasklist_lock);
/*
--
2.20.1.321.g9e740568ce-goog
next reply index
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-21 21:58 Shakeel Butt [this message]
2019-01-21 21:58 ` [PATCH v3 2/2] mm, oom: remove 'prefer children over parent' heuristic Shakeel Butt
2019-01-21 21:58 ` Shakeel Butt
2019-01-22 2:41 ` Shakeel Butt
2019-01-22 2:41 ` Shakeel Butt
2019-01-22 8:52 ` Michal Hocko
2019-01-23 22:57 ` [PATCH v3 1/2] mm, oom: fix use-after-free in oom_kill_process Sasha Levin
2019-01-23 22:57 ` Sasha Levin
2019-01-23 23:14 ` Shakeel Butt
2019-01-23 23:14 ` Shakeel Butt
2019-01-23 23:35 ` Tetsuo Handa
2019-01-24 8:13 ` Greg KH
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190121215850.221745-1-shakeelb@google.com \
--to=shakeelb@google.com \
--cc=akpm@linux-foundation.org \
--cc=guro@fb.com \
--cc=hannes@cmpxchg.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=mhocko@kernel.org \
--cc=mhocko@suse.com \
--cc=penguin-kernel@i-love.sakura.ne.jp \
--cc=rientjes@google.com \
--cc=stable@kernel.org \
--cc=syzbot+7fbbfa368521945f0e3d@syzkaller.appspotmail.com \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Linux-mm Archive on lore.kernel.org
Archives are clonable:
git clone --mirror https://lore.kernel.org/linux-mm/0 linux-mm/git/0.git
# If you have public-inbox 1.1+ installed, you may
# initialize and index your mirror using the following commands:
public-inbox-init -V2 linux-mm linux-mm/ https://lore.kernel.org/linux-mm \
linux-mm@kvack.org
public-inbox-index linux-mm
Example config snippet for mirrors
Newsgroup available over NNTP:
nntp://nntp.lore.kernel.org/org.kvack.linux-mm
AGPL code for this site: git clone https://public-inbox.org/public-inbox.git