linux-kernel - Re: [PATCH 3/3] slub: add hooks for kmemcheck

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: <19f34abd0804181527i3b65e82bk2a5d8b3c2b196f9e@mail.gmail.com>
Date:	Sat, 19 Apr 2008 00:27:44 +0200
From:	"Vegard Nossum" <vegard.nossum@...il.com>
To:	"Christoph Lameter" <clameter@....com>
Cc:	"Andrew Morton" <akpm@...ux-foundation.org>,
	"Pekka Enberg" <penberg@...helsinki.fi>,
	"Ingo Molnar" <mingo@...e.hu>, "Andi Kleen" <andi@...stfloor.org>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] slub: add hooks for kmemcheck

Thank you for your comments.

On Sat, Apr 19, 2008 at 12:12 AM, Christoph Lameter <clameter@....com> wrote:
> On Sat, 19 Apr 2008, Vegard Nossum wrote:
>
>  > index f62caaa..d5505b1 100644
>  > --- a/include/linux/slab.h
>  > +++ b/include/linux/slab.h
>  > @@ -29,6 +29,13 @@
>  >  #define SLAB_MEM_SPREAD              0x00100000UL    /* Spread some memory over cpuset */
>  >  #define SLAB_TRACE           0x00200000UL    /* Trace allocations and frees */
>  >
>  > +#ifdef CONFIG_KMEMCHECK
>  > +/* Don't track use of uninitialized memory */
>  > +# define SLAB_NOTRACK                0x00400000UL
>  > +#else
>  > +# define SLAB_NOTRACK                0
>  > +#endif
>  > +
>
>  It maybe easier to just avoid the #ifdef. The other debug flags also only
>  have a function if certain kernel config options are set.

We set it to zero to allow the compiler to optimize out code if
CONFIG_KMEMCHECK=n. For instance

        if (kmemcheck_page_is_tracked(page) && !(s->flags & SLAB_NOTRACK)) {

Will be turned into if(0 && !(s->flags & 0)), which can be completely
optimized away. On the other hand, if SLAB_NOTRACK is non-zero, this
is not the case, since the flag must be tested.

>  > +#ifdef CONFIG_KMEMCHECK
>  > +struct page *kmemcheck_allocate_slab(struct kmem_cache *s,
>  > +     gfp_t flags, int node, int pages);
>  > +void kmemcheck_free_slab(struct kmem_cache *s, struct page *page, int pages);
>  > +
>  > +void kmemcheck_slab_alloc(struct kmem_cache *s, gfp_t gfpflags, void *object);
>  > +void kmemcheck_slab_free(struct kmem_cache *s, void *object);
>  > +#else
>  > +static inline struct page *kmemcheck_allocate_slab(struct kmem_cache *s,
>  > +     gfp_t flags, int node, int pages) { return NULL; }
>  > +static inline void kmemcheck_free_slab(struct kmem_cache *s,
>  > +     struct page *page, int pages) { }
>  > +static inline void kmemcheck_slab_alloc(struct kmem_cache *s,
>  > +     gfp_t gfpflags, void *object) { }
>  > +static inline void kmemcheck_slab_free(struct kmem_cache *s, void *object) { }
>  > +#endif /* CONFIG_KMEMCHECK */
>  > +
>
>  Should this not go into some kmemcheck.h file?

The implementations of these prototypes are in mm/slub_kmemcheck.c.
They are only ever called from slub.c since they represent the
interface between SLUB and kmemcheck.

Would you rather have it in include/linux/slub_kmemcheck.h?

>  >       }
>  >
>  > +     if (kmemcheck_page_is_tracked(page) && !(s->flags & SLAB_NOTRACK)) {
>  > +             kmemcheck_free_slab(s, page, pages);
>  > +             return;
>  > +     }
>  > +
>  > +     __ClearPageSlab(page);
>  > +
>
>  Why is __ClearPageSlab moved?

It has been moved because the flag should only be cleared just before
it is returned to the page allocator.

Currently, with SLAB/SLUB debugging enabled, the page flag will be
cleared, then accessed in check_object(), thus generating a few
accesses to the page. It is safer to delay the clearing of the page
flag until no more accesses can be generated for the page.

>  > @@ -2449,12 +2462,10 @@ static int __init setup_slub_nomerge(char *str)
>  >  __setup("slub_nomerge", setup_slub_nomerge);
>  >
>  >  static struct kmem_cache *create_kmalloc_cache(struct kmem_cache *s,
>  > -             const char *name, int size, gfp_t gfp_flags)
>  > +     const char *name, int size, gfp_t gfp_flags, unsigned int flags)
>
>  The second flags parameter is always 0?

Hm, let's see... Yep, you are correct. This is an artifact from
previous versions, where this flag was actually used, and should now
be removed!

Thanks!

Vegard


-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/