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

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

[Chen Gang]

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 <gang.chen@asianux.com>
---
 mm/zbud.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/mm/zbud.c b/mm/zbud.c
index 9451361..b5363ea 100644
--- a/mm/zbud.c
+++ b/mm/zbud.c
@@ -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)
 {
-- 
1.7.7.6

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

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

[Seth Jennings]

On Tue, Aug 20, 2013 at 12:16:53PM +0800, Chen Gang wrote:
> 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.

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.

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

> 
>   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 <gang.chen@asianux.com>
> ---
>  mm/zbud.c |   12 +++++++++---
>  1 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/zbud.c b/mm/zbud.c
> index 9451361..b5363ea 100644
> --- a/mm/zbud.c
> +++ b/mm/zbud.c
> @@ -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)
>  {
> -- 
> 1.7.7.6
> 

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

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

[Chen Gang]

On 08/20/2013 11:29 PM, Seth Jennings wrote:
> On Tue, Aug 20, 2013 at 12:16:53PM +0800, Chen Gang wrote:
>> 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.
> 
> Didn't quite understand you here.  kfree() is able to handle a NULL
> pointer, but double frees are not allowed.
> 

It seems, we don't allow the caller double frees, either.

Is it correct ?


> Proper refcounting of your allocations can protect against double frees.
>

So the caller need additional value to mark current status (e.g refcounting)

And can the caller know about whether succeed or not after call zbud_map() ?


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

Also include NULL ?


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

Yes, but it let the caller can not use this interface, easily.


> Not really seeing the point of this change.
> 

So need additional discussion.  :-)


Thanks.

> Seth
> 
>>
>>   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 <gang.chen@asianux.com>
>> ---
>>  mm/zbud.c |   12 +++++++++---
>>  1 files changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/zbud.c b/mm/zbud.c
>> index 9451361..b5363ea 100644
>> --- a/mm/zbud.c
>> +++ b/mm/zbud.c
>> @@ -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)
>>  {
>> -- 
>> 1.7.7.6
>>
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> 


-- 
Chen Gang

-- 
Chen Gang

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