linux-kernel - Re: [PATCH 2/4] KVM: x86: Drop unnecessary goto+label in kvm_arch

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: <YtV37rQaExsfITH3@google.com>
Date:   Mon, 18 Jul 2022 15:10:38 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Maxim Levitsky <mlevitsk@...hat.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] KVM: x86: Drop unnecessary goto+label in
 kvm_arch_init()

On Mon, Jul 18, 2022, Maxim Levitsky wrote:
> I honestly don't see much value in this change, but I don't mind it either.

Yeah, this particular instance isn't a significant improvement, but I really dislike
the pattern (if the target is a raw return) and want to discourage its use in KVM.

For longer functions, having to scroll down to see that the target is nothing more
than a raw return is quite annoying.  And for more complex usage, the pattern sometimes
leads to setting the return value well ahead of the "goto", which combined with the
scrolling is very unfriendly to readers.

E.g. prior to commit 71a4c30bf0d3 ("KVM: Refactor error handling for setting memory
region"), the memslot code input validation was written as so.  The "r = 0" in the
"Nothing to change" path was especially gross.

        r = -EINVAL;
        as_id = mem->slot >> 16;
        id = (u16)mem->slot;

        /* General sanity checks */
        if (mem->memory_size & (PAGE_SIZE - 1))
                goto out;
        if (mem->guest_phys_addr & (PAGE_SIZE - 1))
                goto out;
        /* We can read the guest memory with __xxx_user() later on. */
        if ((id < KVM_USER_MEM_SLOTS) &&
            ((mem->userspace_addr & (PAGE_SIZE - 1)) ||
             !access_ok((void __user *)(unsigned long)mem->userspace_addr,
                        mem->memory_size)))
                goto out;
        if (as_id >= KVM_ADDRESS_SPACE_NUM || id >= KVM_MEM_SLOTS_NUM)
                goto out;
        if (mem->guest_phys_addr + mem->memory_size < mem->guest_phys_addr)
                goto out;

        slot = id_to_memslot(__kvm_memslots(kvm, as_id), id);
        base_gfn = mem->guest_phys_addr >> PAGE_SHIFT;
        npages = mem->memory_size >> PAGE_SHIFT;

        if (npages > KVM_MEM_MAX_NR_PAGES)
                goto out;

        new = old = *slot;

        new.id = id;
        new.base_gfn = base_gfn;
        new.npages = npages;
        new.flags = mem->flags;
        new.userspace_addr = mem->userspace_addr;

        if (npages) {
                if (!old.npages)
                        change = KVM_MR_CREATE;
                else { /* Modify an existing slot. */
                        if ((new.userspace_addr != old.userspace_addr) ||
                            (npages != old.npages) ||
                            ((new.flags ^ old.flags) & KVM_MEM_READONLY))
                                goto out;

                        if (base_gfn != old.base_gfn)
                                change = KVM_MR_MOVE;
                        else if (new.flags != old.flags)
                                change = KVM_MR_FLAGS_ONLY;
                        else { /* Nothing to change. */
                                r = 0;
                                goto out;
                        }
                }
        } else {
                if (!old.npages)
                        goto out;

                change = KVM_MR_DELETE;
                new.base_gfn = 0;
                new.flags = 0;
        }