[kvm-unit-tests PATCH v2] alloc: Add memalign error checks

[kvm-unit-tests PATCH v2] alloc: Add memalign error checks

[Janosch Frank]

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

Re: [kvm-unit-tests PATCH v2] alloc: Add memalign error checks

[David Hildenbrand]

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

Re: [kvm-unit-tests PATCH v2] alloc: Add memalign error checks

[Andrew Jones]

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;

Re: [kvm-unit-tests PATCH v2] alloc: Add memalign error checks

[Paolo Bonzini]

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

Re: [kvm-unit-tests PATCH v2] alloc: Add memalign error checks

[David Hildenbrand]

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

Re: [kvm-unit-tests PATCH v2] alloc: Add memalign error checks

[Janosch Frank]

[-- 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 --]

Re: [kvm-unit-tests PATCH v2] alloc: Add memalign error checks

[Andrew Jones]

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

Re: [kvm-unit-tests PATCH v2] alloc: Add memalign error checks

[Paolo Bonzini]

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

Re: [kvm-unit-tests PATCH v2] alloc: Add memalign error checks

[Janosch Frank]

[-- 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 --]

Re: [kvm-unit-tests PATCH v2] alloc: Add memalign error checks

[Paolo Bonzini]

[-- 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 --]