mm/zbud.c: consider about the invalid handle value for handle related extern functions.

For handle related extern functions, recommend to consider about invalid handle value, or can not be easily used by callers.

In our case:

if need call zbud_alloc/free() multiple times, the caller need additional value to mark current status. the caller can not continue call zbud_free() multiple times like kfree().
when call zbud_map(), the caller can not know about whether succeed or not.

And NULL (or 0) should be also treated as invalid handle value, since the internal implementation has already assumed NULL is an invalid address.

e.g. "struct zbud_header *zhdr = NULL;" in function 'zbud_alloc'.

At least, current interface definition still has no bugs, so the common patch appliers is better to keep compatible with the original interface.

And related maintainers can re-construct interface without considering the compatibility at a suitable time point, so the interface can be get additional improvement.

e.g. for handle's type, "void *" is more commonly used than "unsigned long".
can find support macros or functions in "include/linux/err.h" for "void *".
but can not find any support macros or functions for "unsigned long" in "./include" sub directory.

e.g. for zbud_alloc(), can return a handle directly instead of an error code.

Signed-off-by Chen Gang <[email protected]>

---
 mm/zbud.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

mm/zbud.c +9 -3

index 9451361..b5363ea 100644
@@ -244,8 +244,8 @@ void zbud_destroy_pool(struct zbud_pool *pool)
  * as zbud pool pages.
  *
  * Return: 0 if success and handle is set, otherwise -EINVAL if the size or
- * gfp arguments are invalid or -ENOMEM if the pool was unable to allocate
- * a new page.
+ * gfp arguments are invalid, or -ENOMEM if the pool was unable to allocate
+ * a new page, or -NOSPC if no space left, also set invalid value to handle.
  */
 int zbud_alloc(struct zbud_pool *pool, int size, gfp_t gfp,
 			unsigned long *handle)

@@ -255,6 +255,8 @@ int zbud_alloc(struct zbud_pool *pool, int size, gfp_t gfp,
 	enum buddy bud;
 	struct page *page;
 
+	*handle = 0;
+
 	if (size <= 0 || gfp & __GFP_HIGHMEM)
 		return -EINVAL;
 	if (size > PAGE_SIZE - ZHDR_SIZE_ALIGNED - CHUNK_SIZE)

@@ -328,6 +330,9 @@ void zbud_free(struct zbud_pool *pool, unsigned long handle)
 	struct zbud_header *zhdr;
 	int freechunks;
 
+	if (IS_ERR_OR_NULL((void *)handle))
+		return;
+
 	spin_lock(&pool->lock);
 	zhdr = handle_to_zbud_header(handle);
 

@@ -478,7 +483,8 @@ next:
  * in the handle and could create temporary mappings to make the data
  * accessible to the user.
  *
- * Returns: a pointer to the mapped allocation
+ * Returns: a pointer to the mapped allocation, or an invalid value which can
+ * be checked by IS_ERR_OR_NULL().
  */
 void *zbud_map(struct zbud_pool *pool, unsigned long handle)
 {

Signature · 1 line

1.7.7.6

Quoted 3 lines · Chen Gang

For handle related extern functions, recommend to consider about invalid handle value, or can not be easily used by callers.

Quoted 5 lines

In our case:

if need call zbud_alloc/free() multiple times, the caller need additional value to mark current status. the caller can not continue call zbud_free() multiple times like kfree().
when call zbud_map(), the caller can not know about whether succeed or not.

Didn't quite understand you here. kfree() is able to handle a NULL pointer, but double frees are not allowed.

Proper refcounting of your allocations can protect against double frees.

Quoted 4 lines

And NULL (or 0) should be also treated as invalid handle value, since the internal implementation has already assumed NULL is an invalid address.

I'd like to keep the handle space open to all valid values that can be represented by an unsigned long.

The return value of zbud_alloc() will tell you if the handle is valid or not.

Not really seeing the point of this change.

Seth

Quoted 69 lines

e.g. "struct zbud_header *zhdr = NULL;" in function 'zbud_alloc'.

At least, current interface definition still has no bugs, so the common patch appliers is better to keep compatible with the original interface.

And related maintainers can re-construct interface without considering the compatibility at a suitable time point, so the interface can be get additional improvement.

e.g. for handle's type, "void *" is more commonly used than "unsigned long".
can find support macros or functions in "include/linux/err.h" for "void *".
but can not find any support macros or functions for "unsigned long" in "./include" sub directory.

e.g. for zbud_alloc(), can return a handle directly instead of an error code.

Signed-off-by Chen Gang <[email protected]>

---
 mm/zbud.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

mm/zbud.c +9 -3

index 9451361..b5363ea 100644
@@ -244,8 +244,8 @@ void zbud_destroy_pool(struct zbud_pool *pool)
  * as zbud pool pages.
  *
  * Return: 0 if success and handle is set, otherwise -EINVAL if the size or
- * gfp arguments are invalid or -ENOMEM if the pool was unable to allocate
- * a new page.
+ * gfp arguments are invalid, or -ENOMEM if the pool was unable to allocate
+ * a new page, or -NOSPC if no space left, also set invalid value to handle.
  */
 int zbud_alloc(struct zbud_pool *pool, int size, gfp_t gfp,
 			unsigned long *handle)

@@ -255,6 +255,8 @@ int zbud_alloc(struct zbud_pool *pool, int size, gfp_t gfp,
 	enum buddy bud;
 	struct page *page;
 
+	*handle = 0;
+
 	if (size <= 0 || gfp & __GFP_HIGHMEM)
 		return -EINVAL;
 	if (size > PAGE_SIZE - ZHDR_SIZE_ALIGNED - CHUNK_SIZE)

@@ -328,6 +330,9 @@ void zbud_free(struct zbud_pool *pool, unsigned long handle)
 	struct zbud_header *zhdr;
 	int freechunks;
 
+	if (IS_ERR_OR_NULL((void *)handle))
+		return;
+
 	spin_lock(&pool->lock);
 	zhdr = handle_to_zbud_header(handle);
 

@@ -478,7 +483,8 @@ next:
  * in the handle and could create temporary mappings to make the data
  * accessible to the user.
  *
- * Returns: a pointer to the mapped allocation
+ * Returns: a pointer to the mapped allocation, or an invalid value which can
+ * be checked by IS_ERR_OR_NULL().
  */
 void *zbud_map(struct zbud_pool *pool, unsigned long handle)
 {

Signature · 1 line

1.7.7.6

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>