linux-kernel - Re: [PATCH 00/10] KVM: Avoid literal numbers as return values

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: <7eb84037-303c-4218-aeaa-2d08ef0b3267@suse.com>
Date: Fri, 5 Dec 2025 16:47:42 +0100
From: Jürgen Groß <jgross@...e.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: linux-kernel@...r.kernel.org, kvm@...r.kernel.org, x86@...nel.org,
 linux-coco@...ts.linux.dev, Paolo Bonzini <pbonzini@...hat.com>,
 Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
 Borislav Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>,
 "H. Peter Anvin" <hpa@...or.com>, Kiryl Shutsemau <kas@...nel.org>,
 Rick Edgecombe <rick.p.edgecombe@...el.com>,
 Vitaly Kuznetsov <vkuznets@...hat.com>, David Woodhouse
 <dwmw2@...radead.org>, Paul Durrant <paul@....org>
Subject: Re: [PATCH 00/10] KVM: Avoid literal numbers as return values

On 05.12.25 15:16, Sean Christopherson wrote:
> On Fri, Dec 05, 2025, Juergen Gross wrote:
>> This series is the first part of replacing the use of literal numbers
>> (0 and 1) as return values with either true/false or with defines.
> 
> Sorry, but NAK to using true/false.  IMO, it's far worse than 0/1.  At least 0/1
> draws from the kernel's 0/-errno approach.  With booleans, the polarity is often
> hard to discern without a priori knowledge of the pattern, and even then it can
> be confusing.  E.g. for me, returning "true" when .set_{c,d}r() fails is unexpected,
> and results in unintuitive code like this:
> 
>                  if (!kvm_dr6_valid(val))
> 			return true;

I don't see "return 1;" being much better here.

> For isolated APIs whose values aren't intented to be propagated back up to the
> .handle_exit() call site, I would much rather return 0/-EINVAL.

Fine with me (I agree this would be more readable).

> Do you have a sketch of what the end goal/result will look like?  IIRC, last time
> anyone looked at doing this (which was a few years ago, but I don't think KVM has
> changed _that_ much), we backed off because a partial conversion would leave KVM
> in an unwieldy and somewhat scary state.

In the end I'd like to get rid of most "return 1;" and several "return 0;"
instances in KVM.

The main reason is that it is sometimes very hard to determine what the
current "return 1" is meant to say ("error" or "return to guest" or just
"okay"). This is especially true in some of the low level MSR emulation
code, e.g. in kvm_pmu_get_msr(): only after examining the call paths I was
sure the "return 0" wasn't meant to return to qemu, but to indicate success.

I have already started to replace the "return 1;" instances in the exit
handlers with "return KVM_RET_GUEST;", but the MSR emulation code convinced
me to analyze it first and to clear it up before changing any of its "1"
return values by accident to "KVM_RET_GUEST".

In the end my plan is to cover all archs to replace the literal "1"s with
"KVM_RET_GUEST" where appropriate, and as many other literal "1"s as possible
with more meaningful defines.

I hoped to get this done much earlier and faster, but this is quite a yak to
shave. :-)

I realized that pushing out patches as soon as possible is the only way to
get this finished at all, as this is a moving target with all the work of
others which might interfere. So my revised plan is to do one arch after
the other and in each arch to cover stuff like the MSR emulation first in
order not to mix things up again.

>> This work is a prelude of getting rid of the magic value "1" for
>> "return to guest". I started in x86 KVM host code doing that and soon
>> stumbled over lots of other use cases of the magic "1" as return value,
>> especially in MSR emulation where a comment even implied this "1" was
>> due to the "return to guest" semantics.
>>
>> A detailed analysis of all related code paths revealed that there was
>> indeed a rather clean interface between the functions using the MSR
>> emulation "1" and those using the "return to guest" "1".
> 
> Ya, we've started chipping away at the MSR stuff.  The big challenge is avoiding
> subtle ABI changes related to the fixups done by kvm_do_msr_access().

Right.

This whole work was triggered by my accidental "fix" of kvm_mmu_page_fault()
replacing a "1" with "RET_PF_RETRY", which you stopped from hitting upstream.

Juergen

Download attachment "OpenPGP_0xB0DE9DD628BF132F.asc" of type "application/pgp-keys" (3684 bytes)

Download attachment "OpenPGP_signature.asc" of type "application/pgp-signature" (496 bytes)