Received-SPF: pass (google.com: domain of linux-kernel+bounces-157118-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 24 Apr 2024 07:47:20 -0700
In-Reply-To: <9f67df9d-ab27-40b9-8849-3069649dc082@moroto.mountain>
Precedence: bulk
Mime-Version: 1.0
References: <20240423073952.2001989-1-chentao@kylinos.cn> <878bf83c-cd5b-48d0-8b4e-77223f1806dc@web.de>
 <ZifMAWn32tZBQHs0@google.com> <20240423-0db9024011213dcffe815c5c@orel>
 <ZigI48_cI7Twb9gD@google.com> <9f67df9d-ab27-40b9-8849-3069649dc082@moroto.mountain>
Message-ID: <ZikbeCsIom80tDPR@google.com>
Subject: Re: [PATCH] KVM: selftests: Add 'malloc' failure check in test_vmx_nested_state
From: Sean Christopherson <seanjc@google.com>
To: Dan Carpenter <dan.carpenter@linaro.org>
Cc: Andrew Jones <ajones@ventanamicro.com>, Markus Elfring <Markus.Elfring@web.de>, 
	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>, 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

On Wed, Apr 24, 2024, Dan Carpenter wrote:
> On Tue, Apr 23, 2024 at 12:15:47PM -0700, Sean Christopherson wrote:
> > On Tue, Apr 23, 2024, Andrew Jones wrote:
> > > On Tue, Apr 23, 2024 at 07:56:01AM -0700, Sean Christopherson wrote:
> > > > +others
> > > >=20
> > > > 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 de=
scription.
> > > > >=20
> > > > > * Would you like to add the tag =E2=80=9CFixes=E2=80=9D according=
ly?
> > > >=20
> > > > Nah, don't bother with Fixes.  OOM will cause the test to fail rega=
rdless, the
> > > > fact that it gets an assert instead a NULL pointer deref is nice to=
 have, but by
> > > > no means does it fix a bug.
> > > >=20
> > > > > > +++ b/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_t=
est.c
> > > > > > @@ -91,6 +91,7 @@ void test_vmx_nested_state(struct kvm_vcpu *v=
cpu)
> > > > > >  	const int state_sz =3D sizeof(struct kvm_nested_state) + getp=
agesize();
> > > > > >  	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 con=
struction?
> > > >=20
> > > > Probably not, but there's also no reason to assume ENOMEM.  TEST_AS=
SERT() spits
> > > > out the actual errno, and we can just say something like "malloc() =
failed for
> > > > blah blah blah". =20
> > > >=20
> > > > But rather than keeping playing whack-a-mole, what if we add macros=
 to perform
> > > > allocations and assert on the result?  I have zero interest in chas=
ing down all
> > > > of the "unsafe" allocations, and odds are very good that we'll coll=
ectively fail
> > > > to enforce checking on new code.
> > > >=20
> > > > E.g. something like (obviously won't compile, just for demonstratio=
n purposes)
> > > >=20
> > > > #define kvm_malloc(x)
> > > > ({
> > > > 	void *__ret;
> > > >=20
> > > > 	__ret  =3D malloc(x);
> > > > 	TEST_ASSERT(__ret, "Failed malloc(" #x ")\n");
> > > > 	__ret;
> > > > })
> > > >=20
> > > > #define kvm_calloc(x, y)
> > > > ({
> > > > 	void *__ret;
> > > >=20
> > > > 	__ret  =3D calloc(x, y);
> > > > 	TEST_ASSERT(__ret, "Failed calloc(" #x ", " #y ")\n");
> > > > 	__ret;
> > > > })
> > >=20
> > > Sounds good to me, but I'd call them test_malloc, test_calloc, etc. a=
nd
> > > put them in include/test_util.h
> >=20
> > Possibly terrible idea: what if we used kmalloc() and kcalloc()?  K is =
for KVM :-)
>=20
> That's a legit terrible idea...  It probably would trigger more static
> checker warnings because the general policy is kmalloc() is kernel code
> and we *have* to test for errors.

Roger that.

> To be honest, I would have just rejected the first patch.  You
> obviously know this and have said this earlier in the thread but just
> for the other people, this is a userspace test that runs for a short
> time and then exits.  If it gets killed because we don't have enough
> memory that's fine.  It would be better to just fix the static checker
> to not print pointless warnings or educate people to ignore warnings
> like this.

This particular patch may have been motiviated by a static checker, but I d=
oubt
static checkers are responsible for all of the many sanity checks on malloc=
() in
KVM selftests.  And while I agree that the sanity checks don't and much val=
ue,
deleting the existing checks and preventing checks from being reintroduced =
would
be a never ending battle.

> Creating the test_malloc() to silence the warning also seems like an
> okay idea as well.

Yeah, it's not exactly my first choice, but the reality is that people writ=
e KVM
elftests by copying an existing test (often literally), and so the best way=
 to
educate developers on the preferred approach/style is to have all existing =
code
adhere to a single approach/style.