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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAG_fn=VwrB3sb9RvMdj0qnafnbNONASTpkxj0zSE7spdEVi7hw@mail.gmail.com>
Date:	Wed, 4 May 2016 10:34:05 +0200
From:	Alexander Potapenko <glider@...gle.com>
To:	Joonsoo Kim <js1304@...il.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Andrey Ryabinin <aryabinin@...tuozzo.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Linux Memory Management List <linux-mm@...ck.org>,
	Joonsoo Kim <iamjoonsoo.kim@....com>
Subject: Re: [PATCH for v4.6] lib/stackdepot: avoid to return 0 handle

On Tue, May 3, 2016 at 7:13 AM,  <js1304@...il.com> wrote:
> From: Joonsoo Kim <iamjoonsoo.kim@....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@....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