linux-kernel - Re: [GIT PULL] KVM changes for Linux 5.6-rc2

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: <b90a886e-b320-43d3-b2b6-7032aac57abe@redhat.com>
Date:   Wed, 12 Feb 2020 20:19:25 +0100
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        KVM list <kvm@...r.kernel.org>,
        Oliver Upton <oupton@...gle.com>
Subject: Re: [GIT PULL] KVM changes for Linux 5.6-rc2

On 12/02/20 19:53, Linus Torvalds wrote:
> It doesn't even compile. Just in the patch itself - so this is not a
> merge issue, I see this:
> 
>           int (*skip_emulated_instruction)(struct kvm_vcpu *vcpu);
>   ..
>   @@ -1599,6 +1599,40 @@ static int skip_emulated_instruction(struct
> kvm_vcpu *vcpu)
>   ..
>   +static void vmx_skip_emulated_instruction(struct kvm_vcpu *vcpu)
>   +       return skip_emulated_instruction(vcpu);
>   ..
>   -       .skip_emulated_instruction = skip_emulated_instruction,
>   +       .skip_emulated_instruction = vmx_skip_emulated_instruction,
> 
> ie note how that vmx_skip_emulated_instruction() is a void function,
> and then you have
> 
>          return skip_emulated_instruction(vcpu);
> 
> in it, and you assign that garbage to ".skip_emulated_instruction"
> which is supposed to be returning 'int'.

Indeed I missed the warning.  Of course the return value is in %rax so,
despite the patch being shitty (it is), it is also true that it
*happens* to pass the corresponding unit test.

Not a particularly high bar to clear I admit, but enough to explain the
mistake and ensure it doesn't happen again; I have now added "ccflags-y
+= -Werror" to the KVM makefile.

> So this clearly never even got a _whiff_ of build-testing.

Oh come on.

> You're now on my shit-list, which means that I want to see only (a)
> pure fixes and (b) well-tested such. Nothing else will be pulled.

Fair enough, I removed the following patches from the pull request and
will resend:

 KVM: nVMX: Emulate MTF when performing instruction emulation
 KVM: nVMX: Rename nested_ept_get_cr3() to nested_ept_get_eptp()
 KVM: nVMX: Rename EPTP validity helper and associated variables
 KVM: nVMX: Drop unnecessary check on ept caps for execute-only
 KVM: Provide kvm_flush_remote_tlbs_common()
 KVM: MIPS: Drop flush_shadow_memslot() callback
 KVM: MIPS: Replace all the kvm_flush_remote_tlbs() references
 KVM: MIPS: Define arch-specific kvm_flush_remote_tlbs()

The first one is a bug fix, but since it's the one that caused all the
mess I guess it's not really a good idea to argue about it.

Paolo