kvm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [kvm-unit-tests PATCH v2] alloc: Add memalign error checks
@ 2019-11-04 10:29 Janosch Frank
  2019-11-04 10:33 ` David Hildenbrand
  0 siblings, 1 reply; 10+ messages in thread
From: Janosch Frank @ 2019-11-04 10:29 UTC (permalink / raw)
  To: kvm; +Cc: thuth, david, pbonzini

Let's test for size and alignment in memalign to catch invalid input
data. Also we need to test for NULL after calling the memalign
function of the registered alloc operations.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 lib/alloc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/lib/alloc.c b/lib/alloc.c
index ecdbbc4..b763c70 100644
--- a/lib/alloc.c
+++ b/lib/alloc.c
@@ -47,6 +47,8 @@ void *memalign(size_t alignment, size_t size)
 	uintptr_t mem;
 
 	assert(alloc_ops && alloc_ops->memalign);
+	if (!size || !alignment)
+		return NULL;
 	if (alignment <= sizeof(uintptr_t))
 		alignment = sizeof(uintptr_t);
 	else
@@ -55,6 +57,7 @@ void *memalign(size_t alignment, size_t size)
 	blkalign = MAX(alignment, alloc_ops->align_min);
 	size = ALIGN(size + METADATA_EXTRA, alloc_ops->align_min);
 	p = alloc_ops->memalign(blkalign, size);
+	assert(p);
 
 	/* Leave room for metadata before aligning the result.  */
 	mem = (uintptr_t)p + METADATA_EXTRA;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [kvm-unit-tests PATCH v2] alloc: Add memalign error checks
  2019-11-04 10:29 [kvm-unit-tests PATCH v2] alloc: Add memalign error checks Janosch Frank
@ 2019-11-04 10:33 ` David Hildenbrand
  2019-11-04 10:54   ` Andrew Jones
  0 siblings, 1 reply; 10+ messages in thread
From: David Hildenbrand @ 2019-11-04 10:33 UTC (permalink / raw)
  To: Janosch Frank, kvm; +Cc: thuth, pbonzini

On 04.11.19 11:29, Janosch Frank wrote:
> Let's test for size and alignment in memalign to catch invalid input
> data. Also we need to test for NULL after calling the memalign
> function of the registered alloc operations.
> 
> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>   lib/alloc.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/lib/alloc.c b/lib/alloc.c
> index ecdbbc4..b763c70 100644
> --- a/lib/alloc.c
> +++ b/lib/alloc.c
> @@ -47,6 +47,8 @@ void *memalign(size_t alignment, size_t size)
>   	uintptr_t mem;
>   
>   	assert(alloc_ops && alloc_ops->memalign);
> +	if (!size || !alignment)
> +		return NULL;
>   	if (alignment <= sizeof(uintptr_t))
>   		alignment = sizeof(uintptr_t);

BTW, memalign MAN page

"EINVAL The alignment argument was not a power of two, or was not a 
multiple of sizeof(void *)."

So we could also return NULL here (not sure if anybody relies on that)

>   	else
> @@ -55,6 +57,7 @@ void *memalign(size_t alignment, size_t size)
>   	blkalign = MAX(alignment, alloc_ops->align_min);
>   	size = ALIGN(size + METADATA_EXTRA, alloc_ops->align_min);
>   	p = alloc_ops->memalign(blkalign, size);
> +	assert(p);
>   
>   	/* Leave room for metadata before aligning the result.  */
>   	mem = (uintptr_t)p + METADATA_EXTRA;
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [kvm-unit-tests PATCH v2] alloc: Add memalign error checks
  2019-11-04 10:33 ` David Hildenbrand
@ 2019-11-04 10:54   ` Andrew Jones
  2019-11-04 11:29     ` Paolo Bonzini
  0 siblings, 1 reply; 10+ messages in thread
From: Andrew Jones @ 2019-11-04 10:54 UTC (permalink / raw)
  To: David Hildenbrand; +Cc: Janosch Frank, kvm, thuth, pbonzini

On Mon, Nov 04, 2019 at 11:33:28AM +0100, David Hildenbrand wrote:
> On 04.11.19 11:29, Janosch Frank wrote:
> > Let's test for size and alignment in memalign to catch invalid input
> > data. Also we need to test for NULL after calling the memalign
> > function of the registered alloc operations.
> > 
> > Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> > ---
> >   lib/alloc.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/lib/alloc.c b/lib/alloc.c
> > index ecdbbc4..b763c70 100644
> > --- a/lib/alloc.c
> > +++ b/lib/alloc.c
> > @@ -47,6 +47,8 @@ void *memalign(size_t alignment, size_t size)
> >   	uintptr_t mem;
> >   	assert(alloc_ops && alloc_ops->memalign);
> > +	if (!size || !alignment)
> > +		return NULL;
> >   	if (alignment <= sizeof(uintptr_t))
> >   		alignment = sizeof(uintptr_t);
> 
> BTW, memalign MAN page
> 
> "EINVAL The alignment argument was not a power of two, or was not a multiple
> of sizeof(void *)."
> 

