linux-kernel - Re: [PATCH v2] kvm: selftests: Add KVM_CAP_MAX_VCPU

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] [day] [month] [year] [list]

Message-ID: <Yn2uW0C+48fnDgfj@google.com>
Date:   Fri, 13 May 2022 01:03:23 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Zeng Guang <guang.zeng@...el.com>
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        Tony Luck <tony.luck@...el.com>,
        Kan Liang <kan.liang@...ux.intel.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "H. Peter Anvin" <hpa@...or.com>,
        Kim Phillips <kim.phillips@....com>,
        Jarkko Sakkinen <jarkko@...nel.org>,
        Jethro Beekman <jethro@...tanix.com>,
        Kai Huang <kai.huang@...el.com>, x86@...nel.org,
        linux-kernel@...r.kernel.org, Robert Hu <robert.hu@...el.com>,
        Gao Chao <chao.gao@...el.com>
Subject: Re: [PATCH v2] kvm: selftests: Add KVM_CAP_MAX_VCPU_ID cap test

On Tue, May 03, 2022, Zeng Guang wrote:
> +int main(int argc, char *argv[])
> +{
> +	struct kvm_vm *vm;
> +	struct kvm_enable_cap cap = { 0 };
> +	int ret;
> +
> +	vm = vm_create(VM_MODE_DEFAULT, 0, O_RDWR);
> +
> +	/* Get KVM_CAP_MAX_VCPU_ID cap supported in KVM */
> +	ret = vm_check_cap(vm, KVM_CAP_MAX_VCPU_ID);

Because it's trivial to do so, and will avoid a hardcoded "max", what about looping
over all possible values?  And then some arbitrary number of the max?

	max_nr_vcpus = vm_check_cap(vm, KVM_CAP_MAX_VCPU_ID);
	TEST_ASSERT(max_nr_vcpus > ???, ...)

	for (i = 0; i < max_nr_vcpus; i++) 
		vm = vm_create(VM_MODE_DEFAULT, 0, O_RDWR);

		vm_set_max_nr_vcpus(vm, 0);
		vm_create_invalid_vcpu(vm, 0);

		vm_set_max_nr_vcpus(vm, i);
		vm_set_max_nr_vcpus(vm, i);
		vm_create_invalid_vcpu(vm, i);
		vm_create_invalid_vcpu(vm, i + 1);

		vm_set_invalid_max_nr_vcpus(vm, 0);
		vm_set_invalid_max_nr_vcpus(vm, i + 1);
		vm_set_invalid_max_nr_vcpus(vm, i - 1);
		vm_set_invalid_max_nr_vcpus(vm, max_nr_vcpus);

		close(vm->fd);
	}

	for ( ; i < max_nr_vcpus + 100; i++) {
		vm = vm_create(VM_MODE_DEFAULT, 0, O_RDWR);

		vm_set_invalid_max_nr_vcpus(vm, i);

		close(vm->fd);
	}
> +
> +	/* Check failure if set KVM_CAP_MAX_VCPU_ID beyond KVM cap */
> +	cap.cap = KVM_CAP_MAX_VCPU_ID;
> +	cap.args[0] = ret + 1;
> +	ret = ioctl(vm->fd, KVM_ENABLE_CAP, &cap);

A helper or two to set the cap would be, uh, helpful :-)  See above for ideas.

> +	TEST_ASSERT(ret < 0,
> +		    "Unexpected success to enable KVM_CAP_MAX_VCPU_ID"
> +		    "beyond KVM cap!\n");

Please don't wrap quoted strings.  Shorten the string and/or let the line run long.
For the string/message, prioritize information that the user _can't_ get from looking
at the code, and info that is highly relevant to the expectations.  E.g. print the
the return value, the errno, and the allegedly bad value.

It's definitely helpful to provide context too (KVM Unit Tests drive me bonkers for
their terse messages), but for cases like this, it's redundant.  "Unexpected success"
is redundant because the "ret < 0" conveys that failure was expected, and hopefully
most people will intuit that the test was trying "to enable" KVM_CAP_MAX_VCPU_ID.
If not, a quick glance at the code (file and line provided) will give them that info.

E.g. assuming this ends up in a helper, something like

	TEST_ASSERT(ret == -1 && errno == EINVAL,
		    "KVM_CAP_MAX_VCPU_ID bug, max ID = %d, ret = %d, errno = %d (%s),
		    max_id, errno, strerror(errno));

IMO it's worth checking errno to reduce the probability of false pass, e.g. if the
ioctl() is rejected for some other reason due to a test bug.

> +
> +	/* Check success if set KVM_CAP_MAX_VCPU_ID */
> +	cap.args[0] = MAX_VCPU_ID;
> +	ret = ioctl(vm->fd, KVM_ENABLE_CAP, &cap);
> +	TEST_ASSERT(ret == 0,
> +		    "Unexpected failure to enable KVM_CAP_MAX_VCPU_ID!\n");