There is a 'malloc' call in test_vmx_nested_state function, which can
be unsuccessful. This patch will add the malloc failure checking
to avoid possible null dereference and give more information
about test fail reasons.

Signed-off-by: Kunwu Chan <chentao@kylinos.cn>
---
 tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c b/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c
index 67a62a5a8895..18afc2000a74 100644
--- a/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c
+++ 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 = sizeof(struct kvm_nested_state) + getpagesize();
 	struct kvm_nested_state *state =
 		(struct kvm_nested_state *)malloc(state_sz);
+	TEST_ASSERT(state, "-ENOMEM when allocating kvm state");
 
 	/* The format must be set to 0. 0 for VMX, 1 for SVM. */
 	set_default_vmx_state(state, state_sz);
-- 
2.40.1

…
> This patch will add the malloc failure checking
…

* Please use a corresponding imperative wording for the change description.

* Would you like to add the tag “Fixes” accordingly?


…
> +++ 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 = sizeof(struct kvm_nested_state) + getpagesize();
>  	struct kvm_nested_state *state =
>  		(struct kvm_nested_state *)malloc(state_sz);
> +	TEST_ASSERT(state, "-ENOMEM when allocating kvm state");
…

Can “errno” be relevant for the error message construction?

Regards,
Markus

+others

On Tue, Apr 23, 2024, Markus Elfring wrote:
> …
> > This patch will add the malloc failure checking
> …
> 
> * Please use a corresponding imperative wording for the change description.
> 
> * Would you like to add the tag “Fixes” 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, but 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 = sizeof(struct kvm_nested_state) + getpagesize();
> >  	struct kvm_nested_state *state =
> >  		(struct kvm_nested_state *)malloc(state_sz);
> > +	TEST_ASSERT(state, "-ENOMEM when allocating kvm state");
> …
> 
> Can “errno” be relevant for the error message construction?

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

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 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 purposes)

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

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

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

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