Since we're not implementing the EINVAL part, then I'd assert when
alignment isn't correct.

> So we could also return NULL here (not sure if anybody relies on that)

I made the following changes and tested with arm/arm64. No problems.

Thanks,
drew


diff --git a/lib/alloc.c b/lib/alloc.c
index ecdbbc44dbf9..ed8f5f94c9b0 100644
--- a/lib/alloc.c
+++ b/lib/alloc.c
@@ -46,15 +46,17 @@ void *memalign(size_t alignment, size_t size)
 	uintptr_t blkalign;
 	uintptr_t mem;
 
+	if (!size)
+		return NULL;
+
+	assert(alignment >= sizeof(void *) && is_power_of_2(alignment));
 	assert(alloc_ops && alloc_ops->memalign);
-	if (alignment <= sizeof(uintptr_t))
-		alignment = sizeof(uintptr_t);
-	else
-		size += alignment - 1;
 
+	size += alignment - 1;
 	blkalign = MAX(alignment, alloc_ops->align_min);
 	size = ALIGN(size + METADATA_EXTRA, alloc_ops->align_min);
 	p = alloc_ops->memalign(blkalign, size);
+	assert(p);
 
 	/* Leave room for metadata before aligning the result.  */
 	mem = (uintptr_t)p + METADATA_EXTRA;


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [kvm-unit-tests PATCH v2] alloc: Add memalign error checks
  2019-11-04 10:54   ` Andrew Jones
@ 2019-11-04 11:29     ` Paolo Bonzini
  2019-11-04 11:31       ` David Hildenbrand
  2019-11-11 10:12       ` Janosch Frank
  0 siblings, 2 replies; 10+ messages in thread
From: Paolo Bonzini @ 2019-11-04 11:29 UTC (permalink / raw)
  To: Andrew Jones, David Hildenbrand; +Cc: Janosch Frank, kvm, thuth

On 04/11/19 11:54, Andrew Jones wrote:
> 
> diff --git a/lib/alloc.c b/lib/alloc.c
> index ecdbbc44dbf9..ed8f5f94c9b0 100644
> --- a/lib/alloc.c
> +++ b/lib/alloc.c
> @@ -46,15 +46,17 @@ void *memalign(size_t alignment, size_t size)
>  	uintptr_t blkalign;
>  	uintptr_t mem;
>  
> +	if (!size)
> +		return NULL;
> +
> +	assert(alignment >= sizeof(void *) && is_power_of_2(alignment));
>  	assert(alloc_ops && alloc_ops->memalign);
> -	if (alignment <= sizeof(uintptr_t))
> -		alignment = sizeof(uintptr_t);
> -	else
> -		size += alignment - 1;
>  
> +	size += alignment - 1;
>  	blkalign = MAX(alignment, alloc_ops->align_min);
>  	size = ALIGN(size + METADATA_EXTRA, alloc_ops->align_min);
>  	p = alloc_ops->memalign(blkalign, size);
> +	assert(p);
>  
>  	/* Leave room for metadata before aligning the result.  */
>  	mem = (uintptr_t)p + METADATA_EXTRA;

Looks good, this is what I am queuing.

