[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20201130225754.38a8d717@oasis.local.home>
Date: Mon, 30 Nov 2020 22:57:54 -0500
From: Steven Rostedt <rostedt@...dmis.org>
To: Axel Rasmussen <axelrasmussen@...gle.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
Chinwen Chang <chinwen.chang@...iatek.com>,
Daniel Jordan <daniel.m.jordan@...cle.com>,
David Rientjes <rientjes@...gle.com>,
Davidlohr Bueso <dbueso@...e.de>,
Ingo Molnar <mingo@...hat.com>, Jann Horn <jannh@...gle.com>,
Laurent Dufour <ldufour@...ux.ibm.com>,
Michel Lespinasse <walken@...gle.com>,
Stephen Rothwell <sfr@...b.auug.org.au>,
Vlastimil Babka <vbabka@...e.cz>,
Yafang Shao <laoar.shao@...il.com>, davem@...emloft.net,
dsahern@...nel.org, gregkh@...uxfoundation.org, kuba@...nel.org,
liuhangbin@...il.com, tj@...nel.org, linux-kernel@...r.kernel.org,
linux-mm@...ck.org
Subject: Re: [PATCH] mm: mmap_lock: fix use-after-free race and css ref leak
in tracepoints
On Mon, 30 Nov 2020 15:35:04 -0800
Axel Rasmussen <axelrasmussen@...gle.com> wrote:
> syzbot reported[1] a use-after-free introduced in 0f818c4bc1f3. The bug
> is that an ongoing trace event might race with the tracepoint being
> disabled (and therefore the _unreg() callback being called). Consider
> this ordering:
>
> T1: trace event fires, get_mm_memcg_path() is called
> T1: get_memcg_path_buf() returns a buffer pointer
> T2: trace_mmap_lock_unreg() is called, buffers are freed
> T1: cgroup_path() is called with the now-freed buffer
>
> The solution in this commit is to modify trace_mmap_lock_unreg() to
> first stop new buffers from being handed out, and then to wait (spin)
> until any existing buffer references are dropped (i.e., those trace
> events complete).
>
> I have a simple reproducer program which spins up two pools of threads,
> doing the following in a tight loop:
>
> Pool 1:
> mmap(NULL, 4096, PROT_READ | PROT_WRITE,
> MAP_PRIVATE | MAP_ANONYMOUS, -1, 0)
> munmap()
>
> Pool 2:
> echo 1 > /sys/kernel/debug/tracing/events/mmap_lock/enable
> echo 0 > /sys/kernel/debug/tracing/events/mmap_lock/enable
>
> This triggers the use-after-free very quickly. With this patch, I let it
> run for an hour without any BUGs.
>
> While fixing this, I also noticed and fixed a css ref leak. Previously
> we called get_mem_cgroup_from_mm(), but we never called css_put() to
> release that reference. get_mm_memcg_path() now does this properly.
>
> [1]: https://syzkaller.appspot.com/bug?extid=19e6dd9943972fa1c58a
>
> Fixes: 0f818c4bc1f3 ("mm: mmap_lock: add tracepoints around lock acquisition")
> Signed-off-by: Axel Rasmussen <axelrasmussen@...gle.com>
>
Looking at the original patch that this fixes, I'm thinking, why use a
spinlock in the reg/unreg callers? Registering and unregistering a
tracepoint can sleep (it calls mutex locks), so the reg/unreg can sleep
too. As the use of the get_mm_memcg_path() is done under
preempt_disable, which is a rcu grace period, you could simply change
the unregister to:
void trace_mmap_lock_unreg(void)
{
int cpu;
mutex_lock(®_lock);
if (--reg_refcount)
goto out;
/* Make sure all users of memcg_path_buf are done */
synchronize_rcu();
for_each_possible_cpu(cpu) {
kfree(per_cpu(memcg_path_buf, cpu));
}
out:
mutex_unlock(®_lock);
}
Obviously, you would need to change reg_lock to mutex in the _reg()
function.
-- Steve
Powered by blists - more mailing lists