On Tue, Apr 23, 2024 at 07:56:01AM -0700, Sean Christopherson wrote:
> +others
> 
> On Tue, Apr 23, 2024, Markus Elfring wrote:
> > …
> > > This patch will add the malloc failure checking
> > …
> > 
> > * Please use a corresponding imperative wording for the change description.
> > 
> > * Would you like to add the tag “Fixes” 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, but 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 = sizeof(struct kvm_nested_state) + getpagesize();
> > >  	struct kvm_nested_state *state =
> > >  		(struct kvm_nested_state *)malloc(state_sz);
> > > +	TEST_ASSERT(state, "-ENOMEM when allocating kvm state");
> > …
> > 
> > Can “errno” be relevant for the error message construction?
> 
> Probably not, but there's also no reason to assume ENOMEM.  TEST_ASSERT() spits
> out the actual errno, and we can just say something like "malloc() failed for
> blah blah blah".  
> 
> 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 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 purposes)
> 
> #define kvm_malloc(x)
> ({
> 	void *__ret;
> 
> 	__ret  = malloc(x);
> 	TEST_ASSERT(__ret, "Failed malloc(" #x ")\n");
> 	__ret;
> })
> 
> #define kvm_calloc(x, y)
> ({
> 	void *__ret;
> 
> 	__ret  = calloc(x, y);
> 	TEST_ASSERT(__ret, "Failed calloc(" #x ", " #y ")\n");
> 	__ret;
> })

Sounds good to me, but I'd call them test_malloc, test_calloc, etc. and
put them in include/test_util.h

Thanks,
drew

On Tue, Apr 23, 2024, Andrew Jones wrote:
> On Tue, Apr 23, 2024 at 07:56:01AM -0700, Sean Christopherson wrote:
> > +others
> > 
> > On Tue, Apr 23, 2024, Markus Elfring wrote:
> > > …
> > > > This patch will add the malloc failure checking
> > > …
> > > 
> > > * Please use a corresponding imperative wording for the change description.
> > > 
> > > * Would you like to add the tag “Fixes” 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, but 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 = sizeof(struct kvm_nested_state) + getpagesize();
> > > >  	struct kvm_nested_state *state =
> > > >  		(struct kvm_nested_state *)malloc(state_sz);
> > > > +	TEST_ASSERT(state, "-ENOMEM when allocating kvm state");
> > > …
> > > 
> > > Can “errno” be relevant for the error message construction?
> > 
> > Probably not, but there's also no reason to assume ENOMEM.  TEST_ASSERT() spits
> > out the actual errno, and we can just say something like "malloc() failed for
> > blah blah blah".  
> > 
> > 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 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 purposes)
> > 
> > #define kvm_malloc(x)
> > ({
> > 	void *__ret;
> > 
> > 	__ret  = malloc(x);
> > 	TEST_ASSERT(__ret, "Failed malloc(" #x ")\n");
> > 	__ret;
> > })
> > 
> > #define kvm_calloc(x, y)
> > ({
> > 	void *__ret;
> > 
> > 	__ret  = calloc(x, y);
> > 	TEST_ASSERT(__ret, "Failed calloc(" #x ", " #y ")\n");
> > 	__ret;
> > })
> 
> Sounds good to me, but I'd call them test_malloc, test_calloc, etc. and
> put them in include/test_util.h

Possibly terrible idea: what if we used kmalloc() and kcalloc()?  K is for KVM :-)

I like test_* more than kvm_*, but I'm mildly concerned that readers will be
confused by "test", e.g. initially thinking the "test" means it's just "testing"
if allocation is possible.

The obvious counter-argument is that people might also get tripped by kmalloc(),
e.g. thinking that selftests is somehow doing a kernel allocation.

I almost wonder if we should just pick a prefix that's less obviously connected
to KVM and/or selftests, but unique and short.

Hmm, tmalloc(), i.e t[est]malloc()?  tcalloc() gets a bit close to Google's
TCMalloc[*], but I suspect that any confusion would be entirely limited to
Googlers, and I'll volunteer us to suck it up and deal with it :-)

[*] https://github.com/google/tcmalloc

On Tue, Apr 23, 2024 at 12:15:47PM -0700, Sean Christopherson wrote:
...
> I almost wonder if we should just pick a prefix that's less obviously connected
> to KVM and/or selftests, but unique and short.
>

How about kvmsft_ ? It's based on the ksft_ prefix of kselftest.h. Maybe
it's too close to ksft though and would be confusing when using both in
the same test? I'm not a huge fan of capital letters, but we could also
do something like MALLOC()/CALLOC(). Eh, I don't know. Naming is hard.

Thanks,
drew

On Wed, Apr 24, 2024, Andrew Jones wrote:
> On Tue, Apr 23, 2024 at 12:15:47PM -0700, Sean Christopherson wrote:
> ...
> > I almost wonder if we should just pick a prefix that's less obviously connected
> > to KVM and/or selftests, but unique and short.
> >
> 
> How about kvmsft_ ? It's based on the ksft_ prefix of kselftest.h. Maybe
> it's too close to ksft though and would be confusing when using both in
> the same test?

I would prefer something short, and for whatever reason I have a mental block
with ksft.  I always read it as "k soft", which is completely nonsensical :-)

> I'm not a huge fan of capital letters, but we could also do something like
> MALLOC()/CALLOC().

Hmm, I'm not usually a fan either, but that could actually work quite well in this
case.  It would be quite intuitive, easy to visually parse whereas tmalloc() vs
malloc() kinda looks like a typo, and would more clearly communicate that they're
macros.

Hey,

On Wed, Apr 24, 2024 at 07:51:44AM -0700, Sean Christopherson wrote:
> On Wed, Apr 24, 2024, Andrew Jones wrote:
> > On Tue, Apr 23, 2024 at 12:15:47PM -0700, Sean Christopherson wrote:
> > ...
> > > I almost wonder if we should just pick a prefix that's less obviously connected
> > > to KVM and/or selftests, but unique and short.
> > >
> > 
> > How about kvmsft_ ? It's based on the ksft_ prefix of kselftest.h. Maybe
> > it's too close to ksft though and would be confusing when using both in
> > the same test?
> 
> I would prefer something short, and for whatever reason I have a mental block
> with ksft.  I always read it as "k soft", which is completely nonsensical :-)

