lkml.org 
[lkml]   [2020]   [Dec]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    Subject[PATCH] perf: Break deadlock involving exec_update_mutex
    On Mon, Dec 07, 2020 at 10:40:11AM -0800, Linus Torvalds wrote:
    > On Mon, Dec 7, 2020 at 1:10 AM Peter Zijlstra <peterz@infradead.org> wrote:
    > >
    > > > PeterZ, is there something I'm missing?
    > >
    > > Like this?
    > >
    > > https://lkml.kernel.org/r/20200828123720.GZ1362448@hirez.programming.kicks-ass.net
    >
    > Yes, except I think you should remove the old ptrace_may_access() check.

    > I don't see any point at all in checking privileges twice, and I do
    > see real downsides. Not just that KCSAN issue, but also lack of
    > coverage (ie the second check will then effectively never be tested,
    > which is bad too).

    Fair enough, find below.

    I suppose I'll queue the below into tip/perf/core for next merge window,
    unless you want it in a hurry?

    ---
    Subject: perf: Break deadlock involving exec_update_mutex
    From: Peter Zijlstra <peterz@infradead.org>
    Date: Fri, 28 Aug 2020 14:37:20 +0200

    Syzbot reported a lock inversion involving perf. The sore point being
    perf holding exec_update_mutex() for a very long time, specifically
    across a whole bunch of filesystem ops in pmu::event_init() (uprobes)
    and anon_inode_getfile().

    This then inverts against procfs code trying to take
    exec_update_mutex.

    Move the permission checks later, such that we need to hold the mutex
    over less code.

    Reported-by: syzbot+db9cdf3dd1f64252c6ef@syzkaller.appspotmail.com
    Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
    ---
    kernel/events/core.c | 46 +++++++++++++++++++++++-----------------------
    1 file changed, 23 insertions(+), 23 deletions(-)

    --- a/kernel/events/core.c
    +++ b/kernel/events/core.c
    @@ -11832,24 +11832,6 @@ SYSCALL_DEFINE5(perf_event_open,
    goto err_task;
    }

    - if (task) {
    - err = mutex_lock_interruptible(&task->signal->exec_update_mutex);
    - if (err)
    - goto err_task;
    -
    - /*
    - * Preserve ptrace permission check for backwards compatibility.
    - *
    - * We must hold exec_update_mutex across this and any potential
    - * perf_install_in_context() call for this new event to
    - * serialize against exec() altering our credentials (and the
    - * perf_event_exit_task() that could imply).
    - */
    - err = -EACCES;
    - if (!perfmon_capable() && !ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS))
    - goto err_cred;
    - }
    -
    if (flags & PERF_FLAG_PID_CGROUP)
    cgroup_fd = pid;

    @@ -11857,7 +11839,7 @@ SYSCALL_DEFINE5(perf_event_open,
    NULL, NULL, cgroup_fd);
    if (IS_ERR(event)) {
    err = PTR_ERR(event);
    - goto err_cred;
    + goto err_task;
    }

    if (is_sampling_event(event)) {
    @@ -11976,6 +11958,24 @@ SYSCALL_DEFINE5(perf_event_open,
    goto err_context;
    }

    + if (task) {
    + err = mutex_lock_interruptible(&task->signal->exec_update_mutex);
    + if (err)
    + goto err_file;
    +
    + /*
    + * Preserve ptrace permission check for backwards compatibility.
    + *
    + * We must hold exec_update_mutex across this and any potential
    + * perf_install_in_context() call for this new event to
    + * serialize against exec() altering our credentials (and the
    + * perf_event_exit_task() that could imply).
    + */
    + err = -EACCES;
    + if (!perfmon_capable() && !ptrace_may_access(task, PTRACE_MODE_READ_REALCREDS))
    + goto err_cred;
    + }
    +
    if (move_group) {
    gctx = __perf_event_ctx_lock_double(group_leader, ctx);

    @@ -12151,7 +12151,10 @@ SYSCALL_DEFINE5(perf_event_open,
    if (move_group)
    perf_event_ctx_unlock(group_leader, gctx);
    mutex_unlock(&ctx->mutex);
    -/* err_file: */
    +err_cred:
    + if (task)
    + mutex_unlock(&task->signal->exec_update_mutex);
    +err_file:
    fput(event_file);
    err_context:
    perf_unpin_context(ctx);
    @@ -12163,9 +12166,6 @@ SYSCALL_DEFINE5(perf_event_open,
    */
    if (!event_file)
    free_event(event);
    -err_cred:
    - if (task)
    - mutex_unlock(&task->signal->exec_update_mutex);
    err_task:
    if (task)
    put_task_struct(task);
    \
     
     \ /
      Last update: 2020-12-08 09:37    [W:4.902 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site