Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   ebiederm@xmission.com (Eric W. Biederman)
To:     Linus Torvalds <torvalds@linux-foundation.org>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Oleg Nesterov <oleg@redhat.com>, Jann Horn <jannh@google.com>,
        Kees Cook <keescook@chromium.org>,
        Greg Ungerer <gerg@linux-m68k.org>,
        Rob Landley <rob@landley.net>,
        Bernd Edlinger <bernd.edlinger@hotmail.de>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        Al Viro <viro@zeniv.linux.org.uk>,
        Alexey Dobriyan <adobriyan@gmail.com>,
        Andrew Morton <akpm@linux-foundation.org>
References: <87h7wujhmz.fsf@x220.int.ebiederm.org>
        <87sgga6ze4.fsf@x220.int.ebiederm.org>
        <87blmy6zay.fsf_-_@x220.int.ebiederm.org>
        <CAHk-=wguq6FwYb8_WZ_ZOxpHtwyc0xpz+PitNuf4pVxjWFmjFQ@mail.gmail.com>
Date:   Sat, 09 May 2020 14:57:33 -0500
In-Reply-To: <CAHk-=wguq6FwYb8_WZ_ZOxpHtwyc0xpz+PitNuf4pVxjWFmjFQ@mail.gmail.com>
        (Linus Torvalds's message of "Sat, 9 May 2020 12:18:06 -0700")
Message-ID: <87k11kyj82.fsf@x220.int.ebiederm.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PATCH 3/6] exec: Stop open coding mutex_lock_killable of cred_guard_mutex
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Fri, May 8, 2020 at 11:48 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
>>
>>
>> Oleg modified the code that did
>> "mutex_lock_interruptible(&current->cred_guard_mutex)" to return
>> -ERESTARTNOINTR instead of -EINTR, so that userspace will never see a
>> failure to grab the mutex.
>>
>> Slightly earlier Liam R. Howlett defined mutex_lock_killable for
>> exactly the same situation but it does it a little more cleanly.
>
> What what what?
>
> None of this makes sense. Your commit message is completely wrong, and
> the patch is utter shite.
>
> mutex_lock_interruptible() and mutex_lock_killable() are completely
> different operations, and the difference has absolutely nothing to do
> with  -ERESTARTNOINTR or -EINTR.
>
> mutex_lock_interruptible() is interrupted by any signal.
>
> mutex_lock_killable() is - surprise surprise - only interrupted by
> SIGKILL (in theory any fatal signal, but we never actually implemented
> that logic, so it's only interruptible by the known-to-always-be-fatal
> SIGKILL).
>
>> Switch the code to mutex_lock_killable so that it is clearer what the
>> code is doing.
>
> This nonsensical patch makes me worry about all your other patches.
> The explanation is wrong, the patch is wrong, and it changes things to
> be fundamentally broken.
>
> Before this, ^C would break out of a blocked execve()/ptrace()
> situation. After this patch, you need special tools to do so.
>
> This patch is completely wrong.

Sigh.  Brain fart on my part. You are correct.

I saw the restart, and totally forgot that it allows the handling of a
signal before restarting the system call.

Except for the handling of the signal in userspace it is the same as
mutex_lock_killable but that is a big big big if.

My apologies.  I will drop this patch.

Eric