I despise brevity in tests, so my strong preference is to use some form
of 'namespaced' helper. Perhaps others have better memory than
I do, but I'm quick to forget the selftests library and find the more
verbose / obvious function names helpful for jogging my memory.

> > I'm not a huge fan of capital letters, but we could also do something like
> > MALLOC()/CALLOC().
> 
> Hmm, I'm not usually a fan either, but that could actually work quite well in this
> case.  It would be quite intuitive, easy to visually parse whereas tmalloc() vs
> malloc() kinda looks like a typo, and would more clearly communicate that they're
> macros.

Ooo, don't leave me out on the bikeshedding! How about TEST_MALLOC() /
TEST_CALLOC(). It is vaguely similar to TEST_ASSERT(), which I'd hope
would give the impression that an assertion is lurking below.

-- 
Thanks,
Oliver

On Wed, Apr 24, 2024, Oliver Upton wrote:
> Hey,
> 
> On Wed, Apr 24, 2024 at 07:51:44AM -0700, Sean Christopherson wrote:
> > On Wed, Apr 24, 2024, Andrew Jones wrote:
> > > On Tue, Apr 23, 2024 at 12:15:47PM -0700, Sean Christopherson wrote:
> > > ...
> > > > I almost wonder if we should just pick a prefix that's less obviously connected
> > > > to KVM and/or selftests, but unique and short.
> > > >
> > > 
> > > How about kvmsft_ ? It's based on the ksft_ prefix of kselftest.h. Maybe
> > > it's too close to ksft though and would be confusing when using both in
> > > the same test?
> > 
> > I would prefer something short, and for whatever reason I have a mental block
> > with ksft.  I always read it as "k soft", which is completely nonsensical :-)
> 
> I despise brevity in tests, so my strong preference is to use some form
> of 'namespaced' helper. Perhaps others have better memory than
> I do, but I'm quick to forget the selftests library and find the more
> verbose / obvious function names helpful for jogging my memory.

Hmm, I generally agree, but in this case I think there's value in having the
names *not* stand out, because they really are uninteresting and would ideally
blend in.  I can't envision a scenario where we don't want to assert on an OOM,
i.e. there should never be a need to use a raw malloc(), and so I don't see much
value in making it obvious that the call sites are doing something special.

> > > I'm not a huge fan of capital letters, but we could also do something like
> > > MALLOC()/CALLOC().
> > 
> > Hmm, I'm not usually a fan either, but that could actually work quite well in this
> > case.  It would be quite intuitive, easy to visually parse whereas tmalloc() vs
> > malloc() kinda looks like a typo, and would more clearly communicate that they're
> > macros.
> 
> Ooo, don't leave me out on the bikeshedding! How about TEST_MALLOC() /
> TEST_CALLOC(). It is vaguely similar to TEST_ASSERT(), which I'd hope
> would give the impression that an assertion is lurking below.

Yeah, but it could also give the false impression that the macro does something
fancier, e.g. this makes me want to peek at TEST_MALLOC() to see what it's doing

	cpuid = TEST_MALLOC(kvm_cpuid2_size(nr_entries));

whereas this isn't quite enough to pique my curiosity.

	cpuid = MALLOC(kvm_cpuid2_size(nr_entries));

