[PATCH for v4.6] lib/stackdepot: avoid to return 0 handle

[PATCH for v4.6] lib/stackdepot: avoid to return 0 handle

[js1304]

From: Joonsoo Kim <iamjoonsoo.kim@lge.com>

Recently, we allow to save the stacktrace whose hashed value is 0.
It causes the problem that stackdepot could return 0 even if in success.
User of stackdepot cannot distinguish whether it is success or not so we
need to solve this problem. In this patch, 1 bit are added to handle
and make valid handle none 0 by setting this bit. After that, valid handle
will not be 0 and 0 handle will represent failure correctly.

Fixes: 33334e25769c ("lib/stackdepot.c: allow the stack trace hash
to be zero")
Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
---
 lib/stackdepot.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lib/stackdepot.c b/lib/stackdepot.c
index 9e0b031..53ad6c0 100644
--- a/lib/stackdepot.c
+++ b/lib/stackdepot.c
@@ -42,12 +42,14 @@
 
 #define DEPOT_STACK_BITS (sizeof(depot_stack_handle_t) * 8)
 
+#define STACK_ALLOC_NULL_PROTECTION_BITS 1
 #define STACK_ALLOC_ORDER 2 /* 'Slab' size order for stack depot, 4 pages */
 #define STACK_ALLOC_SIZE (1LL << (PAGE_SHIFT + STACK_ALLOC_ORDER))
 #define STACK_ALLOC_ALIGN 4
 #define STACK_ALLOC_OFFSET_BITS (STACK_ALLOC_ORDER + PAGE_SHIFT - \
 					STACK_ALLOC_ALIGN)
-#define STACK_ALLOC_INDEX_BITS (DEPOT_STACK_BITS - STACK_ALLOC_OFFSET_BITS)
+#define STACK_ALLOC_INDEX_BITS (DEPOT_STACK_BITS - \
+		STACK_ALLOC_NULL_PROTECTION_BITS - STACK_ALLOC_OFFSET_BITS)
 #define STACK_ALLOC_SLABS_CAP 1024
 #define STACK_ALLOC_MAX_SLABS \
 	(((1LL << (STACK_ALLOC_INDEX_BITS)) < STACK_ALLOC_SLABS_CAP) ? \
@@ -59,6 +61,7 @@ union handle_parts {
 	struct {
 		u32 slabindex : STACK_ALLOC_INDEX_BITS;
 		u32 offset : STACK_ALLOC_OFFSET_BITS;
+		u32 valid : STACK_ALLOC_NULL_PROTECTION_BITS;
 	};
 };
 
@@ -136,6 +139,7 @@ static struct stack_record *depot_alloc_stack(unsigned long *entries, int size,
 	stack->size = size;
 	stack->handle.slabindex = depot_index;
 	stack->handle.offset = depot_offset >> STACK_ALLOC_ALIGN;
+	stack->handle.valid = 1;
 	memcpy(stack->entries, entries, size * sizeof(unsigned long));
 	depot_offset += required_size;
 
-- 
1.9.1

--
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 for v4.6] lib/stackdepot: avoid to return 0 handle

[Alexander Potapenko]

On Tue, May 3, 2016 at 7:13 AM,  <js1304@gmail.com> wrote:
> From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>
> Recently, we allow to save the stacktrace whose hashed value is 0.
> It causes the problem that stackdepot could return 0 even if in success.
> User of stackdepot cannot distinguish whether it is success or not so we
> need to solve this problem. In this patch, 1 bit are added to handle
> and make valid handle none 0 by setting this bit. After that, valid handle
> will not be 0 and 0 handle will represent failure correctly.
Returning success or failure doesn't require a special bit, we can
just make depot_alloc_stack() return a boolean value.
If I'm understanding correctly, your primary intention is to reserve
an invalid handle value that will never collide with valid handles
returned in the future.
Can you reflect this in the description?
> Fixes: 33334e25769c ("lib/stackdepot.c: allow the stack trace hash
> to be zero")
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> ---
>  lib/stackdepot.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/lib/stackdepot.c b/lib/stackdepot.c
> index 9e0b031..53ad6c0 100644
> --- a/lib/stackdepot.c
> +++ b/lib/stackdepot.c
> @@ -42,12 +42,14 @@
>
>  #define DEPOT_STACK_BITS (sizeof(depot_stack_handle_t) * 8)
>
> +#define STACK_ALLOC_NULL_PROTECTION_BITS 1
>  #define STACK_ALLOC_ORDER 2 /* 'Slab' size order for stack depot, 4 pages */
>  #define STACK_ALLOC_SIZE (1LL << (PAGE_SHIFT + STACK_ALLOC_ORDER))
>  #define STACK_ALLOC_ALIGN 4
>  #define STACK_ALLOC_OFFSET_BITS (STACK_ALLOC_ORDER + PAGE_SHIFT - \
>                                         STACK_ALLOC_ALIGN)
> -#define STACK_ALLOC_INDEX_BITS (DEPOT_STACK_BITS - STACK_ALLOC_OFFSET_BITS)
> +#define STACK_ALLOC_INDEX_BITS (DEPOT_STACK_BITS - \
> +               STACK_ALLOC_NULL_PROTECTION_BITS - STACK_ALLOC_OFFSET_BITS)
>  #define STACK_ALLOC_SLABS_CAP 1024
>  #define STACK_ALLOC_MAX_SLABS \
>         (((1LL << (STACK_ALLOC_INDEX_BITS)) < STACK_ALLOC_SLABS_CAP) ? \
> @@ -59,6 +61,7 @@ union handle_parts {
>         struct {
>                 u32 slabindex : STACK_ALLOC_INDEX_BITS;
>                 u32 offset : STACK_ALLOC_OFFSET_BITS;
> +               u32 valid : STACK_ALLOC_NULL_PROTECTION_BITS;
>         };
>  };
>
> @@ -136,6 +139,7 @@ static struct stack_record *depot_alloc_stack(unsigned long *entries, int size,
>         stack->size = size;
>         stack->handle.slabindex = depot_index;
>         stack->handle.offset = depot_offset >> STACK_ALLOC_ALIGN;
> +       stack->handle.valid = 1;
>         memcpy(stack->entries, entries, size * sizeof(unsigned long));
>         depot_offset += required_size;
>
> --
> 1.9.1
>



-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Matthew Scott Sucherman, Paul Terence Manicle
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

--
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 for v4.6] lib/stackdepot: avoid to return 0 handle

[Joonsoo Kim]

2016-05-04 17:34 GMT+09:00 Alexander Potapenko <glider@google.com>:
> On Tue, May 3, 2016 at 7:13 AM,  <js1304@gmail.com> wrote:
>> From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>>
>> Recently, we allow to save the stacktrace whose hashed value is 0.
>> It causes the problem that stackdepot could return 0 even if in success.
>> User of stackdepot cannot distinguish whether it is success or not so we
>> need to solve this problem. In this patch, 1 bit are added to handle
>> and make valid handle none 0 by setting this bit. After that, valid handle
>> will not be 0 and 0 handle will represent failure correctly.
> Returning success or failure doesn't require a special bit, we can
> just make depot_alloc_stack() return a boolean value.
> If I'm understanding correctly, your primary intention is to reserve
> an invalid handle value that will never collide with valid handles
> returned in the future.
> Can you reflect this in the description?

Indeed. I will add it in the description and respin the patch.

Thanks.

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