alloc: Add memalign error checks

PATCH In review Janosch Frank 4 participants 10 messages Nov 4, 2019 – Nov 11, 2019

Subsystems (2)

KVM Library code

lore

Share & tools

lore

Janosch Frank

author

2019-11-04 10:29 UTC #0 lore raw

PATCH v2 1/1 alloc: Add memalign error checks

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

---
 lib/alloc.c | 3 +++
 1 file changed, 3 insertions(+)

lib/alloc.c +3 -0

index ecdbbc4..b763c70 100644
@@ -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;

Signature · 1 line

2.20.1

David Hildenbrand

Reviewed-by on v2 1/1

2019-11-04 10:33 UTC #1 lore raw

Quoted 14 lines · 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 <[email protected]>
---
  lib/alloc.c | 3 +++
  1 file changed, 3 insertions(+)
lib/alloc.c +0 -0
index ecdbbc4..b763c70 100644

lib/alloc.c void *memalign(size_t alignment, size_t size) · line 53

Quoted 7 lines

lib/alloc.c +2 -0

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

lib/alloc.c void *memalign(size_t alignment, size_t size) · line 63

Quoted 10 lines

lib/alloc.c +1 -0

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

Signature · 3 lines

Thanks,

David / dhildenb

Andrew Jones

2019-11-04 10:54 UTC #2 lore raw

Quoted 24 lines · David Hildenbrand

Quoted 14 lines · 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 <[email protected]>
---
  lib/alloc.c | 3 +++
  1 file changed, 3 insertions(+)
lib/alloc.c +0 -0
index ecdbbc4..b763c70 100644
lib/alloc.c void *memalign(size_t alignment, size_t size) · line 52
Quoted 6 lines
lib/alloc.c +2 -0
@@ -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

Quoted 3 lines

"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.

Quoted 1 line

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

lib/alloc.c +6 -4

index ecdbbc44dbf9..ed8f5f94c9b0 100644
@@ -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

maintainer

2019-11-04 11:29 UTC #3 lore raw

Quoted 17 lines · Andrew Jones

lib/alloc.c +4 -3

index ecdbbc44dbf9..ed8f5f94c9b0 100644
@@ -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

lib/alloc.c void *memalign(size_t alignment, size_t size) · line 62

Quoted 10 lines

lib/alloc.c +2 -1

-		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

Reviewed-by

2019-11-04 11:31 UTC #4 lore raw

Quoted 26 lines · Paolo Bonzini

Quoted 25 lines · Andrew Jones

lib/alloc.c +6 -4

index ecdbbc44dbf9..ed8f5f94c9b0 100644
@@ -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);

Quoted 7 lines

lib/alloc.c void *memalign(size_t alignment, size_t size) · line 62
Quoted 2 lines
lib/alloc.c +0 -0
  	/* 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 <[email protected]>

Signature · 3 lines

Thanks,

David / dhildenb

Janosch Frank

author

2019-11-11 10:12 UTC #5 lore raw

In reply to Paolo Bonzini

Quoted 20 lines · Paolo Bonzini

Quoted 19 lines · Andrew Jones

lib/alloc.c +4 -4

index ecdbbc44dbf9..ed8f5f94c9b0 100644
@@ -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;

Quoted 5 lines

lib/alloc.c void *memalign(size_t alignment, size_t size) · line 59

Quoted 5 lines

lib/alloc.c +2 -0

+	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.

Quoted 7 lines

lib/alloc.c void *memalign(size_t alignment, size_t size) · line 62
Quoted 3 lines
lib/alloc.c +0 -0
 	/* 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

2019-11-11 12:31 UTC #6 lore raw

Quoted 21 lines · Janosch Frank

Quoted 20 lines · Paolo Bonzini

Quoted 19 lines · Andrew Jones

lib/alloc.c +4 -4

index ecdbbc44dbf9..ed8f5f94c9b0 100644
@@ -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;

Quoted 9 lines

Quoted 5 lines
lib/alloc.c void *memalign(size_t alignment, size_t size) · line 59
Quoted 5 lines
lib/alloc.c +2 -0
+	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

maintainer

2019-11-11 13:04 UTC #7 lore raw

Quoted 5 lines · Andrew Jones

Quoted 3 lines
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

author

2019-11-11 13:13 UTC #8 lore raw

Quoted 11 lines · Paolo Bonzini

Quoted 5 lines · Andrew Jones
Quoted 3 lines
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

maintainer

2019-11-11 13:17 UTC #9 lore raw

Quoted 5 lines · Janosch Frank

Quoted 4 lines · Paolo Bonzini
Quoted 3 lines · Andrew Jones
Quoted 3 lines
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.

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
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