lkml.org 
[lkml]   [2020]   [Nov]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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@google.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@google.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(&reg_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(&reg_lock);
}


Obviously, you would need to change reg_lock to mutex in the _reg()
function.

-- Steve


\
 
 \ /
  Last update: 2020-12-01 04:59    [W:0.084 / U:0.120 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site