Received-SPF: pass (google.com: domain of linux-kernel+bounces-155400-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Tue, 23 Apr 2024 07:56:01 -0700
In-Reply-To: <878bf83c-cd5b-48d0-8b4e-77223f1806dc@web.de>
Precedence: bulk
Mime-Version: 1.0
References: <20240423073952.2001989-1-chentao@kylinos.cn> <878bf83c-cd5b-48d0-8b4e-77223f1806dc@web.de>
Message-ID: <ZifMAWn32tZBQHs0@google.com>
Subject: Re: [PATCH] KVM: selftests: Add 'malloc' failure check in test_vmx_nested_state
From: Sean Christopherson <seanjc@google.com>
To: Markus Elfring <Markus.Elfring@web.de>
Cc: Kunwu Chan <chentao@kylinos.cn>, linux-kselftest@vger.kernel.org, 
	kvm@vger.kernel.org, kernel-janitors@vger.kernel.org, 
	Muhammad Usama Anjum <usama.anjum@collabora.com>, Paolo Bonzini <pbonzini@redhat.com>, 
	Shuah Khan <shuah@kernel.org>, LKML <linux-kernel@vger.kernel.org>, 
	Kunwu Chan <kunwu.chan@hotmail.com>, Andrew Jones <ajones@ventanamicro.com>, 
	Anup Patel <anup@brainfault.org>, Thomas Huth <thuth@redhat.com>, 
	Oliver Upton <oliver.upton@linux.dev>
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

+others

On Tue, Apr 23, 2024, Markus Elfring wrote:
> =E2=80=A6
> > This patch will add the malloc failure checking
> =E2=80=A6
>=20
> * Please use a corresponding imperative wording for the change descriptio=
n.
>=20
> * Would you like to add the tag =E2=80=9CFixes=E2=80=9D accordingly?

Nah, don't bother with Fixes.  OOM will cause the test to fail regardless, =
the
fact that it gets an assert instead a NULL pointer deref is nice to have, b=
ut by
no means does it fix a bug.

> > +++ b/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c
> > @@ -91,6 +91,7 @@ void test_vmx_nested_state(struct kvm_vcpu *vcpu)
> >  	const int state_sz =3D sizeof(struct kvm_nested_state) + getpagesize(=
);
> >  	struct kvm_nested_state *state =3D
> >  		(struct kvm_nested_state *)malloc(state_sz);
> > +	TEST_ASSERT(state, "-ENOMEM when allocating kvm state");
> =E2=80=A6
>=20
> Can =E2=80=9Cerrno=E2=80=9D be relevant for the error message constructio=
n?

Probably not, but there's also no reason to assume ENOMEM.  TEST_ASSERT() s=
pits
out the actual errno, and we can just say something like "malloc() failed f=
or
blah blah blah". =20

But rather than keeping playing whack-a-mole, what if we add macros to perf=
orm
allocations and assert on the result?  I have zero interest in chasing down=
 all
of the "unsafe" allocations, and odds are very good that we'll collectively=
 fail
to enforce checking on new code.

E.g. something like (obviously won't compile, just for demonstration purpos=
es)

#define kvm_malloc(x)
({
	void *__ret;

	__ret  =3D malloc(x);
	TEST_ASSERT(__ret, "Failed malloc(" #x ")\n");
	__ret;
})

#define kvm_calloc(x, y)
({
	void *__ret;

	__ret  =3D calloc(x, y);
	TEST_ASSERT(__ret, "Failed calloc(" #x ", " #y ")\n");
	__ret;
})