linux-kernel - Re: [PATCH v2] x86/kvm: Disable KVM_ASYNC_PF_SEND

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <772A564B-3268-49F4-9AEA-CDA648F6131F@amacapital.net>
Date:   Tue, 7 Apr 2020 10:38:56 -0700
From:   Andy Lutomirski <luto@...capital.net>
To:     Vivek Goyal <vgoyal@...hat.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Andy Lutomirski <luto@...nel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>, X86 ML <x86@...nel.org>,
        kvm list <kvm@...r.kernel.org>, stable <stable@...r.kernel.org>
Subject: Re: [PATCH v2] x86/kvm: Disable KVM_ASYNC_PF_SEND_ALWAYS



> On Apr 7, 2020, at 10:21 AM, Vivek Goyal <vgoyal@...hat.com> wrote:
> 
> On Mon, Apr 06, 2020 at 01:42:28PM -0700, Andy Lutomirski wrote:
>> 
>>>> On Apr 6, 2020, at 1:32 PM, Andy Lutomirski <luto@...capital.net> wrote:
>>> 
>>> 
>>>> On Apr 6, 2020, at 1:25 PM, Peter Zijlstra <peterz@...radead.org> wrote:
>>>> 
>>>> On Mon, Apr 06, 2020 at 03:09:51PM -0400, Vivek Goyal wrote:
>>>>>> On Mon, Mar 09, 2020 at 09:22:15PM +0100, Peter Zijlstra wrote:
>>>>>>> On Mon, Mar 09, 2020 at 08:05:18PM +0100, Thomas Gleixner wrote:
>>>>>>>> Andy Lutomirski <luto@...nel.org> writes:
>>>>>>> 
>>>>>>>>> I'm okay with the save/restore dance, I guess.  It's just yet more
>>>>>>>>> entry crud to deal with architecture nastiness, except that this
>>>>>>>>> nastiness is 100% software and isn't Intel/AMD's fault.
>>>>>>>> 
>>>>>>>> And we can do it in C and don't have to fiddle with it in the ASM
>>>>>>>> maze.
>>>>>>> 
>>>>>>> Right; I'd still love to kill KVM_ASYNC_PF_SEND_ALWAYS though, even if
>>>>>>> we do the save/restore in do_nmi(). That is some wild brain melt. Also,
>>>>>>> AFAIK none of the distros are actually shipping a PREEMPT=y kernel
>>>>>>> anyway, so killing it shouldn't matter much.
>>>>> 
>>>>> It will be nice if we can retain KVM_ASYNC_PF_SEND_ALWAYS. I have another
>>>>> use case outside CONFIG_PREEMPT.
>>>>> 
>>>>> I am trying to extend async pf interface to also report page fault errors
>>>>> to the guest.
>>>> 
>>>> Then please start over and design a sane ParaVirt Fault interface. The
>>>> current one is utter crap.
>>> 
>>> Agreed. Don’t extend the current mechanism. Replace it.
>>> 
>>> I would be happy to review a replacement. I’m not really excited to review an extension of the current mess.  The current thing is barely, if at all, correct.
>> 
>> I read your patch. It cannot possibly be correct.  You need to decide what happens if you get a memory failure when guest interrupts are off. If this happens, you can’t send #PF, but you also can’t just swallow the error. The existing APF code is so messy that it’s not at all obvious what your code ends up doing, but I’m pretty sure it doesn’t do anything sensible, especially since the ABI doesn’t have a sensible option.
> 
> Hi Andy,
> 
> I am not familiar with this KVM code and trying to understand it. I think
> error exception gets queued and gets delivered at some point of time, even
> if interrupts are disabled at the time of exception. Most likely at the time
> of next VM entry.

I’ve read the code three or four times and I barely understand it. I’m not convinced the author understood it.  It’s spaghetti.

> 
> Whether interrupts are enabled or not check only happens before we decide
> if async pf protocol should be followed or not. Once we decide to
> send PAGE_NOT_PRESENT, later notification PAGE_READY does not check
> if interrupts are enabled or not. And it kind of makes sense otherwise
> guest process will wait infinitely to receive PAGE_READY.
> 
> I modified the code a bit to disable interrupt and wait 10 seconds (after
> getting PAGE_NOT_PRESENT message). And I noticed that error async pf
> got delivered after 10 seconds after enabling interrupts. So error
> async pf was not lost because interrupts were disabled.
> 
> Havind said that, I thought disabling interrupts does not mask exceptions.
> So page fault exception should have been delivered even with interrupts
> disabled. Is that correct? May be there was no vm exit/entry during
> those 10 seconds and that's why.

My point is that the entire async pf is nonsense. There are two types of events right now:

“Page not ready”:  normally this isn’t even visible to the guest — the guest just waits. With async pf, the idea is to try to tell the guest that a particular instruction would block and the guest should do something else instead. Sending a normal exception is a poor design, though: the guest may not expect this instruction to cause an exception. I think KVM should try to deliver an *interrupt* and, if it can’t, then just block the guest.

“Page ready”: this is a regular asynchronous notification just like, say, a virtio completion. It should be an ordinary interrupt.  Some in memory data structure should indicate which pages are ready.

“Page is malfunctioning” is tricky because you *must* deliver the event. x86’s #MC is not exactly a masterpiece, but it does kind of work.

> 
> Thanks
> Vivek
>