So I have a slight preference for just MALLOC()/CALLOC(), but I'm also ok with a
TEST_ prefix, my brain can adapt.  One of those two flavors has my vote.

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
> > > 
> > > On Tue, Apr 23, 2024, Markus Elfring wrote:
> > > > …
> > > > > This patch will add the malloc failure checking
> > > > …
> > > > 
> > > > * Please use a corresponding imperative wording for the change description.
> > > > 
> > > > * Would you like to add the tag “Fixes” 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, but 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 = sizeof(struct kvm_nested_state) + getpagesize();
> > > > >  	struct kvm_nested_state *state =
> > > > >  		(struct kvm_nested_state *)malloc(state_sz);
> > > > > +	TEST_ASSERT(state, "-ENOMEM when allocating kvm state");
> > > > …
> > > > 
> > > > Can “errno” be relevant for the error message construction?
> > > 
> > > Probably not, but there's also no reason to assume ENOMEM.  TEST_ASSERT() spits
> > > out the actual errno, and we can just say something like "malloc() failed for
> > > blah blah blah".  
> > > 
> > > 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 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 purposes)
> > > 
> > > #define kvm_malloc(x)
> > > ({
> > > 	void *__ret;
> > > 
> > > 	__ret  = malloc(x);
> > > 	TEST_ASSERT(__ret, "Failed malloc(" #x ")\n");
> > > 	__ret;
> > > })
> > > 
> > > #define kvm_calloc(x, y)
> > > ({
> > > 	void *__ret;
> > > 
> > > 	__ret  = calloc(x, y);
> > > 	TEST_ASSERT(__ret, "Failed calloc(" #x ", " #y ")\n");
> > > 	__ret;
> > > })
> > 
> > Sounds good to me, but I'd call them test_malloc, test_calloc, etc. and
> > put them in include/test_util.h
> 
> Possibly terrible idea: what if we used kmalloc() and kcalloc()?  K is for KVM :-)

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.

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.

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

regards,
dan carpenter

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
> > > > 
> > > > On Tue, Apr 23, 2024, Markus Elfring wrote:
> > > > > …
> > > > > > This patch will add the malloc failure checking
> > > > > …
> > > > > 
> > > > > * Please use a corresponding imperative wording for the change description.
> > > > > 
> > > > > * Would you like to add the tag “Fixes” 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, but 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 = sizeof(struct kvm_nested_state) + getpagesize();
> > > > > >  	struct kvm_nested_state *state =
> > > > > >  		(struct kvm_nested_state *)malloc(state_sz);
> > > > > > +	TEST_ASSERT(state, "-ENOMEM when allocating kvm state");
> > > > > …
> > > > > 
> > > > > Can “errno” be relevant for the error message construction?
> > > > 
> > > > Probably not, but there's also no reason to assume ENOMEM.  TEST_ASSERT() spits
> > > > out the actual errno, and we can just say something like "malloc() failed for
> > > > blah blah blah".  
> > > > 
> > > > 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 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 purposes)
> > > > 
> > > > #define kvm_malloc(x)
> > > > ({
> > > > 	void *__ret;
> > > > 
> > > > 	__ret  = malloc(x);
> > > > 	TEST_ASSERT(__ret, "Failed malloc(" #x ")\n");
> > > > 	__ret;
> > > > })
> > > > 
> > > > #define kvm_calloc(x, y)
> > > > ({
> > > > 	void *__ret;
> > > > 
> > > > 	__ret  = calloc(x, y);
> > > > 	TEST_ASSERT(__ret, "Failed calloc(" #x ", " #y ")\n");
> > > > 	__ret;
> > > > })
> > > 
> > > Sounds good to me, but I'd call them test_malloc, test_calloc, etc. and
> > > put them in include/test_util.h
> > 
> > Possibly terrible idea: what if we used kmalloc() and kcalloc()?  K is for KVM :-)
> 
> 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 doubt
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 value,
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 write 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.

On 4/23/24 12:39 PM, Kunwu Chan wrote:
> There is a 'malloc' call in test_vmx_nested_state function, which can
> be unsuccessful. This patch will add the malloc failure checking
> to avoid possible null dereference and give more information
> about test fail reasons.
LGTM

Reviewed-by: Muhammad Usama Anjum <usama.anjum@collabora.com>

> 
> Signed-off-by: Kunwu Chan <chentao@kylinos.cn>
> ---
>  tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c b/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c
> index 67a62a5a8895..18afc2000a74 100644
> --- a/tools/testing/selftests/kvm/x86_64/vmx_set_nested_state_test.c
> +++ 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 = sizeof(struct kvm_nested_state) + getpagesize();
>  	struct kvm_nested_state *state =
>  		(struct kvm_nested_state *)malloc(state_sz);
> +	TEST_ASSERT(state, "-ENOMEM when allocating kvm state");
>  
>  	/* The format must be set to 0. 0 for VMX, 1 for SVM. */
>  	set_default_vmx_state(state, state_sz);

-- 
BR,
Muhammad Usama Anjum