[kvm-unit-tests,v2] alloc: Add memalign error checks
diff mbox series

Message ID 20191104102916.10554-1-frankja@linux.ibm.com
State New
Headers show
Series
  • [kvm-unit-tests,v2] alloc: Add memalign error checks
Related show

Commit Message

Janosch Frank Nov. 4, 2019, 10:29 a.m. UTC
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(+)

Comments

David Hildenbrand Nov. 4, 2019, 10:33 a.m. UTC | #1
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>
Andrew Jones Nov. 4, 2019, 10:54 a.m. UTC | #2
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;
Paolo Bonzini Nov. 4, 2019, 11:29 a.m. UTC | #3
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
David Hildenbrand Nov. 4, 2019, 11:31 a.m. UTC | #4
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>
Janosch Frank Nov. 11, 2019, 10:12 a.m. UTC | #5
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
Andrew Jones Nov. 11, 2019, 12:31 p.m. UTC | #6
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
Paolo Bonzini Nov. 11, 2019, 1:04 p.m. UTC | #7
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
Janosch Frank Nov. 11, 2019, 1:13 p.m. UTC | #8
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?
Paolo Bonzini Nov. 11, 2019, 1:17 p.m. UTC | #9
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

Patch
diff mbox series

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;