Paolo

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [kvm-unit-tests PATCH v2] alloc: Add memalign error checks
  2019-11-04 11:29     ` Paolo Bonzini
@ 2019-11-04 11:31       ` David Hildenbrand
  2019-11-11 10:12       ` Janosch Frank
  1 sibling, 0 replies; 10+ messages in thread
From: David Hildenbrand @ 2019-11-04 11:31 UTC (permalink / raw)
  To: Paolo Bonzini, Andrew Jones; +Cc: Janosch Frank, kvm, thuth

On 04.11.19 12:29, Paolo Bonzini wrote:
> On 04/11/19 11:54, Andrew Jones wrote:
>>
>> diff --git a/lib/alloc.c b/lib/alloc.c
>> index ecdbbc44dbf9..ed8f5f94c9b0 100644
>> --- a/lib/alloc.c
>> +++ b/lib/alloc.c
>> @@ -46,15 +46,17 @@ void *memalign(size_t alignment, size_t size)
>>   	uintptr_t blkalign;
>>   	uintptr_t mem;
>>   
>> +	if (!size)
>> +		return NULL;
>> +
>> +	assert(alignment >= sizeof(void *) && is_power_of_2(alignment));
>>   	assert(alloc_ops && alloc_ops->memalign);
>> -	if (alignment <= sizeof(uintptr_t))
>> -		alignment = sizeof(uintptr_t);
>> -	else
>> -		size += alignment - 1;
>>   
>> +	size += alignment - 1;
>>   	blkalign = MAX(alignment, alloc_ops->align_min);
>>   	size = ALIGN(size + METADATA_EXTRA, alloc_ops->align_min);
>>   	p = alloc_ops->memalign(blkalign, size);
>> +	assert(p);
>>   
>>   	/* Leave room for metadata before aligning the result.  */
>>   	mem = (uintptr_t)p + METADATA_EXTRA;
> 
> Looks good, this is what I am queuing.
> 
> Paolo
> 

Please add my

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [kvm-unit-tests PATCH v2] alloc: Add memalign error checks
  2019-11-04 11:29     ` Paolo Bonzini
  2019-11-04 11:31       ` David Hildenbrand
@ 2019-11-11 10:12       ` Janosch Frank
  2019-11-11 12:31         ` Andrew Jones
  1 sibling, 1 reply; 10+ messages in thread
From: Janosch Frank @ 2019-11-11 10:12 UTC (permalink / raw)
  To: Paolo Bonzini, Andrew Jones, David Hildenbrand; +Cc: kvm, thuth


[-- Attachment #1.1: Type: text/plain, Size: 1231 bytes --]

On 11/4/19 12:29 PM, Paolo Bonzini wrote:
> On 04/11/19 11:54, Andrew Jones wrote:
>>
>> diff --git a/lib/alloc.c b/lib/alloc.c
>> index ecdbbc44dbf9..ed8f5f94c9b0 100644
>> --- a/lib/alloc.c
>> +++ b/lib/alloc.c
>> @@ -46,15 +46,17 @@ void *memalign(size_t alignment, size_t size)
>>  	uintptr_t blkalign;
>>  	uintptr_t mem;
>>  
>> +	if (!size)
>> +		return NULL;
>> +
>> +	assert(alignment >= sizeof(void *) && is_power_of_2(alignment));
>>  	assert(alloc_ops && alloc_ops->memalign);
>> -	if (alignment <= sizeof(uintptr_t))
>> -		alignment = sizeof(uintptr_t);
>> -	else
>> -		size += alignment - 1;
>>  
>> +	size += alignment - 1;
>>  	blkalign = MAX(alignment, alloc_ops->align_min);
>>  	size = ALIGN(size + METADATA_EXTRA, alloc_ops->align_min);
>>  	p = alloc_ops->memalign(blkalign, size);
>> +	assert(p);

I had some more time to think about and test this and I think returning
NULL here is more useful. My usecase is a limit test where I allocate
until I get a NULL and then free everything afterwards.

>>  
>>  	/* Leave room for metadata before aligning the result.  */
>>  	mem = (uintptr_t)p + METADATA_EXTRA;
> 
> Looks good, this is what I am queuing.
> 
> Paolo





[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [kvm-unit-tests PATCH v2] alloc: Add memalign error checks
  2019-11-11 10:12       ` Janosch Frank
@ 2019-11-11 12:31         ` Andrew Jones
  2019-11-11 13:04           ` Paolo Bonzini
  0 siblings, 1 reply; 10+ messages in thread
From: Andrew Jones @ 2019-11-11 12:31 UTC (permalink / raw)
  To: Janosch Frank; +Cc: Paolo Bonzini, David Hildenbrand, kvm, thuth

On Mon, Nov 11, 2019 at 11:12:41AM +0100, Janosch Frank wrote:
> On 11/4/19 12:29 PM, Paolo Bonzini wrote:
> > On 04/11/19 11:54, Andrew Jones wrote:
> >>
> >> diff --git a/lib/alloc.c b/lib/alloc.c
> >> index ecdbbc44dbf9..ed8f5f94c9b0 100644
> >> --- a/lib/alloc.c
> >> +++ b/lib/alloc.c
> >> @@ -46,15 +46,17 @@ void *memalign(size_t alignment, size_t size)
> >>  	uintptr_t blkalign;
> >>  	uintptr_t mem;
> >>  
> >> +	if (!size)
> >> +		return NULL;
> >> +
> >> +	assert(alignment >= sizeof(void *) && is_power_of_2(alignment));
> >>  	assert(alloc_ops && alloc_ops->memalign);
> >> -	if (alignment <= sizeof(uintptr_t))
> >> -		alignment = sizeof(uintptr_t);
> >> -	else
> >> -		size += alignment - 1;
> >>  
> >> +	size += alignment - 1;
> >>  	blkalign = MAX(alignment, alloc_ops->align_min);
> >>  	size = ALIGN(size + METADATA_EXTRA, alloc_ops->align_min);
> >>  	p = alloc_ops->memalign(blkalign, size);
> >> +	assert(p);
> 
> I had some more time to think about and test this and I think returning
> NULL here is more useful. My usecase is a limit test where I allocate
> until I get a NULL and then free everything afterwards.

