lkml.org 
[lkml]   [2017]   [Feb]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH V2 1/2] exec: don't wait for zombie threads with cred_guard_mutex held
    Eric,

    Thanks for looking into this! and sorry for delay.

    On 02/17, Eric W. Biederman wrote:
    >
    > Oleg Nesterov <oleg@redhat.com> writes:
    >
    > > - In any case we should limit the scope of cred_guard_mutex in execve paths.
    > > It is not clear why do we take it at the start of execve, and worse, it is
    > > not clear why we do we actually overload this mutex to exclude other threads
    > > (except check_unsafe_exec() but this is solveable). The original motivation
    > > was signal->in_exec_mm protection but this idea was replaced by 3c77f8457221
    > > ("exec: make argv/envp memory visible to oom-killer"). It is just ugly to
    > > call copy_strings/etc with this mutex held.
    >
    >
    > The original changes that introduced cred_guard_mutex are:
    > a6f76f23d297 ("CRED: Make execve() take advantage of copy-on-write credentials")
    > d84f4f992cbd ("CRED: Inaugurate COW credentials")
    >
    > So I don't think you actually have your history right.
    >
    > Beyond that there is a compelling reason to have exec appear atomic from
    > the perspective of ptrace_attach. If the operation is a setuid exec
    > and the tracer does not have permission to trace the original or the
    > result of the exec there could be some significant information leakage
    > if the exec operation is not atomic from the perspective of
    > ptrace_attach.

    Yes sure.

    But I meant execve() should not take cred_guard_mutex at the start, it
    should take it later even if we do not rework the security hooks. At least
    it should take it after copy_strings(), but probably this needs some work.

    > Additionally your comment makes me nervous when you are wondering why we
    > take this mutex to exclude other threads and I look in the git history
    > and see:
    >
    > commit 9b1bf12d5d51bca178dea21b04a0805e29d60cf1
    > Author: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
    > Date: Wed Oct 27 15:34:08 2010 -0700
    >
    > signals: move cred_guard_mutex from task_struct to signal_struct
    >
    > Oleg Nesterov pointed out we have to prevent multiple-threads-inside-exec
    > itself and we can reuse ->cred_guard_mutex for it. Yes, concurrent
    > execve() has no worth.
    >
    > Let's move ->cred_guard_mutex from task_struct to signal_struct. It
    > naturally prevent multiple-threads-inside-exec.

    Yes, and let me explain the original motivation for this change.

    To remind, we had a problem with copy_strings() which can use a lot of
    memory, and this memory was not visible to OOM-killer.

    So we were going to add the new member,

    signal_struct->in_exec_mm = bprm->mm

    and change OOM-killer to account both task->mm and task->signal->in_exec_mm.

    And in this case we obviously need to ensure that only one thread
    can enter exec and use signal_struct->in_exec_mm.

    That patch was ready, but then we found another (better) solution:
    3c77f8457221 ("exec: make argv/envp memory visible to oom-killer").

    So I do not think we need to exclude other threads today, and we do
    not need to hold cred_guard_mutex throughout the whole execve path.

    Again, this needs some work. For example check_unsafe_exec() assumes
    it can't race with another thread, see 9e00cdb091b008cb3c78192651180
    "exec:check_unsafe_exec: kill the dead -EAGAIN and clear_in_exec logic".
    But this looks solvable.


    > So while I fully agree we have issues here that we need to address and
    > fix your patch description does not inspire confidence.

    See above... what do you think I should change in this part of changelog?

    Thanks,

    Oleg.

    \
     
     \ /
      Last update: 2017-02-20 16:25    [W:4.136 / U:0.700 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site