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:     Paolo Bonzini <pbonzini@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Oleg Nesterov <oleg@redhat.com>,
        Al Viro <viro@zeniv.linux.org.uk>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        KVM list <kvm@vger.kernel.org>
References: <20210923181252.44385-1-pbonzini@redhat.com>
        <CAHk-=wjp7psdNc8KpxVDmcVYaAAxDUvvFTgx21OwZJzkghktLg@mail.gmail.com>
Date:   Fri, 24 Sep 2021 10:13:00 -0500
In-Reply-To: <CAHk-=wjp7psdNc8KpxVDmcVYaAAxDUvvFTgx21OwZJzkghktLg@mail.gmail.com>
        (Linus Torvalds's message of "Thu, 23 Sep 2021 11:35:06 -0700")
Message-ID: <87r1deqa6b.fsf@disp2133>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [GIT PULL] KVM/rseq changes for Linux 5.15-rc3
Precedence: bulk

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

> On Thu, Sep 23, 2021 at 11:13 AM Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> A fix for a bug with restartable sequences and KVM.  KVM's handling
>> of TIF_NOTIFY_RESUME, e.g. for task migration, clears the flag without
>> informing rseq and leads to stale data in userspace's rseq struct.
>
> Ok, patches look reasonable.
>
>> I'm sending this as a separate pull request since it's not code
>> that I usually touch.  In particular, patch 2 ("entry: rseq: Call
>> rseq_handle_notify_resume() in tracehook_notify_resume()") is just a
>> cleanup to try and make future bugs less likely.  If you prefer this to
>> be sent via Thomas and only in 5.16, please speak up.
>
> So I took the pull request this way, thanks for separating it like this.
>
> But I'm adding a few people to the cc for a completely different
> reason: the cleanup to move all the notify_resume stuff to
> tracehook_notify_resume() is good, but it does make me go - once again
> - "Hmm, that naming is really really bad".
>
> The <linux/tracehook.h> code was literally meant for tracing. It's
> where the name comes from, and it's the original intent: having a
> place that you can hook into for tracing that doesn't depend on how
> the core kernel code ends up changing.
>
> But that's not how it actually acts right now. That header file is now
> some very core functionality, and little of it is actually related to
> tracing any more. It's more core process state handling for the user
> space return path.

Yes.  The tracehook header was a precursor to merging utrace which
ultimately was replaced by uprobes.  Quite a few of the tracehooks hooks
have become regular ptrace hooks over the years, and left tracehook.h

It looks like that is the path that should happen with the rest of the
hooks as well.

It looks like: tracehook_report_syscall_entry, and
tracehook_report_syscall_exit should just become
ptrace_report_syscall_entry and ptrace_report_syscall_exit.

That tracehook_signal_handler should just be inlined into it's one
caller.

That leaves set_notify_resume, tracehook_notify_resume,
tracehook_notify_signal, and set_notify_signal.

I am still waiting to hear if we can just remove
tracehook_notify_signal now that io_uring has become an ordinary process
thing.

It looks like tracehook_notify_resume should be renamed and put
somewhere I just don't know where.

The config option HAVE_ARCH_TRACEHOOK appears to have nothing to do
with the header tracehook.h any more.  It looks to be just about
regsets, and task_current_syscall.  It looks like only alpha, h8300,
m68k, and microblaze need an implementation and then we can make all
of the code that depends upon HAVE_ARCH_TRACEHOOK unconditional.


> So I don't object to the patches, and they are merged, but I'm cc'ing people to
>
>  (a) let them know about this (see commit a68de80f61f6: "entry: rseq:
> Call rseq_handle_notify_resume() in tracehook_notify_resume()" in the
> current -git tree)
>
>  (b) possibly prod some people into perhaps moving/renaming some of
> that code to actual core kernel C files, instead of a misnamed header
> file..
>
> Hmm?

It is on my radar.  Does anyone have any idea what to call
tracehook_notify_resume so that it describes it's current usage?

Eric