I think I prefer the assert here, since most users of this will not be
expecting to run out of memory, and therefore not checking for it. I
think you may want to just write your own allocator in your unit test
that's based on alloc_pages().

Thanks,
drew


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [kvm-unit-tests PATCH v2] alloc: Add memalign error checks
  2019-11-11 12:31         ` Andrew Jones
@ 2019-11-11 13:04           ` Paolo Bonzini
  2019-11-11 13:13             ` Janosch Frank
  0 siblings, 1 reply; 10+ messages in thread
From: Paolo Bonzini @ 2019-11-11 13:04 UTC (permalink / raw)
  To: Andrew Jones, Janosch Frank; +Cc: David Hildenbrand, kvm, thuth

On 11/11/19 13:31, Andrew Jones wrote:
>> I had some more time to think about and test this and I think returning
>> NULL here is more useful. My usecase is a limit test where I allocate
>> until I get a NULL and then free everything afterwards.
> I think I prefer the assert here, since most users of this will not be
> expecting to run out of memory, and therefore not checking for it.

Yeah, the main issue with returning NULL is that (void*) 0 is a valid
address for kvm-unit-tests.

Paolo


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [kvm-unit-tests PATCH v2] alloc: Add memalign error checks
  2019-11-11 13:04           ` Paolo Bonzini
@ 2019-11-11 13:13             ` Janosch Frank
  2019-11-11 13:17               ` Paolo Bonzini
  0 siblings, 1 reply; 10+ messages in thread
From: Janosch Frank @ 2019-11-11 13:13 UTC (permalink / raw)
  To: Paolo Bonzini, Andrew Jones; +Cc: David Hildenbrand, kvm, thuth


[-- Attachment #1.1: Type: text/plain, Size: 630 bytes --]

On 11/11/19 2:04 PM, Paolo Bonzini wrote:
> On 11/11/19 13:31, Andrew Jones wrote:
>>> I had some more time to think about and test this and I think returning
>>> NULL here is more useful. My usecase is a limit test where I allocate
>>> until I get a NULL and then free everything afterwards.
>> I think I prefer the assert here, since most users of this will not be
>> expecting to run out of memory, and therefore not checking for it.
> 
> Yeah, the main issue with returning NULL is that (void*) 0 is a valid
> address for kvm-unit-tests.
> 
> Paolo

So, images are not loaded to 0 for non s390x architectures?


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [kvm-unit-tests PATCH v2] alloc: Add memalign error checks
  2019-11-11 13:13             ` Janosch Frank
@ 2019-11-11 13:17               ` Paolo Bonzini
  0 siblings, 0 replies; 10+ messages in thread
From: Paolo Bonzini @ 2019-11-11 13:17 UTC (permalink / raw)
  To: Janosch Frank, Andrew Jones; +Cc: David Hildenbrand, kvm, thuth


[-- Attachment #1.1: Type: text/plain, Size: 931 bytes --]

On 11/11/19 14:13, Janosch Frank wrote:
> On 11/11/19 2:04 PM, Paolo Bonzini wrote:
>> On 11/11/19 13:31, Andrew Jones wrote:
>>>> I had some more time to think about and test this and I think returning
>>>> NULL here is more useful. My usecase is a limit test where I allocate
>>>> until I get a NULL and then free everything afterwards.
>>> I think I prefer the assert here, since most users of this will not be
>>> expecting to run out of memory, and therefore not checking for it.
>>
>> Yeah, the main issue with returning NULL is that (void*) 0 is a valid
>> address for kvm-unit-tests.
> 
> So, images are not loaded to 0 for non s390x architectures?

It depends on the architecture.  Either way, if there is an out of
memory error it would turn into a rogue memory write.  That would be
harder to debug than an assertion failure.

If there is need, we can add try_malloc and try_memalign, too.

Paolo


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-11-11 13:17 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-04 10:29 [kvm-unit-tests PATCH v2] alloc: Add memalign error checks Janosch Frank
2019-11-04 10:33 ` David Hildenbrand
2019-11-04 10:54   ` Andrew Jones
2019-11-04 11:29     ` Paolo Bonzini
2019-11-04 11:31       ` David Hildenbrand
2019-11-11 10:12       ` Janosch Frank
2019-11-11 12:31         ` Andrew Jones
2019-11-11 13:04           ` Paolo Bonzini
2019-11-11 13:13             ` Janosch Frank
2019-11-11 13:17               ` Paolo Bonzini

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).