[RFC,v1,18/26] kmsan: mm: call KMSAN hooks from SLUB code

Message ID	20191018094304.37056-19-glider@google.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <SRS0=Fj1U=YL=kvack.org=owner-linux-mm@kernel.org> DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 83D2020820 Date: Fri, 18 Oct 2019 11:42:56 +0200 In-Reply-To: <20191018094304.37056-1-glider@google.com> Message-Id: <20191018094304.37056-19-glider@google.com> Mime-Version: 1.0 References: <20191018094304.37056-1-glider@google.com> Subject: [PATCH RFC v1 18/26] kmsan: mm: call KMSAN hooks from SLUB code From: glider@google.com To: Alexander Potapenko <glider@google.com> Cc: Andrew Morton <akpm@linux-foundation.org>, Vegard Nossum <vegard.nossum@oracle.com>, Dmitry Vyukov <dvyukov@google.com>, linux-mm@kvack.org Content-Type: text/plain; charset="UTF-8" Sender: owner-linux-mm@kvack.org Precedence: bulk
Series	Add KernelMemorySanitizer infrastructure \| expand [RFC,v1,00/26] Add KernelMemorySanitizer infrastructure [RFC,v1,01/26] stackdepot: check depot_index before accessing the stack slab [RFC,v1,02/26] stackdepot: prevent Clang from optimizing away stackdepot_memcmp() [RFC,v1,03/26] kasan: stackdepot: move filter_irq_stacks() to stackdepot.c [RFC,v1,04/26] stackdepot: reserve 5 extra bits in depot_stack_handle_t [RFC,v1,05/26] printk_safe: externalize printk_context [RFC,v1,06/26] kasan: compiler.h: rename __no_kasan_or_inline into __no_memory_tool_or_inline [RFC,v1,07/26] kmsan: add ReST documentation [RFC,v1,08/26] kmsan: gfp: introduce __GFP_NO_KMSAN_SHADOW [RFC,v1,09/26] kmsan: introduce __no_sanitize_memory and __SANITIZE_MEMORY__ [RFC,v1,10/26] kmsan: reduce vmalloc space [RFC,v1,11/26] kmsan: add KMSAN runtime [RFC,v1,12/26] kmsan: x86: sync metadata pages on page fault [RFC,v1,13/26] kmsan: add tests for KMSAN [RFC,v1,14/26] kmsan: make READ_ONCE_TASK_STACK() return initialized values [RFC,v1,15/26] kmsan: Kconfig changes to disable options incompatible with KMSAN [RFC,v1,16/26] kmsan: Changing existing files to enable KMSAN builds [RFC,v1,17/26] kmsan: disable KMSAN instrumentation for certain kernel parts [RFC,v1,18/26] kmsan: mm: call KMSAN hooks from SLUB code [RFC,v1,19/26] kmsan: call KMSAN hooks where needed [RFC,v1,20/26] kmsan: disable instrumentation of certain functions [RFC,v1,21/26] kmsan: unpoison \|tlb\| in arch_tlb_gather_mmu() [RFC,v1,22/26] kmsan: use __msan_memcpy() where possible. [RFC,v1,23/26] kmsan: unpoisoning buffers from devices etc. [RFC,v1,24/26] kmsan: hooks for copy_to_user() and friends [RFC,v1,25/26] kmsan: disable strscpy() optimization under KMSAN [RFC,v1,26/26] net: kasan: kmsan: support CONFIG_GENERIC_CSUM on x86, enable it for KASAN/KMSAN

Alexander Potapenko Oct. 18, 2019, 9:42 a.m. UTC

In order to report uninitialized memory coming from heap allocations
KMSAN has to poison them unless they're created with __GFP_ZERO.

It's handy that we need KMSAN hooks in the places where
init_on_alloc/init_on_free initialization is performed.

Signed-off-by: Alexander Potapenko <glider@google.com>
To: Alexander Potapenko <glider@google.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Vegard Nossum <vegard.nossum@oracle.com>
Cc: Dmitry Vyukov <dvyukov@google.com>
Cc: linux-mm@kvack.org
---

Change-Id: I51103b7981d3aabed747d0c85cbdc85568665871
---
 mm/slub.c | 37 +++++++++++++++++++++++++++++++------
 1 file changed, 31 insertions(+), 6 deletions(-)

Qian Cai Oct. 18, 2019, 1:22 p.m. UTC | #1

On Fri, 2019-10-18 at 11:42 +0200, glider@google.com wrote:
> In order to report uninitialized memory coming from heap allocations
> KMSAN has to poison them unless they're created with __GFP_ZERO.
> 
> It's handy that we need KMSAN hooks in the places where
> init_on_alloc/init_on_free initialization is performed.

Well, there is SLUB debug which has red zoning and poisoning checks. What's
value of this patch?

> 
> Signed-off-by: Alexander Potapenko <glider@google.com>
> To: Alexander Potapenko <glider@google.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Vegard Nossum <vegard.nossum@oracle.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: linux-mm@kvack.org
> ---
> 
> Change-Id: I51103b7981d3aabed747d0c85cbdc85568665871
> ---
>  mm/slub.c | 37 +++++++++++++++++++++++++++++++------
>  1 file changed, 31 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 3d63ae320d31..3d6d4c63446e 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -21,6 +21,8 @@
>  #include <linux/proc_fs.h>
>  #include <linux/seq_file.h>
>  #include <linux/kasan.h>
> +#include <linux/kmsan.h>
> +#include <linux/kmsan-checks.h> /* KMSAN_INIT_VALUE */
>  #include <linux/cpu.h>
>  #include <linux/cpuset.h>
>  #include <linux/mempolicy.h>
> @@ -285,17 +287,27 @@ static void prefetch_freepointer(const struct kmem_cache *s, void *object)
>  	prefetch(object + s->offset);
>  }
>  
> +/*
> + * When running under KMSAN, get_freepointer_safe() may return an uninitialized
> + * pointer value in the case the current thread loses the race for the next
> + * memory chunk in the freelist. In that case this_cpu_cmpxchg_double() in
> + * slab_alloc_node() will fail, so the uninitialized value won't be used, but
> + * KMSAN will still check all arguments of cmpxchg because of imperfect
> + * handling of inline assembly.
> + * To work around this problem, use KMSAN_INIT_VALUE() to force initialize the
> + * return value of get_freepointer_safe().
> + */
>  static inline void *get_freepointer_safe(struct kmem_cache *s, void *object)
>  {
>  	unsigned long freepointer_addr;
>  	void *p;
>  
>  	if (!debug_pagealloc_enabled())
> -		return get_freepointer(s, object);
> +		return KMSAN_INIT_VALUE(get_freepointer(s, object));
>  
>  	freepointer_addr = (unsigned long)object + s->offset;
>  	probe_kernel_read(&p, (void **)freepointer_addr, sizeof(p));
> -	return freelist_ptr(s, p, freepointer_addr);
> +	return KMSAN_INIT_VALUE(freelist_ptr(s, p, freepointer_addr));
>  }
>  
>  static inline void set_freepointer(struct kmem_cache *s, void *object, void *fp)
> @@ -1390,6 +1402,7 @@ static inline void *kmalloc_large_node_hook(void *ptr, size_t size, gfp_t flags)
>  	ptr = kasan_kmalloc_large(ptr, size, flags);
>  	/* As ptr might get tagged, call kmemleak hook after KASAN. */
>  	kmemleak_alloc(ptr, size, 1, flags);
> +	kmsan_kmalloc_large(ptr, size, flags);
>  	return ptr;
>  }
>  
> @@ -1397,6 +1410,7 @@ static __always_inline void kfree_hook(void *x)
>  {
>  	kmemleak_free(x);
>  	kasan_kfree_large(x, _RET_IP_);
> +	kmsan_kfree_large(x);
>  }
>  
>  static __always_inline bool slab_free_hook(struct kmem_cache *s, void *x)
> @@ -1453,6 +1467,12 @@ static inline bool slab_free_freelist_hook(struct kmem_cache *s,
>  		} while (object != old_tail);
>  	}
>  
> +	do {
> +		object = next;
> +		next = get_freepointer(s, object);
> +		kmsan_slab_free(s, object);
> +	} while (object != old_tail);
> +
>  /*
>   * Compiler cannot detect this function can be removed if slab_free_hook()
>   * evaluates to nothing.  Thus, catch all relevant config debug options here.
> @@ -2769,6 +2789,7 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s,
>  	if (unlikely(slab_want_init_on_alloc(gfpflags, s)) && object)
>  		memset(object, 0, s->object_size);
>  
> +	kmsan_slab_alloc(s, object, gfpflags);
>  	slab_post_alloc_hook(s, gfpflags, 1, &object);
>  
>  	return object;
> @@ -2797,6 +2818,7 @@ void *kmem_cache_alloc_trace(struct kmem_cache *s, gfp_t gfpflags, size_t size)
>  	void *ret = slab_alloc(s, gfpflags, _RET_IP_);
>  	trace_kmalloc(_RET_IP_, ret, size, s->size, gfpflags);
>  	ret = kasan_kmalloc(s, ret, size, gfpflags);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL(kmem_cache_alloc_trace);
> @@ -2809,7 +2831,6 @@ void *kmem_cache_alloc_node(struct kmem_cache *s, gfp_t gfpflags, int node)
>  
>  	trace_kmem_cache_alloc_node(_RET_IP_, ret,
>  				    s->object_size, s->size, gfpflags, node);
> -
>  	return ret;
>  }
>  EXPORT_SYMBOL(kmem_cache_alloc_node);
> @@ -2825,6 +2846,7 @@ void *kmem_cache_alloc_node_trace(struct kmem_cache *s,
>  			   size, s->size, gfpflags, node);
>  
>  	ret = kasan_kmalloc(s, ret, size, gfpflags);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL(kmem_cache_alloc_node_trace);
> @@ -3150,7 +3172,7 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
>  			  void **p)
>  {
>  	struct kmem_cache_cpu *c;
> -	int i;
> +	int i, j;
>  
>  	/* memcg and kmem_cache debug support */
>  	s = slab_pre_alloc_hook(s, flags);
> @@ -3188,11 +3210,11 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
>  
>  	/* Clear memory outside IRQ disabled fastpath loop */
>  	if (unlikely(slab_want_init_on_alloc(flags, s))) {
> -		int j;
> -
>  		for (j = 0; j < i; j++)
>  			memset(p[j], 0, s->object_size);
>  	}
> +	for (j = 0; j < i; j++)
> +		kmsan_slab_alloc(s, p[j], flags);
>  
>  	/* memcg and kmem_cache debug support */
>  	slab_post_alloc_hook(s, flags, size, p);
> @@ -3793,6 +3815,7 @@ static int __init setup_slub_min_objects(char *str)
>  
>  __setup("slub_min_objects=", setup_slub_min_objects);
>  
> +__no_sanitize_memory
>  void *__kmalloc(size_t size, gfp_t flags)
>  {
>  	struct kmem_cache *s;
> @@ -5698,6 +5721,7 @@ static char *create_unique_id(struct kmem_cache *s)
>  	p += sprintf(p, "%07u", s->size);
>  
>  	BUG_ON(p > name + ID_STR_LENGTH - 1);
> +	kmsan_unpoison_shadow(name, p - name);
>  	return name;
>  }
>  
> @@ -5847,6 +5871,7 @@ static int sysfs_slab_alias(struct kmem_cache *s, const char *name)
>  	al->name = name;
>  	al->next = alias_list;
>  	alias_list = al;
> +	kmsan_unpoison_shadow(al, sizeof(struct saved_alias));
>  	return 0;
>  }
>

Alexander Potapenko Oct. 18, 2019, 1:33 p.m. UTC | #2

On Fri, Oct 18, 2019 at 3:22 PM Qian Cai <cai@lca.pw> wrote:
>
> On Fri, 2019-10-18 at 11:42 +0200, glider@google.com wrote:
> > In order to report uninitialized memory coming from heap allocations
> > KMSAN has to poison them unless they're created with __GFP_ZERO.
> >
> > It's handy that we need KMSAN hooks in the places where
> > init_on_alloc/init_on_free initialization is performed.
>
> Well, there is SLUB debug which has red zoning and poisoning checks. What's
> value of this patch?
Sorry, are you talking about the whole patchset or just this patch?
Note that SLUB debug is unable to detect uninitialized values with
bit-to-bit precision, neither have I heard of anyone using it for
detecting uses of uninitialized memory in the kernel at all.
The purpose of SLUB debug is to detect corruptions of freed memory.
> >
> > Signed-off-by: Alexander Potapenko <glider@google.com>
> > To: Alexander Potapenko <glider@google.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Vegard Nossum <vegard.nossum@oracle.com>
> > Cc: Dmitry Vyukov <dvyukov@google.com>
> > Cc: linux-mm@kvack.org
> > ---
> >
> > Change-Id: I51103b7981d3aabed747d0c85cbdc85568665871
> > ---
> >  mm/slub.c | 37 +++++++++++++++++++++++++++++++------
> >  1 file changed, 31 insertions(+), 6 deletions(-)
> >
> > diff --git a/mm/slub.c b/mm/slub.c
> > index 3d63ae320d31..3d6d4c63446e 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -21,6 +21,8 @@
> >  #include <linux/proc_fs.h>
> >  #include <linux/seq_file.h>
> >  #include <linux/kasan.h>
> > +#include <linux/kmsan.h>
> > +#include <linux/kmsan-checks.h> /* KMSAN_INIT_VALUE */
> >  #include <linux/cpu.h>
> >  #include <linux/cpuset.h>
> >  #include <linux/mempolicy.h>
> > @@ -285,17 +287,27 @@ static void prefetch_freepointer(const struct kmem_cache *s, void *object)
> >       prefetch(object + s->offset);
> >  }
> >
> > +/*
> > + * When running under KMSAN, get_freepointer_safe() may return an uninitialized
> > + * pointer value in the case the current thread loses the race for the next
> > + * memory chunk in the freelist. In that case this_cpu_cmpxchg_double() in
> > + * slab_alloc_node() will fail, so the uninitialized value won't be used, but
> > + * KMSAN will still check all arguments of cmpxchg because of imperfect
> > + * handling of inline assembly.
> > + * To work around this problem, use KMSAN_INIT_VALUE() to force initialize the
> > + * return value of get_freepointer_safe().
> > + */
> >  static inline void *get_freepointer_safe(struct kmem_cache *s, void *object)
> >  {
> >       unsigned long freepointer_addr;
> >       void *p;
> >
> >       if (!debug_pagealloc_enabled())
> > -             return get_freepointer(s, object);
> > +             return KMSAN_INIT_VALUE(get_freepointer(s, object));
> >
> >       freepointer_addr = (unsigned long)object + s->offset;
> >       probe_kernel_read(&p, (void **)freepointer_addr, sizeof(p));
> > -     return freelist_ptr(s, p, freepointer_addr);
> > +     return KMSAN_INIT_VALUE(freelist_ptr(s, p, freepointer_addr));
> >  }
> >
> >  static inline void set_freepointer(struct kmem_cache *s, void *object, void *fp)
> > @@ -1390,6 +1402,7 @@ static inline void *kmalloc_large_node_hook(void *ptr, size_t size, gfp_t flags)
> >       ptr = kasan_kmalloc_large(ptr, size, flags);
> >       /* As ptr might get tagged, call kmemleak hook after KASAN. */
> >       kmemleak_alloc(ptr, size, 1, flags);
> > +     kmsan_kmalloc_large(ptr, size, flags);
> >       return ptr;
> >  }
> >
> > @@ -1397,6 +1410,7 @@ static __always_inline void kfree_hook(void *x)
> >  {
> >       kmemleak_free(x);
> >       kasan_kfree_large(x, _RET_IP_);
> > +     kmsan_kfree_large(x);
> >  }
> >
> >  static __always_inline bool slab_free_hook(struct kmem_cache *s, void *x)
> > @@ -1453,6 +1467,12 @@ static inline bool slab_free_freelist_hook(struct kmem_cache *s,
> >               } while (object != old_tail);
> >       }
> >
> > +     do {
> > +             object = next;
> > +             next = get_freepointer(s, object);
> > +             kmsan_slab_free(s, object);
> > +     } while (object != old_tail);
> > +
> >  /*
> >   * Compiler cannot detect this function can be removed if slab_free_hook()
> >   * evaluates to nothing.  Thus, catch all relevant config debug options here.
> > @@ -2769,6 +2789,7 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s,
> >       if (unlikely(slab_want_init_on_alloc(gfpflags, s)) && object)
> >               memset(object, 0, s->object_size);
> >
> > +     kmsan_slab_alloc(s, object, gfpflags);
> >       slab_post_alloc_hook(s, gfpflags, 1, &object);
> >
> >       return object;
> > @@ -2797,6 +2818,7 @@ void *kmem_cache_alloc_trace(struct kmem_cache *s, gfp_t gfpflags, size_t size)
> >       void *ret = slab_alloc(s, gfpflags, _RET_IP_);
> >       trace_kmalloc(_RET_IP_, ret, size, s->size, gfpflags);
> >       ret = kasan_kmalloc(s, ret, size, gfpflags);
> > +
> >       return ret;
> >  }
> >  EXPORT_SYMBOL(kmem_cache_alloc_trace);
> > @@ -2809,7 +2831,6 @@ void *kmem_cache_alloc_node(struct kmem_cache *s, gfp_t gfpflags, int node)
> >
> >       trace_kmem_cache_alloc_node(_RET_IP_, ret,
> >                                   s->object_size, s->size, gfpflags, node);
> > -
> >       return ret;
> >  }
> >  EXPORT_SYMBOL(kmem_cache_alloc_node);
> > @@ -2825,6 +2846,7 @@ void *kmem_cache_alloc_node_trace(struct kmem_cache *s,
> >                          size, s->size, gfpflags, node);
> >
> >       ret = kasan_kmalloc(s, ret, size, gfpflags);
> > +
> >       return ret;
> >  }
> >  EXPORT_SYMBOL(kmem_cache_alloc_node_trace);
> > @@ -3150,7 +3172,7 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
> >                         void **p)
> >  {
> >       struct kmem_cache_cpu *c;
> > -     int i;
> > +     int i, j;
> >
> >       /* memcg and kmem_cache debug support */
> >       s = slab_pre_alloc_hook(s, flags);
> > @@ -3188,11 +3210,11 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
> >
> >       /* Clear memory outside IRQ disabled fastpath loop */
> >       if (unlikely(slab_want_init_on_alloc(flags, s))) {
> > -             int j;
> > -
> >               for (j = 0; j < i; j++)
> >                       memset(p[j], 0, s->object_size);
> >       }
> > +     for (j = 0; j < i; j++)
> > +             kmsan_slab_alloc(s, p[j], flags);
> >
> >       /* memcg and kmem_cache debug support */
> >       slab_post_alloc_hook(s, flags, size, p);
> > @@ -3793,6 +3815,7 @@ static int __init setup_slub_min_objects(char *str)
> >
> >  __setup("slub_min_objects=", setup_slub_min_objects);
> >
> > +__no_sanitize_memory
> >  void *__kmalloc(size_t size, gfp_t flags)
> >  {
> >       struct kmem_cache *s;
> > @@ -5698,6 +5721,7 @@ static char *create_unique_id(struct kmem_cache *s)
> >       p += sprintf(p, "%07u", s->size);
> >
> >       BUG_ON(p > name + ID_STR_LENGTH - 1);
> > +     kmsan_unpoison_shadow(name, p - name);
> >       return name;
> >  }
> >
> > @@ -5847,6 +5871,7 @@ static int sysfs_slab_alias(struct kmem_cache *s, const char *name)
> >       al->name = name;
> >       al->next = alias_list;
> >       alias_list = al;
> > +     kmsan_unpoison_shadow(al, sizeof(struct saved_alias));
> >       return 0;
> >  }
> >

Qian Cai Oct. 18, 2019, 1:41 p.m. UTC | #3

On Fri, 2019-10-18 at 15:33 +0200, Alexander Potapenko wrote:
> On Fri, Oct 18, 2019 at 3:22 PM Qian Cai <cai@lca.pw> wrote:
> > 
> > On Fri, 2019-10-18 at 11:42 +0200, glider@google.com wrote:
> > > In order to report uninitialized memory coming from heap allocations
> > > KMSAN has to poison them unless they're created with __GFP_ZERO.
> > > 
> > > It's handy that we need KMSAN hooks in the places where
> > > init_on_alloc/init_on_free initialization is performed.
> > 
> > Well, there is SLUB debug which has red zoning and poisoning checks. What's
> > value of this patch?
> 
> Sorry, are you talking about the whole patchset or just this patch?

Just this patch.

> Note that SLUB debug is unable to detect uninitialized values with
> bit-to-bit precision, neither have I heard of anyone using it for
> detecting uses of uninitialized memory in the kernel at all.
> The purpose of SLUB debug is to detect corruptions of freed memory.

The point is if developers have SLUB debug enabled, all the free objects will be
poisoned, so what's the point of checking uninitialized memory there?

> > > 
> > > Signed-off-by: Alexander Potapenko <glider@google.com>
> > > To: Alexander Potapenko <glider@google.com>
> > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > Cc: Vegard Nossum <vegard.nossum@oracle.com>
> > > Cc: Dmitry Vyukov <dvyukov@google.com>
> > > Cc: linux-mm@kvack.org
> > > ---
> > > 
> > > Change-Id: I51103b7981d3aabed747d0c85cbdc85568665871
> > > ---
> > >  mm/slub.c | 37 +++++++++++++++++++++++++++++++------
> > >  1 file changed, 31 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/mm/slub.c b/mm/slub.c
> > > index 3d63ae320d31..3d6d4c63446e 100644
> > > --- a/mm/slub.c
> > > +++ b/mm/slub.c
> > > @@ -21,6 +21,8 @@
> > >  #include <linux/proc_fs.h>
> > >  #include <linux/seq_file.h>
> > >  #include <linux/kasan.h>
> > > +#include <linux/kmsan.h>
> > > +#include <linux/kmsan-checks.h> /* KMSAN_INIT_VALUE */
> > >  #include <linux/cpu.h>
> > >  #include <linux/cpuset.h>
> > >  #include <linux/mempolicy.h>
> > > @@ -285,17 +287,27 @@ static void prefetch_freepointer(const struct kmem_cache *s, void *object)
> > >       prefetch(object + s->offset);
> > >  }
> > > 
> > > +/*
> > > + * When running under KMSAN, get_freepointer_safe() may return an uninitialized
> > > + * pointer value in the case the current thread loses the race for the next
> > > + * memory chunk in the freelist. In that case this_cpu_cmpxchg_double() in
> > > + * slab_alloc_node() will fail, so the uninitialized value won't be used, but
> > > + * KMSAN will still check all arguments of cmpxchg because of imperfect
> > > + * handling of inline assembly.
> > > + * To work around this problem, use KMSAN_INIT_VALUE() to force initialize the
> > > + * return value of get_freepointer_safe().
> > > + */
> > >  static inline void *get_freepointer_safe(struct kmem_cache *s, void *object)
> > >  {
> > >       unsigned long freepointer_addr;
> > >       void *p;
> > > 
> > >       if (!debug_pagealloc_enabled())
> > > -             return get_freepointer(s, object);
> > > +             return KMSAN_INIT_VALUE(get_freepointer(s, object));
> > > 
> > >       freepointer_addr = (unsigned long)object + s->offset;
> > >       probe_kernel_read(&p, (void **)freepointer_addr, sizeof(p));
> > > -     return freelist_ptr(s, p, freepointer_addr);
> > > +     return KMSAN_INIT_VALUE(freelist_ptr(s, p, freepointer_addr));
> > >  }
> > > 
> > >  static inline void set_freepointer(struct kmem_cache *s, void *object, void *fp)
> > > @@ -1390,6 +1402,7 @@ static inline void *kmalloc_large_node_hook(void *ptr, size_t size, gfp_t flags)
> > >       ptr = kasan_kmalloc_large(ptr, size, flags);
> > >       /* As ptr might get tagged, call kmemleak hook after KASAN. */
> > >       kmemleak_alloc(ptr, size, 1, flags);
> > > +     kmsan_kmalloc_large(ptr, size, flags);
> > >       return ptr;
> > >  }
> > > 
> > > @@ -1397,6 +1410,7 @@ static __always_inline void kfree_hook(void *x)
> > >  {
> > >       kmemleak_free(x);
> > >       kasan_kfree_large(x, _RET_IP_);
> > > +     kmsan_kfree_large(x);
> > >  }
> > > 
> > >  static __always_inline bool slab_free_hook(struct kmem_cache *s, void *x)
> > > @@ -1453,6 +1467,12 @@ static inline bool slab_free_freelist_hook(struct kmem_cache *s,
> > >               } while (object != old_tail);
> > >       }
> > > 
> > > +     do {
> > > +             object = next;
> > > +             next = get_freepointer(s, object);
> > > +             kmsan_slab_free(s, object);
> > > +     } while (object != old_tail);
> > > +
> > >  /*
> > >   * Compiler cannot detect this function can be removed if slab_free_hook()
> > >   * evaluates to nothing.  Thus, catch all relevant config debug options here.
> > > @@ -2769,6 +2789,7 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s,
> > >       if (unlikely(slab_want_init_on_alloc(gfpflags, s)) && object)
> > >               memset(object, 0, s->object_size);
> > > 
> > > +     kmsan_slab_alloc(s, object, gfpflags);
> > >       slab_post_alloc_hook(s, gfpflags, 1, &object);
> > > 
> > >       return object;
> > > @@ -2797,6 +2818,7 @@ void *kmem_cache_alloc_trace(struct kmem_cache *s, gfp_t gfpflags, size_t size)
> > >       void *ret = slab_alloc(s, gfpflags, _RET_IP_);
> > >       trace_kmalloc(_RET_IP_, ret, size, s->size, gfpflags);
> > >       ret = kasan_kmalloc(s, ret, size, gfpflags);
> > > +
> > >       return ret;
> > >  }
> > >  EXPORT_SYMBOL(kmem_cache_alloc_trace);
> > > @@ -2809,7 +2831,6 @@ void *kmem_cache_alloc_node(struct kmem_cache *s, gfp_t gfpflags, int node)
> > > 
> > >       trace_kmem_cache_alloc_node(_RET_IP_, ret,
> > >                                   s->object_size, s->size, gfpflags, node);
> > > -
> > >       return ret;
> > >  }
> > >  EXPORT_SYMBOL(kmem_cache_alloc_node);
> > > @@ -2825,6 +2846,7 @@ void *kmem_cache_alloc_node_trace(struct kmem_cache *s,
> > >                          size, s->size, gfpflags, node);
> > > 
> > >       ret = kasan_kmalloc(s, ret, size, gfpflags);
> > > +
> > >       return ret;
> > >  }
> > >  EXPORT_SYMBOL(kmem_cache_alloc_node_trace);
> > > @@ -3150,7 +3172,7 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
> > >                         void **p)
> > >  {
> > >       struct kmem_cache_cpu *c;
> > > -     int i;
> > > +     int i, j;
> > > 
> > >       /* memcg and kmem_cache debug support */
> > >       s = slab_pre_alloc_hook(s, flags);
> > > @@ -3188,11 +3210,11 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
> > > 
> > >       /* Clear memory outside IRQ disabled fastpath loop */
> > >       if (unlikely(slab_want_init_on_alloc(flags, s))) {
> > > -             int j;
> > > -
> > >               for (j = 0; j < i; j++)
> > >                       memset(p[j], 0, s->object_size);
> > >       }
> > > +     for (j = 0; j < i; j++)
> > > +             kmsan_slab_alloc(s, p[j], flags);
> > > 
> > >       /* memcg and kmem_cache debug support */
> > >       slab_post_alloc_hook(s, flags, size, p);
> > > @@ -3793,6 +3815,7 @@ static int __init setup_slub_min_objects(char *str)
> > > 
> > >  __setup("slub_min_objects=", setup_slub_min_objects);
> > > 
> > > +__no_sanitize_memory
> > >  void *__kmalloc(size_t size, gfp_t flags)
> > >  {
> > >       struct kmem_cache *s;
> > > @@ -5698,6 +5721,7 @@ static char *create_unique_id(struct kmem_cache *s)
> > >       p += sprintf(p, "%07u", s->size);
> > > 
> > >       BUG_ON(p > name + ID_STR_LENGTH - 1);
> > > +     kmsan_unpoison_shadow(name, p - name);
> > >       return name;
> > >  }
> > > 
> > > @@ -5847,6 +5871,7 @@ static int sysfs_slab_alias(struct kmem_cache *s, const char *name)
> > >       al->name = name;
> > >       al->next = alias_list;
> > >       alias_list = al;
> > > +     kmsan_unpoison_shadow(al, sizeof(struct saved_alias));
> > >       return 0;
> > >  }
> > > 
> 
> 
>

Alexander Potapenko Oct. 18, 2019, 1:55 p.m. UTC | #4

On Fri, Oct 18, 2019 at 3:42 PM Qian Cai <cai@lca.pw> wrote:
>
> On Fri, 2019-10-18 at 15:33 +0200, Alexander Potapenko wrote:
> > On Fri, Oct 18, 2019 at 3:22 PM Qian Cai <cai@lca.pw> wrote:
> > >
> > > On Fri, 2019-10-18 at 11:42 +0200, glider@google.com wrote:
> > > > In order to report uninitialized memory coming from heap allocations
> > > > KMSAN has to poison them unless they're created with __GFP_ZERO.
> > > >
> > > > It's handy that we need KMSAN hooks in the places where
> > > > init_on_alloc/init_on_free initialization is performed.
> > >
> > > Well, there is SLUB debug which has red zoning and poisoning checks. What's
> > > value of this patch?
> >
> > Sorry, are you talking about the whole patchset or just this patch?
>
> Just this patch.
>
> > Note that SLUB debug is unable to detect uninitialized values with
> > bit-to-bit precision, neither have I heard of anyone using it for
> > detecting uses of uninitialized memory in the kernel at all.
> > The purpose of SLUB debug is to detect corruptions of freed memory.
>
> The point is if developers have SLUB debug enabled, all the free objects will be
> poisoned, so what's the point of checking uninitialized memory there?
You are right, SLUB debug has to be handled separately. If I'm
understanding correctly, right now KMSAN poisons freed memory before
SLUB debug wipes it, therefore the memory will count as initialized.
On the other hand, newly allocated memory is still marked as
uninitialized, so a lot of bugs still remain detectable.
TBH, I haven't tested KMSAN with SLUB debug good enough. Note that
it's anyway a separate build that requires Clang, so right now it
doesn't make much sense to combine KMSAN and SLUB debug together.
> > > >
> > > > Signed-off-by: Alexander Potapenko <glider@google.com>
> > > > To: Alexander Potapenko <glider@google.com>
> > > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > > Cc: Vegard Nossum <vegard.nossum@oracle.com>
> > > > Cc: Dmitry Vyukov <dvyukov@google.com>
> > > > Cc: linux-mm@kvack.org
> > > > ---
> > > >
> > > > Change-Id: I51103b7981d3aabed747d0c85cbdc85568665871
> > > > ---
> > > >  mm/slub.c | 37 +++++++++++++++++++++++++++++++------
> > > >  1 file changed, 31 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/mm/slub.c b/mm/slub.c
> > > > index 3d63ae320d31..3d6d4c63446e 100644
> > > > --- a/mm/slub.c
> > > > +++ b/mm/slub.c
> > > > @@ -21,6 +21,8 @@
> > > >  #include <linux/proc_fs.h>
> > > >  #include <linux/seq_file.h>
> > > >  #include <linux/kasan.h>
> > > > +#include <linux/kmsan.h>
> > > > +#include <linux/kmsan-checks.h> /* KMSAN_INIT_VALUE */
> > > >  #include <linux/cpu.h>
> > > >  #include <linux/cpuset.h>
> > > >  #include <linux/mempolicy.h>
> > > > @@ -285,17 +287,27 @@ static void prefetch_freepointer(const struct kmem_cache *s, void *object)
> > > >       prefetch(object + s->offset);
> > > >  }
> > > >
> > > > +/*
> > > > + * When running under KMSAN, get_freepointer_safe() may return an uninitialized
> > > > + * pointer value in the case the current thread loses the race for the next
> > > > + * memory chunk in the freelist. In that case this_cpu_cmpxchg_double() in
> > > > + * slab_alloc_node() will fail, so the uninitialized value won't be used, but
> > > > + * KMSAN will still check all arguments of cmpxchg because of imperfect
> > > > + * handling of inline assembly.
> > > > + * To work around this problem, use KMSAN_INIT_VALUE() to force initialize the
> > > > + * return value of get_freepointer_safe().
> > > > + */
> > > >  static inline void *get_freepointer_safe(struct kmem_cache *s, void *object)
> > > >  {
> > > >       unsigned long freepointer_addr;
> > > >       void *p;
> > > >
> > > >       if (!debug_pagealloc_enabled())
> > > > -             return get_freepointer(s, object);
> > > > +             return KMSAN_INIT_VALUE(get_freepointer(s, object));
> > > >
> > > >       freepointer_addr = (unsigned long)object + s->offset;
> > > >       probe_kernel_read(&p, (void **)freepointer_addr, sizeof(p));
> > > > -     return freelist_ptr(s, p, freepointer_addr);
> > > > +     return KMSAN_INIT_VALUE(freelist_ptr(s, p, freepointer_addr));
> > > >  }
> > > >
> > > >  static inline void set_freepointer(struct kmem_cache *s, void *object, void *fp)
> > > > @@ -1390,6 +1402,7 @@ static inline void *kmalloc_large_node_hook(void *ptr, size_t size, gfp_t flags)
> > > >       ptr = kasan_kmalloc_large(ptr, size, flags);
> > > >       /* As ptr might get tagged, call kmemleak hook after KASAN. */
> > > >       kmemleak_alloc(ptr, size, 1, flags);
> > > > +     kmsan_kmalloc_large(ptr, size, flags);
> > > >       return ptr;
> > > >  }
> > > >
> > > > @@ -1397,6 +1410,7 @@ static __always_inline void kfree_hook(void *x)
> > > >  {
> > > >       kmemleak_free(x);
> > > >       kasan_kfree_large(x, _RET_IP_);
> > > > +     kmsan_kfree_large(x);
> > > >  }
> > > >
> > > >  static __always_inline bool slab_free_hook(struct kmem_cache *s, void *x)
> > > > @@ -1453,6 +1467,12 @@ static inline bool slab_free_freelist_hook(struct kmem_cache *s,
> > > >               } while (object != old_tail);
> > > >       }
> > > >
> > > > +     do {
> > > > +             object = next;
> > > > +             next = get_freepointer(s, object);
> > > > +             kmsan_slab_free(s, object);
> > > > +     } while (object != old_tail);
> > > > +
> > > >  /*
> > > >   * Compiler cannot detect this function can be removed if slab_free_hook()
> > > >   * evaluates to nothing.  Thus, catch all relevant config debug options here.
> > > > @@ -2769,6 +2789,7 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s,
> > > >       if (unlikely(slab_want_init_on_alloc(gfpflags, s)) && object)
> > > >               memset(object, 0, s->object_size);
> > > >
> > > > +     kmsan_slab_alloc(s, object, gfpflags);
> > > >       slab_post_alloc_hook(s, gfpflags, 1, &object);
> > > >
> > > >       return object;
> > > > @@ -2797,6 +2818,7 @@ void *kmem_cache_alloc_trace(struct kmem_cache *s, gfp_t gfpflags, size_t size)
> > > >       void *ret = slab_alloc(s, gfpflags, _RET_IP_);
> > > >       trace_kmalloc(_RET_IP_, ret, size, s->size, gfpflags);
> > > >       ret = kasan_kmalloc(s, ret, size, gfpflags);
> > > > +
> > > >       return ret;
> > > >  }
> > > >  EXPORT_SYMBOL(kmem_cache_alloc_trace);
> > > > @@ -2809,7 +2831,6 @@ void *kmem_cache_alloc_node(struct kmem_cache *s, gfp_t gfpflags, int node)
> > > >
> > > >       trace_kmem_cache_alloc_node(_RET_IP_, ret,
> > > >                                   s->object_size, s->size, gfpflags, node);
> > > > -
> > > >       return ret;
> > > >  }
> > > >  EXPORT_SYMBOL(kmem_cache_alloc_node);
> > > > @@ -2825,6 +2846,7 @@ void *kmem_cache_alloc_node_trace(struct kmem_cache *s,
> > > >                          size, s->size, gfpflags, node);
> > > >
> > > >       ret = kasan_kmalloc(s, ret, size, gfpflags);
> > > > +
> > > >       return ret;
> > > >  }
> > > >  EXPORT_SYMBOL(kmem_cache_alloc_node_trace);
> > > > @@ -3150,7 +3172,7 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
> > > >                         void **p)
> > > >  {
> > > >       struct kmem_cache_cpu *c;
> > > > -     int i;
> > > > +     int i, j;
> > > >
> > > >       /* memcg and kmem_cache debug support */
> > > >       s = slab_pre_alloc_hook(s, flags);
> > > > @@ -3188,11 +3210,11 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
> > > >
> > > >       /* Clear memory outside IRQ disabled fastpath loop */
> > > >       if (unlikely(slab_want_init_on_alloc(flags, s))) {
> > > > -             int j;
> > > > -
> > > >               for (j = 0; j < i; j++)
> > > >                       memset(p[j], 0, s->object_size);
> > > >       }
> > > > +     for (j = 0; j < i; j++)
> > > > +             kmsan_slab_alloc(s, p[j], flags);
> > > >
> > > >       /* memcg and kmem_cache debug support */
> > > >       slab_post_alloc_hook(s, flags, size, p);
> > > > @@ -3793,6 +3815,7 @@ static int __init setup_slub_min_objects(char *str)
> > > >
> > > >  __setup("slub_min_objects=", setup_slub_min_objects);
> > > >
> > > > +__no_sanitize_memory
> > > >  void *__kmalloc(size_t size, gfp_t flags)
> > > >  {
> > > >       struct kmem_cache *s;
> > > > @@ -5698,6 +5721,7 @@ static char *create_unique_id(struct kmem_cache *s)
> > > >       p += sprintf(p, "%07u", s->size);
> > > >
> > > >       BUG_ON(p > name + ID_STR_LENGTH - 1);
> > > > +     kmsan_unpoison_shadow(name, p - name);
> > > >       return name;
> > > >  }
> > > >
> > > > @@ -5847,6 +5871,7 @@ static int sysfs_slab_alias(struct kmem_cache *s, const char *name)
> > > >       al->name = name;
> > > >       al->next = alias_list;
> > > >       alias_list = al;
> > > > +     kmsan_unpoison_shadow(al, sizeof(struct saved_alias));
> > > >       return 0;
> > > >  }
> > > >
> >
> >
> >

Qian Cai Oct. 18, 2019, 2:42 p.m. UTC | #5

On Fri, 2019-10-18 at 15:55 +0200, Alexander Potapenko wrote:
> On Fri, Oct 18, 2019 at 3:42 PM Qian Cai <cai@lca.pw> wrote:
> > 
> > On Fri, 2019-10-18 at 15:33 +0200, Alexander Potapenko wrote:
> > > On Fri, Oct 18, 2019 at 3:22 PM Qian Cai <cai@lca.pw> wrote:
> > > > 
> > > > On Fri, 2019-10-18 at 11:42 +0200, glider@google.com wrote:
> > > > > In order to report uninitialized memory coming from heap allocations
> > > > > KMSAN has to poison them unless they're created with __GFP_ZERO.
> > > > > 
> > > > > It's handy that we need KMSAN hooks in the places where
> > > > > init_on_alloc/init_on_free initialization is performed.
> > > > 
> > > > Well, there is SLUB debug which has red zoning and poisoning checks. What's
> > > > value of this patch?
> > > 
> > > Sorry, are you talking about the whole patchset or just this patch?
> > 
> > Just this patch.
> > 
> > > Note that SLUB debug is unable to detect uninitialized values with
> > > bit-to-bit precision, neither have I heard of anyone using it for
> > > detecting uses of uninitialized memory in the kernel at all.
> > > The purpose of SLUB debug is to detect corruptions of freed memory.
> > 
> > The point is if developers have SLUB debug enabled, all the free objects will be
> > poisoned, so what's the point of checking uninitialized memory there?
> 
> You are right, SLUB debug has to be handled separately. If I'm
> understanding correctly, right now KMSAN poisons freed memory before
> SLUB debug wipes it, therefore the memory will count as initialized.
> On the other hand, newly allocated memory is still marked as
> uninitialized, so a lot of bugs still remain detectable.

Yes, but newly allocated slub objects will be poisoned as well.

> TBH, I haven't tested KMSAN with SLUB debug good enough. Note that
> it's anyway a separate build that requires Clang, so right now it
> doesn't make much sense to combine KMSAN and SLUB debug together.

Can't you just build a debug kernel with SLUB debug enabled but dropping this
patch? If there is an uninitialized memory here leading to data corruption, SLUB
debug should be detected as well as this patch. If not, it needs to understand
why.

> > > > > 
> > > > > Signed-off-by: Alexander Potapenko <glider@google.com>
> > > > > To: Alexander Potapenko <glider@google.com>
> > > > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > > > Cc: Vegard Nossum <vegard.nossum@oracle.com>
> > > > > Cc: Dmitry Vyukov <dvyukov@google.com>
> > > > > Cc: linux-mm@kvack.org
> > > > > ---
> > > > > 
> > > > > Change-Id: I51103b7981d3aabed747d0c85cbdc85568665871
> > > > > ---
> > > > >  mm/slub.c | 37 +++++++++++++++++++++++++++++++------
> > > > >  1 file changed, 31 insertions(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/mm/slub.c b/mm/slub.c
> > > > > index 3d63ae320d31..3d6d4c63446e 100644
> > > > > --- a/mm/slub.c
> > > > > +++ b/mm/slub.c
> > > > > @@ -21,6 +21,8 @@
> > > > >  #include <linux/proc_fs.h>
> > > > >  #include <linux/seq_file.h>
> > > > >  #include <linux/kasan.h>
> > > > > +#include <linux/kmsan.h>
> > > > > +#include <linux/kmsan-checks.h> /* KMSAN_INIT_VALUE */
> > > > >  #include <linux/cpu.h>
> > > > >  #include <linux/cpuset.h>
> > > > >  #include <linux/mempolicy.h>
> > > > > @@ -285,17 +287,27 @@ static void prefetch_freepointer(const struct kmem_cache *s, void *object)
> > > > >       prefetch(object + s->offset);
> > > > >  }
> > > > > 
> > > > > +/*
> > > > > + * When running under KMSAN, get_freepointer_safe() may return an uninitialized
> > > > > + * pointer value in the case the current thread loses the race for the next
> > > > > + * memory chunk in the freelist. In that case this_cpu_cmpxchg_double() in
> > > > > + * slab_alloc_node() will fail, so the uninitialized value won't be used, but
> > > > > + * KMSAN will still check all arguments of cmpxchg because of imperfect
> > > > > + * handling of inline assembly.
> > > > > + * To work around this problem, use KMSAN_INIT_VALUE() to force initialize the
> > > > > + * return value of get_freepointer_safe().
> > > > > + */
> > > > >  static inline void *get_freepointer_safe(struct kmem_cache *s, void *object)
> > > > >  {
> > > > >       unsigned long freepointer_addr;
> > > > >       void *p;
> > > > > 
> > > > >       if (!debug_pagealloc_enabled())
> > > > > -             return get_freepointer(s, object);
> > > > > +             return KMSAN_INIT_VALUE(get_freepointer(s, object));
> > > > > 
> > > > >       freepointer_addr = (unsigned long)object + s->offset;
> > > > >       probe_kernel_read(&p, (void **)freepointer_addr, sizeof(p));
> > > > > -     return freelist_ptr(s, p, freepointer_addr);
> > > > > +     return KMSAN_INIT_VALUE(freelist_ptr(s, p, freepointer_addr));
> > > > >  }
> > > > > 
> > > > >  static inline void set_freepointer(struct kmem_cache *s, void *object, void *fp)
> > > > > @@ -1390,6 +1402,7 @@ static inline void *kmalloc_large_node_hook(void *ptr, size_t size, gfp_t flags)
> > > > >       ptr = kasan_kmalloc_large(ptr, size, flags);
> > > > >       /* As ptr might get tagged, call kmemleak hook after KASAN. */
> > > > >       kmemleak_alloc(ptr, size, 1, flags);
> > > > > +     kmsan_kmalloc_large(ptr, size, flags);
> > > > >       return ptr;
> > > > >  }
> > > > > 
> > > > > @@ -1397,6 +1410,7 @@ static __always_inline void kfree_hook(void *x)
> > > > >  {
> > > > >       kmemleak_free(x);
> > > > >       kasan_kfree_large(x, _RET_IP_);
> > > > > +     kmsan_kfree_large(x);
> > > > >  }
> > > > > 
> > > > >  static __always_inline bool slab_free_hook(struct kmem_cache *s, void *x)
> > > > > @@ -1453,6 +1467,12 @@ static inline bool slab_free_freelist_hook(struct kmem_cache *s,
> > > > >               } while (object != old_tail);
> > > > >       }
> > > > > 
> > > > > +     do {
> > > > > +             object = next;
> > > > > +             next = get_freepointer(s, object);
> > > > > +             kmsan_slab_free(s, object);
> > > > > +     } while (object != old_tail);
> > > > > +
> > > > >  /*
> > > > >   * Compiler cannot detect this function can be removed if slab_free_hook()
> > > > >   * evaluates to nothing.  Thus, catch all relevant config debug options here.
> > > > > @@ -2769,6 +2789,7 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s,
> > > > >       if (unlikely(slab_want_init_on_alloc(gfpflags, s)) && object)
> > > > >               memset(object, 0, s->object_size);
> > > > > 
> > > > > +     kmsan_slab_alloc(s, object, gfpflags);
> > > > >       slab_post_alloc_hook(s, gfpflags, 1, &object);
> > > > > 
> > > > >       return object;
> > > > > @@ -2797,6 +2818,7 @@ void *kmem_cache_alloc_trace(struct kmem_cache *s, gfp_t gfpflags, size_t size)
> > > > >       void *ret = slab_alloc(s, gfpflags, _RET_IP_);
> > > > >       trace_kmalloc(_RET_IP_, ret, size, s->size, gfpflags);
> > > > >       ret = kasan_kmalloc(s, ret, size, gfpflags);
> > > > > +
> > > > >       return ret;
> > > > >  }
> > > > >  EXPORT_SYMBOL(kmem_cache_alloc_trace);
> > > > > @@ -2809,7 +2831,6 @@ void *kmem_cache_alloc_node(struct kmem_cache *s, gfp_t gfpflags, int node)
> > > > > 
> > > > >       trace_kmem_cache_alloc_node(_RET_IP_, ret,
> > > > >                                   s->object_size, s->size, gfpflags, node);
> > > > > -
> > > > >       return ret;
> > > > >  }
> > > > >  EXPORT_SYMBOL(kmem_cache_alloc_node);
> > > > > @@ -2825,6 +2846,7 @@ void *kmem_cache_alloc_node_trace(struct kmem_cache *s,
> > > > >                          size, s->size, gfpflags, node);
> > > > > 
> > > > >       ret = kasan_kmalloc(s, ret, size, gfpflags);
> > > > > +
> > > > >       return ret;
> > > > >  }
> > > > >  EXPORT_SYMBOL(kmem_cache_alloc_node_trace);
> > > > > @@ -3150,7 +3172,7 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
> > > > >                         void **p)
> > > > >  {
> > > > >       struct kmem_cache_cpu *c;
> > > > > -     int i;
> > > > > +     int i, j;
> > > > > 
> > > > >       /* memcg and kmem_cache debug support */
> > > > >       s = slab_pre_alloc_hook(s, flags);
> > > > > @@ -3188,11 +3210,11 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
> > > > > 
> > > > >       /* Clear memory outside IRQ disabled fastpath loop */
> > > > >       if (unlikely(slab_want_init_on_alloc(flags, s))) {
> > > > > -             int j;
> > > > > -
> > > > >               for (j = 0; j < i; j++)
> > > > >                       memset(p[j], 0, s->object_size);
> > > > >       }
> > > > > +     for (j = 0; j < i; j++)
> > > > > +             kmsan_slab_alloc(s, p[j], flags);
> > > > > 
> > > > >       /* memcg and kmem_cache debug support */
> > > > >       slab_post_alloc_hook(s, flags, size, p);
> > > > > @@ -3793,6 +3815,7 @@ static int __init setup_slub_min_objects(char *str)
> > > > > 
> > > > >  __setup("slub_min_objects=", setup_slub_min_objects);
> > > > > 
> > > > > +__no_sanitize_memory
> > > > >  void *__kmalloc(size_t size, gfp_t flags)
> > > > >  {
> > > > >       struct kmem_cache *s;
> > > > > @@ -5698,6 +5721,7 @@ static char *create_unique_id(struct kmem_cache *s)
> > > > >       p += sprintf(p, "%07u", s->size);
> > > > > 
> > > > >       BUG_ON(p > name + ID_STR_LENGTH - 1);
> > > > > +     kmsan_unpoison_shadow(name, p - name);
> > > > >       return name;
> > > > >  }
> > > > > 
> > > > > @@ -5847,6 +5871,7 @@ static int sysfs_slab_alias(struct kmem_cache *s, const char *name)
> > > > >       al->name = name;
> > > > >       al->next = alias_list;
> > > > >       alias_list = al;
> > > > > +     kmsan_unpoison_shadow(al, sizeof(struct saved_alias));
> > > > >       return 0;
> > > > >  }
> > > > > 
> > > 
> > > 
> > > 
> 
> 
>

Alexander Potapenko Oct. 18, 2019, 2:54 p.m. UTC | #6

On Fri, Oct 18, 2019 at 4:42 PM Qian Cai <cai@lca.pw> wrote:
>
> On Fri, 2019-10-18 at 15:55 +0200, Alexander Potapenko wrote:
> > On Fri, Oct 18, 2019 at 3:42 PM Qian Cai <cai@lca.pw> wrote:
> > >
> > > On Fri, 2019-10-18 at 15:33 +0200, Alexander Potapenko wrote:
> > > > On Fri, Oct 18, 2019 at 3:22 PM Qian Cai <cai@lca.pw> wrote:
> > > > >
> > > > > On Fri, 2019-10-18 at 11:42 +0200, glider@google.com wrote:
> > > > > > In order to report uninitialized memory coming from heap allocations
> > > > > > KMSAN has to poison them unless they're created with __GFP_ZERO.
> > > > > >
> > > > > > It's handy that we need KMSAN hooks in the places where
> > > > > > init_on_alloc/init_on_free initialization is performed.
> > > > >
> > > > > Well, there is SLUB debug which has red zoning and poisoning checks. What's
> > > > > value of this patch?
> > > >
> > > > Sorry, are you talking about the whole patchset or just this patch?
> > >
> > > Just this patch.
> > >
> > > > Note that SLUB debug is unable to detect uninitialized values with
> > > > bit-to-bit precision, neither have I heard of anyone using it for
> > > > detecting uses of uninitialized memory in the kernel at all.
> > > > The purpose of SLUB debug is to detect corruptions of freed memory.
> > >
> > > The point is if developers have SLUB debug enabled, all the free objects will be
> > > poisoned, so what's the point of checking uninitialized memory there?
> >
> > You are right, SLUB debug has to be handled separately. If I'm
> > understanding correctly, right now KMSAN poisons freed memory before
> > SLUB debug wipes it, therefore the memory will count as initialized.
> > On the other hand, newly allocated memory is still marked as
> > uninitialized, so a lot of bugs still remain detectable.
>
> Yes, but newly allocated slub objects will be poisoned as well.
As far as I can tell, KMSAN hook marking newly allocated objects as
uninitialized is called after slub poisoning.
Therefore these allocations will be treated by KMSAN as uninitialized.
> > TBH, I haven't tested KMSAN with SLUB debug good enough. Note that
> > it's anyway a separate build that requires Clang, so right now it
> > doesn't make much sense to combine KMSAN and SLUB debug together.
>
> Can't you just build a debug kernel with SLUB debug enabled but dropping this
> patch? If there is an uninitialized memory here leading to data corruption, SLUB
> debug should be detected as well as this patch. If not, it needs to understand
> why.
Sorry, there might be some misunderstanding here.
KMSAN keeps the state of heap objects separately by keeping exactly
the same amount of initialized/uninitialized bits for every
allocation.
A call to kmsan_slab_alloc()/kmsan_slab_free() will mark an allocation
as uninitialized for KMSAN. Not doing so will result in false reports.
A call to memset(object, POISON_FREE, object_size) performed by SLAB
debug will actually mark this allocation as initialized from KMSAN
point of view, because we're memsetting a range with initialized data.
Note that use of uninitialized data doesn't necessarily lead to
immediate data corruption, so there might be nothing to detect for
SLUB debug.
>
> > > > > >
> > > > > > Signed-off-by: Alexander Potapenko <glider@google.com>
> > > > > > To: Alexander Potapenko <glider@google.com>
> > > > > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > > > > Cc: Vegard Nossum <vegard.nossum@oracle.com>
> > > > > > Cc: Dmitry Vyukov <dvyukov@google.com>
> > > > > > Cc: linux-mm@kvack.org
> > > > > > ---
> > > > > >
> > > > > > Change-Id: I51103b7981d3aabed747d0c85cbdc85568665871
> > > > > > ---
> > > > > >  mm/slub.c | 37 +++++++++++++++++++++++++++++++------
> > > > > >  1 file changed, 31 insertions(+), 6 deletions(-)
> > > > > >
> > > > > > diff --git a/mm/slub.c b/mm/slub.c
> > > > > > index 3d63ae320d31..3d6d4c63446e 100644
> > > > > > --- a/mm/slub.c
> > > > > > +++ b/mm/slub.c
> > > > > > @@ -21,6 +21,8 @@
> > > > > >  #include <linux/proc_fs.h>
> > > > > >  #include <linux/seq_file.h>
> > > > > >  #include <linux/kasan.h>
> > > > > > +#include <linux/kmsan.h>
> > > > > > +#include <linux/kmsan-checks.h> /* KMSAN_INIT_VALUE */
> > > > > >  #include <linux/cpu.h>
> > > > > >  #include <linux/cpuset.h>
> > > > > >  #include <linux/mempolicy.h>
> > > > > > @@ -285,17 +287,27 @@ static void prefetch_freepointer(const struct kmem_cache *s, void *object)
> > > > > >       prefetch(object + s->offset);
> > > > > >  }
> > > > > >
> > > > > > +/*
> > > > > > + * When running under KMSAN, get_freepointer_safe() may return an uninitialized
> > > > > > + * pointer value in the case the current thread loses the race for the next
> > > > > > + * memory chunk in the freelist. In that case this_cpu_cmpxchg_double() in
> > > > > > + * slab_alloc_node() will fail, so the uninitialized value won't be used, but
> > > > > > + * KMSAN will still check all arguments of cmpxchg because of imperfect
> > > > > > + * handling of inline assembly.
> > > > > > + * To work around this problem, use KMSAN_INIT_VALUE() to force initialize the
> > > > > > + * return value of get_freepointer_safe().
> > > > > > + */
> > > > > >  static inline void *get_freepointer_safe(struct kmem_cache *s, void *object)
> > > > > >  {
> > > > > >       unsigned long freepointer_addr;
> > > > > >       void *p;
> > > > > >
> > > > > >       if (!debug_pagealloc_enabled())
> > > > > > -             return get_freepointer(s, object);
> > > > > > +             return KMSAN_INIT_VALUE(get_freepointer(s, object));
> > > > > >
> > > > > >       freepointer_addr = (unsigned long)object + s->offset;
> > > > > >       probe_kernel_read(&p, (void **)freepointer_addr, sizeof(p));
> > > > > > -     return freelist_ptr(s, p, freepointer_addr);
> > > > > > +     return KMSAN_INIT_VALUE(freelist_ptr(s, p, freepointer_addr));
> > > > > >  }
> > > > > >
> > > > > >  static inline void set_freepointer(struct kmem_cache *s, void *object, void *fp)
> > > > > > @@ -1390,6 +1402,7 @@ static inline void *kmalloc_large_node_hook(void *ptr, size_t size, gfp_t flags)
> > > > > >       ptr = kasan_kmalloc_large(ptr, size, flags);
> > > > > >       /* As ptr might get tagged, call kmemleak hook after KASAN. */
> > > > > >       kmemleak_alloc(ptr, size, 1, flags);
> > > > > > +     kmsan_kmalloc_large(ptr, size, flags);
> > > > > >       return ptr;
> > > > > >  }
> > > > > >
> > > > > > @@ -1397,6 +1410,7 @@ static __always_inline void kfree_hook(void *x)
> > > > > >  {
> > > > > >       kmemleak_free(x);
> > > > > >       kasan_kfree_large(x, _RET_IP_);
> > > > > > +     kmsan_kfree_large(x);
> > > > > >  }
> > > > > >
> > > > > >  static __always_inline bool slab_free_hook(struct kmem_cache *s, void *x)
> > > > > > @@ -1453,6 +1467,12 @@ static inline bool slab_free_freelist_hook(struct kmem_cache *s,
> > > > > >               } while (object != old_tail);
> > > > > >       }
> > > > > >
> > > > > > +     do {
> > > > > > +             object = next;
> > > > > > +             next = get_freepointer(s, object);
> > > > > > +             kmsan_slab_free(s, object);
> > > > > > +     } while (object != old_tail);
> > > > > > +
> > > > > >  /*
> > > > > >   * Compiler cannot detect this function can be removed if slab_free_hook()
> > > > > >   * evaluates to nothing.  Thus, catch all relevant config debug options here.
> > > > > > @@ -2769,6 +2789,7 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s,
> > > > > >       if (unlikely(slab_want_init_on_alloc(gfpflags, s)) && object)
> > > > > >               memset(object, 0, s->object_size);
> > > > > >
> > > > > > +     kmsan_slab_alloc(s, object, gfpflags);
> > > > > >       slab_post_alloc_hook(s, gfpflags, 1, &object);
> > > > > >
> > > > > >       return object;
> > > > > > @@ -2797,6 +2818,7 @@ void *kmem_cache_alloc_trace(struct kmem_cache *s, gfp_t gfpflags, size_t size)
> > > > > >       void *ret = slab_alloc(s, gfpflags, _RET_IP_);
> > > > > >       trace_kmalloc(_RET_IP_, ret, size, s->size, gfpflags);
> > > > > >       ret = kasan_kmalloc(s, ret, size, gfpflags);
> > > > > > +
> > > > > >       return ret;
> > > > > >  }
> > > > > >  EXPORT_SYMBOL(kmem_cache_alloc_trace);
> > > > > > @@ -2809,7 +2831,6 @@ void *kmem_cache_alloc_node(struct kmem_cache *s, gfp_t gfpflags, int node)
> > > > > >
> > > > > >       trace_kmem_cache_alloc_node(_RET_IP_, ret,
> > > > > >                                   s->object_size, s->size, gfpflags, node);
> > > > > > -
> > > > > >       return ret;
> > > > > >  }
> > > > > >  EXPORT_SYMBOL(kmem_cache_alloc_node);
> > > > > > @@ -2825,6 +2846,7 @@ void *kmem_cache_alloc_node_trace(struct kmem_cache *s,
> > > > > >                          size, s->size, gfpflags, node);
> > > > > >
> > > > > >       ret = kasan_kmalloc(s, ret, size, gfpflags);
> > > > > > +
> > > > > >       return ret;
> > > > > >  }
> > > > > >  EXPORT_SYMBOL(kmem_cache_alloc_node_trace);
> > > > > > @@ -3150,7 +3172,7 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
> > > > > >                         void **p)
> > > > > >  {
> > > > > >       struct kmem_cache_cpu *c;
> > > > > > -     int i;
> > > > > > +     int i, j;
> > > > > >
> > > > > >       /* memcg and kmem_cache debug support */
> > > > > >       s = slab_pre_alloc_hook(s, flags);
> > > > > > @@ -3188,11 +3210,11 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
> > > > > >
> > > > > >       /* Clear memory outside IRQ disabled fastpath loop */
> > > > > >       if (unlikely(slab_want_init_on_alloc(flags, s))) {
> > > > > > -             int j;
> > > > > > -
> > > > > >               for (j = 0; j < i; j++)
> > > > > >                       memset(p[j], 0, s->object_size);
> > > > > >       }
> > > > > > +     for (j = 0; j < i; j++)
> > > > > > +             kmsan_slab_alloc(s, p[j], flags);
> > > > > >
> > > > > >       /* memcg and kmem_cache debug support */
> > > > > >       slab_post_alloc_hook(s, flags, size, p);
> > > > > > @@ -3793,6 +3815,7 @@ static int __init setup_slub_min_objects(char *str)
> > > > > >
> > > > > >  __setup("slub_min_objects=", setup_slub_min_objects);
> > > > > >
> > > > > > +__no_sanitize_memory
> > > > > >  void *__kmalloc(size_t size, gfp_t flags)
> > > > > >  {
> > > > > >       struct kmem_cache *s;
> > > > > > @@ -5698,6 +5721,7 @@ static char *create_unique_id(struct kmem_cache *s)
> > > > > >       p += sprintf(p, "%07u", s->size);
> > > > > >
> > > > > >       BUG_ON(p > name + ID_STR_LENGTH - 1);
> > > > > > +     kmsan_unpoison_shadow(name, p - name);
> > > > > >       return name;
> > > > > >  }
> > > > > >
> > > > > > @@ -5847,6 +5871,7 @@ static int sysfs_slab_alias(struct kmem_cache *s, const char *name)
> > > > > >       al->name = name;
> > > > > >       al->next = alias_list;
> > > > > >       alias_list = al;
> > > > > > +     kmsan_unpoison_shadow(al, sizeof(struct saved_alias));
> > > > > >       return 0;
> > > > > >  }
> > > > > >
> > > >
> > > >
> > > >
> >
> >
> >

Qian Cai Oct. 18, 2019, 3:13 p.m. UTC | #7

On Fri, 2019-10-18 at 16:54 +0200, Alexander Potapenko wrote:
> On Fri, Oct 18, 2019 at 4:42 PM Qian Cai <cai@lca.pw> wrote:
> > 
> > On Fri, 2019-10-18 at 15:55 +0200, Alexander Potapenko wrote:
> > > On Fri, Oct 18, 2019 at 3:42 PM Qian Cai <cai@lca.pw> wrote:
> > > > 
> > > > On Fri, 2019-10-18 at 15:33 +0200, Alexander Potapenko wrote:
> > > > > On Fri, Oct 18, 2019 at 3:22 PM Qian Cai <cai@lca.pw> wrote:
> > > > > > 
> > > > > > On Fri, 2019-10-18 at 11:42 +0200, glider@google.com wrote:
> > > > > > > In order to report uninitialized memory coming from heap allocations
> > > > > > > KMSAN has to poison them unless they're created with __GFP_ZERO.
> > > > > > > 
> > > > > > > It's handy that we need KMSAN hooks in the places where
> > > > > > > init_on_alloc/init_on_free initialization is performed.
> > > > > > 
> > > > > > Well, there is SLUB debug which has red zoning and poisoning checks. What's
> > > > > > value of this patch?
> > > > > 
> > > > > Sorry, are you talking about the whole patchset or just this patch?
> > > > 
> > > > Just this patch.
> > > > 
> > > > > Note that SLUB debug is unable to detect uninitialized values with
> > > > > bit-to-bit precision, neither have I heard of anyone using it for
> > > > > detecting uses of uninitialized memory in the kernel at all.
> > > > > The purpose of SLUB debug is to detect corruptions of freed memory.
> > > > 
> > > > The point is if developers have SLUB debug enabled, all the free objects will be
> > > > poisoned, so what's the point of checking uninitialized memory there?
> > > 
> > > You are right, SLUB debug has to be handled separately. If I'm
> > > understanding correctly, right now KMSAN poisons freed memory before
> > > SLUB debug wipes it, therefore the memory will count as initialized.
> > > On the other hand, newly allocated memory is still marked as
> > > uninitialized, so a lot of bugs still remain detectable.
> > 
> > Yes, but newly allocated slub objects will be poisoned as well.
> 
> As far as I can tell, KMSAN hook marking newly allocated objects as
> uninitialized is called after slub poisoning.
> Therefore these allocations will be treated by KMSAN as uninitialized.
> > > TBH, I haven't tested KMSAN with SLUB debug good enough. Note that
> > > it's anyway a separate build that requires Clang, so right now it
> > > doesn't make much sense to combine KMSAN and SLUB debug together.
> > 
> > Can't you just build a debug kernel with SLUB debug enabled but dropping this
> > patch? If there is an uninitialized memory here leading to data corruption, SLUB
> > debug should be detected as well as this patch. If not, it needs to understand
> > why.
> 
> Sorry, there might be some misunderstanding here.
> KMSAN keeps the state of heap objects separately by keeping exactly
> the same amount of initialized/uninitialized bits for every
> allocation.
> A call to kmsan_slab_alloc()/kmsan_slab_free() will mark an allocation
> as uninitialized for KMSAN. Not doing so will result in false reports.
> A call to memset(object, POISON_FREE, object_size) performed by SLAB
> debug will actually mark this allocation as initialized from KMSAN
> point of view, because we're memsetting a range with initialized data.
> Note that use of uninitialized data doesn't necessarily lead to
> immediate data corruption, so there might be nothing to detect for
> SLUB debug.

Well, SLUB debug would mark any access of uninitialized memory as data
corruption with the help of poisoning and red zoning.

It is true that KMSAN might be doing a bit more than SLUB debug here, but the
question is if it is worth the maintenance burden? Do you have any existing bugs
to show that KMSAN would find by this patch but SLUB debug can't?

> > 
> > > > > > > 
> > > > > > > Signed-off-by: Alexander Potapenko <glider@google.com>
> > > > > > > To: Alexander Potapenko <glider@google.com>
> > > > > > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > > > > > Cc: Vegard Nossum <vegard.nossum@oracle.com>
> > > > > > > Cc: Dmitry Vyukov <dvyukov@google.com>
> > > > > > > Cc: linux-mm@kvack.org
> > > > > > > ---
> > > > > > > 
> > > > > > > Change-Id: I51103b7981d3aabed747d0c85cbdc85568665871
> > > > > > > ---
> > > > > > >  mm/slub.c | 37 +++++++++++++++++++++++++++++++------
> > > > > > >  1 file changed, 31 insertions(+), 6 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/mm/slub.c b/mm/slub.c
> > > > > > > index 3d63ae320d31..3d6d4c63446e 100644
> > > > > > > --- a/mm/slub.c
> > > > > > > +++ b/mm/slub.c
> > > > > > > @@ -21,6 +21,8 @@
> > > > > > >  #include <linux/proc_fs.h>
> > > > > > >  #include <linux/seq_file.h>
> > > > > > >  #include <linux/kasan.h>
> > > > > > > +#include <linux/kmsan.h>
> > > > > > > +#include <linux/kmsan-checks.h> /* KMSAN_INIT_VALUE */
> > > > > > >  #include <linux/cpu.h>
> > > > > > >  #include <linux/cpuset.h>
> > > > > > >  #include <linux/mempolicy.h>
> > > > > > > @@ -285,17 +287,27 @@ static void prefetch_freepointer(const struct kmem_cache *s, void *object)
> > > > > > >       prefetch(object + s->offset);
> > > > > > >  }
> > > > > > > 
> > > > > > > +/*
> > > > > > > + * When running under KMSAN, get_freepointer_safe() may return an uninitialized
> > > > > > > + * pointer value in the case the current thread loses the race for the next
> > > > > > > + * memory chunk in the freelist. In that case this_cpu_cmpxchg_double() in
> > > > > > > + * slab_alloc_node() will fail, so the uninitialized value won't be used, but
> > > > > > > + * KMSAN will still check all arguments of cmpxchg because of imperfect
> > > > > > > + * handling of inline assembly.
> > > > > > > + * To work around this problem, use KMSAN_INIT_VALUE() to force initialize the
> > > > > > > + * return value of get_freepointer_safe().
> > > > > > > + */
> > > > > > >  static inline void *get_freepointer_safe(struct kmem_cache *s, void *object)
> > > > > > >  {
> > > > > > >       unsigned long freepointer_addr;
> > > > > > >       void *p;
> > > > > > > 
> > > > > > >       if (!debug_pagealloc_enabled())
> > > > > > > -             return get_freepointer(s, object);
> > > > > > > +             return KMSAN_INIT_VALUE(get_freepointer(s, object));
> > > > > > > 
> > > > > > >       freepointer_addr = (unsigned long)object + s->offset;
> > > > > > >       probe_kernel_read(&p, (void **)freepointer_addr, sizeof(p));
> > > > > > > -     return freelist_ptr(s, p, freepointer_addr);
> > > > > > > +     return KMSAN_INIT_VALUE(freelist_ptr(s, p, freepointer_addr));
> > > > > > >  }
> > > > > > > 
> > > > > > >  static inline void set_freepointer(struct kmem_cache *s, void *object, void *fp)
> > > > > > > @@ -1390,6 +1402,7 @@ static inline void *kmalloc_large_node_hook(void *ptr, size_t size, gfp_t flags)
> > > > > > >       ptr = kasan_kmalloc_large(ptr, size, flags);
> > > > > > >       /* As ptr might get tagged, call kmemleak hook after KASAN. */
> > > > > > >       kmemleak_alloc(ptr, size, 1, flags);
> > > > > > > +     kmsan_kmalloc_large(ptr, size, flags);
> > > > > > >       return ptr;
> > > > > > >  }
> > > > > > > 
> > > > > > > @@ -1397,6 +1410,7 @@ static __always_inline void kfree_hook(void *x)
> > > > > > >  {
> > > > > > >       kmemleak_free(x);
> > > > > > >       kasan_kfree_large(x, _RET_IP_);
> > > > > > > +     kmsan_kfree_large(x);
> > > > > > >  }
> > > > > > > 
> > > > > > >  static __always_inline bool slab_free_hook(struct kmem_cache *s, void *x)
> > > > > > > @@ -1453,6 +1467,12 @@ static inline bool slab_free_freelist_hook(struct kmem_cache *s,
> > > > > > >               } while (object != old_tail);
> > > > > > >       }
> > > > > > > 
> > > > > > > +     do {
> > > > > > > +             object = next;
> > > > > > > +             next = get_freepointer(s, object);
> > > > > > > +             kmsan_slab_free(s, object);
> > > > > > > +     } while (object != old_tail);
> > > > > > > +
> > > > > > >  /*
> > > > > > >   * Compiler cannot detect this function can be removed if slab_free_hook()
> > > > > > >   * evaluates to nothing.  Thus, catch all relevant config debug options here.
> > > > > > > @@ -2769,6 +2789,7 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s,
> > > > > > >       if (unlikely(slab_want_init_on_alloc(gfpflags, s)) && object)
> > > > > > >               memset(object, 0, s->object_size);
> > > > > > > 
> > > > > > > +     kmsan_slab_alloc(s, object, gfpflags);
> > > > > > >       slab_post_alloc_hook(s, gfpflags, 1, &object);
> > > > > > > 
> > > > > > >       return object;
> > > > > > > @@ -2797,6 +2818,7 @@ void *kmem_cache_alloc_trace(struct kmem_cache *s, gfp_t gfpflags, size_t size)
> > > > > > >       void *ret = slab_alloc(s, gfpflags, _RET_IP_);
> > > > > > >       trace_kmalloc(_RET_IP_, ret, size, s->size, gfpflags);
> > > > > > >       ret = kasan_kmalloc(s, ret, size, gfpflags);
> > > > > > > +
> > > > > > >       return ret;
> > > > > > >  }
> > > > > > >  EXPORT_SYMBOL(kmem_cache_alloc_trace);
> > > > > > > @@ -2809,7 +2831,6 @@ void *kmem_cache_alloc_node(struct kmem_cache *s, gfp_t gfpflags, int node)
> > > > > > > 
> > > > > > >       trace_kmem_cache_alloc_node(_RET_IP_, ret,
> > > > > > >                                   s->object_size, s->size, gfpflags, node);
> > > > > > > -
> > > > > > >       return ret;
> > > > > > >  }
> > > > > > >  EXPORT_SYMBOL(kmem_cache_alloc_node);
> > > > > > > @@ -2825,6 +2846,7 @@ void *kmem_cache_alloc_node_trace(struct kmem_cache *s,
> > > > > > >                          size, s->size, gfpflags, node);
> > > > > > > 
> > > > > > >       ret = kasan_kmalloc(s, ret, size, gfpflags);
> > > > > > > +
> > > > > > >       return ret;
> > > > > > >  }
> > > > > > >  EXPORT_SYMBOL(kmem_cache_alloc_node_trace);
> > > > > > > @@ -3150,7 +3172,7 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
> > > > > > >                         void **p)
> > > > > > >  {
> > > > > > >       struct kmem_cache_cpu *c;
> > > > > > > -     int i;
> > > > > > > +     int i, j;
> > > > > > > 
> > > > > > >       /* memcg and kmem_cache debug support */
> > > > > > >       s = slab_pre_alloc_hook(s, flags);
> > > > > > > @@ -3188,11 +3210,11 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
> > > > > > > 
> > > > > > >       /* Clear memory outside IRQ disabled fastpath loop */
> > > > > > >       if (unlikely(slab_want_init_on_alloc(flags, s))) {
> > > > > > > -             int j;
> > > > > > > -
> > > > > > >               for (j = 0; j < i; j++)
> > > > > > >                       memset(p[j], 0, s->object_size);
> > > > > > >       }
> > > > > > > +     for (j = 0; j < i; j++)
> > > > > > > +             kmsan_slab_alloc(s, p[j], flags);
> > > > > > > 
> > > > > > >       /* memcg and kmem_cache debug support */
> > > > > > >       slab_post_alloc_hook(s, flags, size, p);
> > > > > > > @@ -3793,6 +3815,7 @@ static int __init setup_slub_min_objects(char *str)
> > > > > > > 
> > > > > > >  __setup("slub_min_objects=", setup_slub_min_objects);
> > > > > > > 
> > > > > > > +__no_sanitize_memory
> > > > > > >  void *__kmalloc(size_t size, gfp_t flags)
> > > > > > >  {
> > > > > > >       struct kmem_cache *s;
> > > > > > > @@ -5698,6 +5721,7 @@ static char *create_unique_id(struct kmem_cache *s)
> > > > > > >       p += sprintf(p, "%07u", s->size);
> > > > > > > 
> > > > > > >       BUG_ON(p > name + ID_STR_LENGTH - 1);
> > > > > > > +     kmsan_unpoison_shadow(name, p - name);
> > > > > > >       return name;
> > > > > > >  }
> > > > > > > 
> > > > > > > @@ -5847,6 +5871,7 @@ static int sysfs_slab_alias(struct kmem_cache *s, const char *name)
> > > > > > >       al->name = name;
> > > > > > >       al->next = alias_list;
> > > > > > >       alias_list = al;
> > > > > > > +     kmsan_unpoison_shadow(al, sizeof(struct saved_alias));
> > > > > > >       return 0;
> > > > > > >  }
> > > > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > 
> > > 
> > > 
> 
> 
>

Alexander Potapenko Oct. 18, 2019, 3:30 p.m. UTC | #8

On Fri, Oct 18, 2019 at 5:13 PM Qian Cai <cai@lca.pw> wrote:
>
> On Fri, 2019-10-18 at 16:54 +0200, Alexander Potapenko wrote:
> > On Fri, Oct 18, 2019 at 4:42 PM Qian Cai <cai@lca.pw> wrote:
> > >
> > > On Fri, 2019-10-18 at 15:55 +0200, Alexander Potapenko wrote:
> > > > On Fri, Oct 18, 2019 at 3:42 PM Qian Cai <cai@lca.pw> wrote:
> > > > >
> > > > > On Fri, 2019-10-18 at 15:33 +0200, Alexander Potapenko wrote:
> > > > > > On Fri, Oct 18, 2019 at 3:22 PM Qian Cai <cai@lca.pw> wrote:
> > > > > > >
> > > > > > > On Fri, 2019-10-18 at 11:42 +0200, glider@google.com wrote:
> > > > > > > > In order to report uninitialized memory coming from heap allocations
> > > > > > > > KMSAN has to poison them unless they're created with __GFP_ZERO.
> > > > > > > >
> > > > > > > > It's handy that we need KMSAN hooks in the places where
> > > > > > > > init_on_alloc/init_on_free initialization is performed.
> > > > > > >
> > > > > > > Well, there is SLUB debug which has red zoning and poisoning checks. What's
> > > > > > > value of this patch?
> > > > > >
> > > > > > Sorry, are you talking about the whole patchset or just this patch?
> > > > >
> > > > > Just this patch.
> > > > >
> > > > > > Note that SLUB debug is unable to detect uninitialized values with
> > > > > > bit-to-bit precision, neither have I heard of anyone using it for
> > > > > > detecting uses of uninitialized memory in the kernel at all.
> > > > > > The purpose of SLUB debug is to detect corruptions of freed memory.
> > > > >
> > > > > The point is if developers have SLUB debug enabled, all the free objects will be
> > > > > poisoned, so what's the point of checking uninitialized memory there?
> > > >
> > > > You are right, SLUB debug has to be handled separately. If I'm
> > > > understanding correctly, right now KMSAN poisons freed memory before
> > > > SLUB debug wipes it, therefore the memory will count as initialized.
> > > > On the other hand, newly allocated memory is still marked as
> > > > uninitialized, so a lot of bugs still remain detectable.
> > >
> > > Yes, but newly allocated slub objects will be poisoned as well.
> >
> > As far as I can tell, KMSAN hook marking newly allocated objects as
> > uninitialized is called after slub poisoning.
> > Therefore these allocations will be treated by KMSAN as uninitialized.
> > > > TBH, I haven't tested KMSAN with SLUB debug good enough. Note that
> > > > it's anyway a separate build that requires Clang, so right now it
> > > > doesn't make much sense to combine KMSAN and SLUB debug together.
> > >
> > > Can't you just build a debug kernel with SLUB debug enabled but dropping this
> > > patch? If there is an uninitialized memory here leading to data corruption, SLUB
> > > debug should be detected as well as this patch. If not, it needs to understand
> > > why.
> >
> > Sorry, there might be some misunderstanding here.
> > KMSAN keeps the state of heap objects separately by keeping exactly
> > the same amount of initialized/uninitialized bits for every
> > allocation.
> > A call to kmsan_slab_alloc()/kmsan_slab_free() will mark an allocation
> > as uninitialized for KMSAN. Not doing so will result in false reports.
> > A call to memset(object, POISON_FREE, object_size) performed by SLAB
> > debug will actually mark this allocation as initialized from KMSAN
> > point of view, because we're memsetting a range with initialized data.
> > Note that use of uninitialized data doesn't necessarily lead to
> > immediate data corruption, so there might be nothing to detect for
> > SLUB debug.
>
> Well, SLUB debug would mark any access of uninitialized memory as data
> corruption with the help of poisoning and red zoning.
>
> It is true that KMSAN might be doing a bit more than SLUB debug here, but the
> question is if it is worth the maintenance burden? Do you have any existing bugs
> to show that KMSAN would find by this patch but SLUB debug can't?
Consider the following case:

  int *a = kmalloc(sizeof(int), GFP_KERNEL);
  if (*a) {
    // do something, e.g. become root
  }

IIUC SLUB poisoning will result in the branch being taken, but as long
as there are no invalid accesses the kernel won't crash and no errors
will be reported.
KMSAN however will report an error, because the contents of the buffer
pointed to by |a| are poisoned by kmsan_slab_alloc().
The compiler instrumentation will retrieve the shadow value for *a,
compare it to zero and report a use of uninitialized memory.

Another example would be copying the contents of newly allocated
buffer to the userspace.

> > >
> > > > > > > >
> > > > > > > > Signed-off-by: Alexander Potapenko <glider@google.com>
> > > > > > > > To: Alexander Potapenko <glider@google.com>
> > > > > > > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > > > > > > Cc: Vegard Nossum <vegard.nossum@oracle.com>
> > > > > > > > Cc: Dmitry Vyukov <dvyukov@google.com>
> > > > > > > > Cc: linux-mm@kvack.org
> > > > > > > > ---
> > > > > > > >
> > > > > > > > Change-Id: I51103b7981d3aabed747d0c85cbdc85568665871
> > > > > > > > ---
> > > > > > > >  mm/slub.c | 37 +++++++++++++++++++++++++++++++------
> > > > > > > >  1 file changed, 31 insertions(+), 6 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/mm/slub.c b/mm/slub.c
> > > > > > > > index 3d63ae320d31..3d6d4c63446e 100644
> > > > > > > > --- a/mm/slub.c
> > > > > > > > +++ b/mm/slub.c
> > > > > > > > @@ -21,6 +21,8 @@
> > > > > > > >  #include <linux/proc_fs.h>
> > > > > > > >  #include <linux/seq_file.h>
> > > > > > > >  #include <linux/kasan.h>
> > > > > > > > +#include <linux/kmsan.h>
> > > > > > > > +#include <linux/kmsan-checks.h> /* KMSAN_INIT_VALUE */
> > > > > > > >  #include <linux/cpu.h>
> > > > > > > >  #include <linux/cpuset.h>
> > > > > > > >  #include <linux/mempolicy.h>
> > > > > > > > @@ -285,17 +287,27 @@ static void prefetch_freepointer(const struct kmem_cache *s, void *object)
> > > > > > > >       prefetch(object + s->offset);
> > > > > > > >  }
> > > > > > > >
> > > > > > > > +/*
> > > > > > > > + * When running under KMSAN, get_freepointer_safe() may return an uninitialized
> > > > > > > > + * pointer value in the case the current thread loses the race for the next
> > > > > > > > + * memory chunk in the freelist. In that case this_cpu_cmpxchg_double() in
> > > > > > > > + * slab_alloc_node() will fail, so the uninitialized value won't be used, but
> > > > > > > > + * KMSAN will still check all arguments of cmpxchg because of imperfect
> > > > > > > > + * handling of inline assembly.
> > > > > > > > + * To work around this problem, use KMSAN_INIT_VALUE() to force initialize the
> > > > > > > > + * return value of get_freepointer_safe().
> > > > > > > > + */
> > > > > > > >  static inline void *get_freepointer_safe(struct kmem_cache *s, void *object)
> > > > > > > >  {
> > > > > > > >       unsigned long freepointer_addr;
> > > > > > > >       void *p;
> > > > > > > >
> > > > > > > >       if (!debug_pagealloc_enabled())
> > > > > > > > -             return get_freepointer(s, object);
> > > > > > > > +             return KMSAN_INIT_VALUE(get_freepointer(s, object));
> > > > > > > >
> > > > > > > >       freepointer_addr = (unsigned long)object + s->offset;
> > > > > > > >       probe_kernel_read(&p, (void **)freepointer_addr, sizeof(p));
> > > > > > > > -     return freelist_ptr(s, p, freepointer_addr);
> > > > > > > > +     return KMSAN_INIT_VALUE(freelist_ptr(s, p, freepointer_addr));
> > > > > > > >  }
> > > > > > > >
> > > > > > > >  static inline void set_freepointer(struct kmem_cache *s, void *object, void *fp)
> > > > > > > > @@ -1390,6 +1402,7 @@ static inline void *kmalloc_large_node_hook(void *ptr, size_t size, gfp_t flags)
> > > > > > > >       ptr = kasan_kmalloc_large(ptr, size, flags);
> > > > > > > >       /* As ptr might get tagged, call kmemleak hook after KASAN. */
> > > > > > > >       kmemleak_alloc(ptr, size, 1, flags);
> > > > > > > > +     kmsan_kmalloc_large(ptr, size, flags);
> > > > > > > >       return ptr;
> > > > > > > >  }
> > > > > > > >
> > > > > > > > @@ -1397,6 +1410,7 @@ static __always_inline void kfree_hook(void *x)
> > > > > > > >  {
> > > > > > > >       kmemleak_free(x);
> > > > > > > >       kasan_kfree_large(x, _RET_IP_);
> > > > > > > > +     kmsan_kfree_large(x);
> > > > > > > >  }
> > > > > > > >
> > > > > > > >  static __always_inline bool slab_free_hook(struct kmem_cache *s, void *x)
> > > > > > > > @@ -1453,6 +1467,12 @@ static inline bool slab_free_freelist_hook(struct kmem_cache *s,
> > > > > > > >               } while (object != old_tail);
> > > > > > > >       }
> > > > > > > >
> > > > > > > > +     do {
> > > > > > > > +             object = next;
> > > > > > > > +             next = get_freepointer(s, object);
> > > > > > > > +             kmsan_slab_free(s, object);
> > > > > > > > +     } while (object != old_tail);
> > > > > > > > +
> > > > > > > >  /*
> > > > > > > >   * Compiler cannot detect this function can be removed if slab_free_hook()
> > > > > > > >   * evaluates to nothing.  Thus, catch all relevant config debug options here.
> > > > > > > > @@ -2769,6 +2789,7 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s,
> > > > > > > >       if (unlikely(slab_want_init_on_alloc(gfpflags, s)) && object)
> > > > > > > >               memset(object, 0, s->object_size);
> > > > > > > >
> > > > > > > > +     kmsan_slab_alloc(s, object, gfpflags);
> > > > > > > >       slab_post_alloc_hook(s, gfpflags, 1, &object);
> > > > > > > >
> > > > > > > >       return object;
> > > > > > > > @@ -2797,6 +2818,7 @@ void *kmem_cache_alloc_trace(struct kmem_cache *s, gfp_t gfpflags, size_t size)
> > > > > > > >       void *ret = slab_alloc(s, gfpflags, _RET_IP_);
> > > > > > > >       trace_kmalloc(_RET_IP_, ret, size, s->size, gfpflags);
> > > > > > > >       ret = kasan_kmalloc(s, ret, size, gfpflags);
> > > > > > > > +
> > > > > > > >       return ret;
> > > > > > > >  }
> > > > > > > >  EXPORT_SYMBOL(kmem_cache_alloc_trace);
> > > > > > > > @@ -2809,7 +2831,6 @@ void *kmem_cache_alloc_node(struct kmem_cache *s, gfp_t gfpflags, int node)
> > > > > > > >
> > > > > > > >       trace_kmem_cache_alloc_node(_RET_IP_, ret,
> > > > > > > >                                   s->object_size, s->size, gfpflags, node);
> > > > > > > > -
> > > > > > > >       return ret;
> > > > > > > >  }
> > > > > > > >  EXPORT_SYMBOL(kmem_cache_alloc_node);
> > > > > > > > @@ -2825,6 +2846,7 @@ void *kmem_cache_alloc_node_trace(struct kmem_cache *s,
> > > > > > > >                          size, s->size, gfpflags, node);
> > > > > > > >
> > > > > > > >       ret = kasan_kmalloc(s, ret, size, gfpflags);
> > > > > > > > +
> > > > > > > >       return ret;
> > > > > > > >  }
> > > > > > > >  EXPORT_SYMBOL(kmem_cache_alloc_node_trace);
> > > > > > > > @@ -3150,7 +3172,7 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
> > > > > > > >                         void **p)
> > > > > > > >  {
> > > > > > > >       struct kmem_cache_cpu *c;
> > > > > > > > -     int i;
> > > > > > > > +     int i, j;
> > > > > > > >
> > > > > > > >       /* memcg and kmem_cache debug support */
> > > > > > > >       s = slab_pre_alloc_hook(s, flags);
> > > > > > > > @@ -3188,11 +3210,11 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
> > > > > > > >
> > > > > > > >       /* Clear memory outside IRQ disabled fastpath loop */
> > > > > > > >       if (unlikely(slab_want_init_on_alloc(flags, s))) {
> > > > > > > > -             int j;
> > > > > > > > -
> > > > > > > >               for (j = 0; j < i; j++)
> > > > > > > >                       memset(p[j], 0, s->object_size);
> > > > > > > >       }
> > > > > > > > +     for (j = 0; j < i; j++)
> > > > > > > > +             kmsan_slab_alloc(s, p[j], flags);
> > > > > > > >
> > > > > > > >       /* memcg and kmem_cache debug support */
> > > > > > > >       slab_post_alloc_hook(s, flags, size, p);
> > > > > > > > @@ -3793,6 +3815,7 @@ static int __init setup_slub_min_objects(char *str)
> > > > > > > >
> > > > > > > >  __setup("slub_min_objects=", setup_slub_min_objects);
> > > > > > > >
> > > > > > > > +__no_sanitize_memory
> > > > > > > >  void *__kmalloc(size_t size, gfp_t flags)
> > > > > > > >  {
> > > > > > > >       struct kmem_cache *s;
> > > > > > > > @@ -5698,6 +5721,7 @@ static char *create_unique_id(struct kmem_cache *s)
> > > > > > > >       p += sprintf(p, "%07u", s->size);
> > > > > > > >
> > > > > > > >       BUG_ON(p > name + ID_STR_LENGTH - 1);
> > > > > > > > +     kmsan_unpoison_shadow(name, p - name);
> > > > > > > >       return name;
> > > > > > > >  }
> > > > > > > >
> > > > > > > > @@ -5847,6 +5871,7 @@ static int sysfs_slab_alias(struct kmem_cache *s, const char *name)
> > > > > > > >       al->name = name;
> > > > > > > >       al->next = alias_list;
> > > > > > > >       alias_list = al;
> > > > > > > > +     kmsan_unpoison_shadow(al, sizeof(struct saved_alias));
> > > > > > > >       return 0;
> > > > > > > >  }
> > > > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > >
> > > >
> > > >
> >
> >
> >

Qian Cai Oct. 18, 2019, 4:08 p.m. UTC | #9

On Fri, 2019-10-18 at 17:30 +0200, Alexander Potapenko wrote:
> On Fri, Oct 18, 2019 at 5:13 PM Qian Cai <cai@lca.pw> wrote:
> > 
> > On Fri, 2019-10-18 at 16:54 +0200, Alexander Potapenko wrote:
> > > On Fri, Oct 18, 2019 at 4:42 PM Qian Cai <cai@lca.pw> wrote:
> > > > 
> > > > On Fri, 2019-10-18 at 15:55 +0200, Alexander Potapenko wrote:
> > > > > On Fri, Oct 18, 2019 at 3:42 PM Qian Cai <cai@lca.pw> wrote:
> > > > > > 
> > > > > > On Fri, 2019-10-18 at 15:33 +0200, Alexander Potapenko wrote:
> > > > > > > On Fri, Oct 18, 2019 at 3:22 PM Qian Cai <cai@lca.pw> wrote:
> > > > > > > > 
> > > > > > > > On Fri, 2019-10-18 at 11:42 +0200, glider@google.com wrote:
> > > > > > > > > In order to report uninitialized memory coming from heap allocations
> > > > > > > > > KMSAN has to poison them unless they're created with __GFP_ZERO.
> > > > > > > > > 
> > > > > > > > > It's handy that we need KMSAN hooks in the places where
> > > > > > > > > init_on_alloc/init_on_free initialization is performed.
> > > > > > > > 
> > > > > > > > Well, there is SLUB debug which has red zoning and poisoning checks. What's
> > > > > > > > value of this patch?
> > > > > > > 
> > > > > > > Sorry, are you talking about the whole patchset or just this patch?
> > > > > > 
> > > > > > Just this patch.
> > > > > > 
> > > > > > > Note that SLUB debug is unable to detect uninitialized values with
> > > > > > > bit-to-bit precision, neither have I heard of anyone using it for
> > > > > > > detecting uses of uninitialized memory in the kernel at all.
> > > > > > > The purpose of SLUB debug is to detect corruptions of freed memory.
> > > > > > 
> > > > > > The point is if developers have SLUB debug enabled, all the free objects will be
> > > > > > poisoned, so what's the point of checking uninitialized memory there?
> > > > > 
> > > > > You are right, SLUB debug has to be handled separately. If I'm
> > > > > understanding correctly, right now KMSAN poisons freed memory before
> > > > > SLUB debug wipes it, therefore the memory will count as initialized.
> > > > > On the other hand, newly allocated memory is still marked as
> > > > > uninitialized, so a lot of bugs still remain detectable.
> > > > 
> > > > Yes, but newly allocated slub objects will be poisoned as well.
> > > 
> > > As far as I can tell, KMSAN hook marking newly allocated objects as
> > > uninitialized is called after slub poisoning.
> > > Therefore these allocations will be treated by KMSAN as uninitialized.
> > > > > TBH, I haven't tested KMSAN with SLUB debug good enough. Note that
> > > > > it's anyway a separate build that requires Clang, so right now it
> > > > > doesn't make much sense to combine KMSAN and SLUB debug together.
> > > > 
> > > > Can't you just build a debug kernel with SLUB debug enabled but dropping this
> > > > patch? If there is an uninitialized memory here leading to data corruption, SLUB
> > > > debug should be detected as well as this patch. If not, it needs to understand
> > > > why.
> > > 
> > > Sorry, there might be some misunderstanding here.
> > > KMSAN keeps the state of heap objects separately by keeping exactly
> > > the same amount of initialized/uninitialized bits for every
> > > allocation.
> > > A call to kmsan_slab_alloc()/kmsan_slab_free() will mark an allocation
> > > as uninitialized for KMSAN. Not doing so will result in false reports.
> > > A call to memset(object, POISON_FREE, object_size) performed by SLAB
> > > debug will actually mark this allocation as initialized from KMSAN
> > > point of view, because we're memsetting a range with initialized data.
> > > Note that use of uninitialized data doesn't necessarily lead to
> > > immediate data corruption, so there might be nothing to detect for
> > > SLUB debug.
> > 
> > Well, SLUB debug would mark any access of uninitialized memory as data
> > corruption with the help of poisoning and red zoning.
> > 
> > It is true that KMSAN might be doing a bit more than SLUB debug here, but the
> > question is if it is worth the maintenance burden? Do you have any existing bugs
> > to show that KMSAN would find by this patch but SLUB debug can't?
> 
> Consider the following case:
> 
>   int *a = kmalloc(sizeof(int), GFP_KERNEL);
>   if (*a) {
>     // do something, e.g. become root
>   }
> 
> IIUC SLUB poisoning will result in the branch being taken, but as long
> as there are no invalid accesses the kernel won't crash and no errors
> will be reported.
> KMSAN however will report an error, because the contents of the buffer
> pointed to by |a| are poisoned by kmsan_slab_alloc().
> The compiler instrumentation will retrieve the shadow value for *a,
> compare it to zero and report a use of uninitialized memory.
> 
> Another example would be copying the contents of newly allocated
> buffer to the userspace.

Make sense. BTW, it might worth testing this patchset with this MM debug config
based on linux-next to edge out some corner cases if you could,

https://raw.githubusercontent.com/cailca/linux-mm/master/x86.config

> 
> > > > 
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Alexander Potapenko <glider@google.com>
> > > > > > > > > To: Alexander Potapenko <glider@google.com>
> > > > > > > > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > > > > > > > Cc: Vegard Nossum <vegard.nossum@oracle.com>
> > > > > > > > > Cc: Dmitry Vyukov <dvyukov@google.com>
> > > > > > > > > Cc: linux-mm@kvack.org
> > > > > > > > > ---
> > > > > > > > > 
> > > > > > > > > Change-Id: I51103b7981d3aabed747d0c85cbdc85568665871
> > > > > > > > > ---
> > > > > > > > >  mm/slub.c | 37 +++++++++++++++++++++++++++++++------
> > > > > > > > >  1 file changed, 31 insertions(+), 6 deletions(-)
> > > > > > > > > 
> > > > > > > > > diff --git a/mm/slub.c b/mm/slub.c
> > > > > > > > > index 3d63ae320d31..3d6d4c63446e 100644
> > > > > > > > > --- a/mm/slub.c
> > > > > > > > > +++ b/mm/slub.c
> > > > > > > > > @@ -21,6 +21,8 @@
> > > > > > > > >  #include <linux/proc_fs.h>
> > > > > > > > >  #include <linux/seq_file.h>
> > > > > > > > >  #include <linux/kasan.h>
> > > > > > > > > +#include <linux/kmsan.h>
> > > > > > > > > +#include <linux/kmsan-checks.h> /* KMSAN_INIT_VALUE */
> > > > > > > > >  #include <linux/cpu.h>
> > > > > > > > >  #include <linux/cpuset.h>
> > > > > > > > >  #include <linux/mempolicy.h>
> > > > > > > > > @@ -285,17 +287,27 @@ static void prefetch_freepointer(const struct kmem_cache *s, void *object)
> > > > > > > > >       prefetch(object + s->offset);
> > > > > > > > >  }
> > > > > > > > > 
> > > > > > > > > +/*
> > > > > > > > > + * When running under KMSAN, get_freepointer_safe() may return an uninitialized
> > > > > > > > > + * pointer value in the case the current thread loses the race for the next
> > > > > > > > > + * memory chunk in the freelist. In that case this_cpu_cmpxchg_double() in
> > > > > > > > > + * slab_alloc_node() will fail, so the uninitialized value won't be used, but
> > > > > > > > > + * KMSAN will still check all arguments of cmpxchg because of imperfect
> > > > > > > > > + * handling of inline assembly.
> > > > > > > > > + * To work around this problem, use KMSAN_INIT_VALUE() to force initialize the
> > > > > > > > > + * return value of get_freepointer_safe().
> > > > > > > > > + */
> > > > > > > > >  static inline void *get_freepointer_safe(struct kmem_cache *s, void *object)
> > > > > > > > >  {
> > > > > > > > >       unsigned long freepointer_addr;
> > > > > > > > >       void *p;
> > > > > > > > > 
> > > > > > > > >       if (!debug_pagealloc_enabled())
> > > > > > > > > -             return get_freepointer(s, object);
> > > > > > > > > +             return KMSAN_INIT_VALUE(get_freepointer(s, object));
> > > > > > > > > 
> > > > > > > > >       freepointer_addr = (unsigned long)object + s->offset;
> > > > > > > > >       probe_kernel_read(&p, (void **)freepointer_addr, sizeof(p));
> > > > > > > > > -     return freelist_ptr(s, p, freepointer_addr);
> > > > > > > > > +     return KMSAN_INIT_VALUE(freelist_ptr(s, p, freepointer_addr));
> > > > > > > > >  }
> > > > > > > > > 
> > > > > > > > >  static inline void set_freepointer(struct kmem_cache *s, void *object, void *fp)
> > > > > > > > > @@ -1390,6 +1402,7 @@ static inline void *kmalloc_large_node_hook(void *ptr, size_t size, gfp_t flags)
> > > > > > > > >       ptr = kasan_kmalloc_large(ptr, size, flags);
> > > > > > > > >       /* As ptr might get tagged, call kmemleak hook after KASAN. */
> > > > > > > > >       kmemleak_alloc(ptr, size, 1, flags);
> > > > > > > > > +     kmsan_kmalloc_large(ptr, size, flags);
> > > > > > > > >       return ptr;
> > > > > > > > >  }
> > > > > > > > > 
> > > > > > > > > @@ -1397,6 +1410,7 @@ static __always_inline void kfree_hook(void *x)
> > > > > > > > >  {
> > > > > > > > >       kmemleak_free(x);
> > > > > > > > >       kasan_kfree_large(x, _RET_IP_);
> > > > > > > > > +     kmsan_kfree_large(x);
> > > > > > > > >  }
> > > > > > > > > 
> > > > > > > > >  static __always_inline bool slab_free_hook(struct kmem_cache *s, void *x)
> > > > > > > > > @@ -1453,6 +1467,12 @@ static inline bool slab_free_freelist_hook(struct kmem_cache *s,
> > > > > > > > >               } while (object != old_tail);
> > > > > > > > >       }
> > > > > > > > > 
> > > > > > > > > +     do {
> > > > > > > > > +             object = next;
> > > > > > > > > +             next = get_freepointer(s, object);
> > > > > > > > > +             kmsan_slab_free(s, object);
> > > > > > > > > +     } while (object != old_tail);
> > > > > > > > > +
> > > > > > > > >  /*
> > > > > > > > >   * Compiler cannot detect this function can be removed if slab_free_hook()
> > > > > > > > >   * evaluates to nothing.  Thus, catch all relevant config debug options here.
> > > > > > > > > @@ -2769,6 +2789,7 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s,
> > > > > > > > >       if (unlikely(slab_want_init_on_alloc(gfpflags, s)) && object)
> > > > > > > > >               memset(object, 0, s->object_size);
> > > > > > > > > 
> > > > > > > > > +     kmsan_slab_alloc(s, object, gfpflags);
> > > > > > > > >       slab_post_alloc_hook(s, gfpflags, 1, &object);
> > > > > > > > > 
> > > > > > > > >       return object;
> > > > > > > > > @@ -2797,6 +2818,7 @@ void *kmem_cache_alloc_trace(struct kmem_cache *s, gfp_t gfpflags, size_t size)
> > > > > > > > >       void *ret = slab_alloc(s, gfpflags, _RET_IP_);
> > > > > > > > >       trace_kmalloc(_RET_IP_, ret, size, s->size, gfpflags);
> > > > > > > > >       ret = kasan_kmalloc(s, ret, size, gfpflags);
> > > > > > > > > +
> > > > > > > > >       return ret;
> > > > > > > > >  }
> > > > > > > > >  EXPORT_SYMBOL(kmem_cache_alloc_trace);
> > > > > > > > > @@ -2809,7 +2831,6 @@ void *kmem_cache_alloc_node(struct kmem_cache *s, gfp_t gfpflags, int node)
> > > > > > > > > 
> > > > > > > > >       trace_kmem_cache_alloc_node(_RET_IP_, ret,
> > > > > > > > >                                   s->object_size, s->size, gfpflags, node);
> > > > > > > > > -
> > > > > > > > >       return ret;
> > > > > > > > >  }
> > > > > > > > >  EXPORT_SYMBOL(kmem_cache_alloc_node);
> > > > > > > > > @@ -2825,6 +2846,7 @@ void *kmem_cache_alloc_node_trace(struct kmem_cache *s,
> > > > > > > > >                          size, s->size, gfpflags, node);
> > > > > > > > > 
> > > > > > > > >       ret = kasan_kmalloc(s, ret, size, gfpflags);
> > > > > > > > > +
> > > > > > > > >       return ret;
> > > > > > > > >  }
> > > > > > > > >  EXPORT_SYMBOL(kmem_cache_alloc_node_trace);
> > > > > > > > > @@ -3150,7 +3172,7 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
> > > > > > > > >                         void **p)
> > > > > > > > >  {
> > > > > > > > >       struct kmem_cache_cpu *c;
> > > > > > > > > -     int i;
> > > > > > > > > +     int i, j;
> > > > > > > > > 
> > > > > > > > >       /* memcg and kmem_cache debug support */
> > > > > > > > >       s = slab_pre_alloc_hook(s, flags);
> > > > > > > > > @@ -3188,11 +3210,11 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
> > > > > > > > > 
> > > > > > > > >       /* Clear memory outside IRQ disabled fastpath loop */
> > > > > > > > >       if (unlikely(slab_want_init_on_alloc(flags, s))) {
> > > > > > > > > -             int j;
> > > > > > > > > -
> > > > > > > > >               for (j = 0; j < i; j++)
> > > > > > > > >                       memset(p[j], 0, s->object_size);
> > > > > > > > >       }
> > > > > > > > > +     for (j = 0; j < i; j++)
> > > > > > > > > +             kmsan_slab_alloc(s, p[j], flags);
> > > > > > > > > 
> > > > > > > > >       /* memcg and kmem_cache debug support */
> > > > > > > > >       slab_post_alloc_hook(s, flags, size, p);
> > > > > > > > > @@ -3793,6 +3815,7 @@ static int __init setup_slub_min_objects(char *str)
> > > > > > > > > 
> > > > > > > > >  __setup("slub_min_objects=", setup_slub_min_objects);
> > > > > > > > > 
> > > > > > > > > +__no_sanitize_memory
> > > > > > > > >  void *__kmalloc(size_t size, gfp_t flags)
> > > > > > > > >  {
> > > > > > > > >       struct kmem_cache *s;
> > > > > > > > > @@ -5698,6 +5721,7 @@ static char *create_unique_id(struct kmem_cache *s)
> > > > > > > > >       p += sprintf(p, "%07u", s->size);
> > > > > > > > > 
> > > > > > > > >       BUG_ON(p > name + ID_STR_LENGTH - 1);
> > > > > > > > > +     kmsan_unpoison_shadow(name, p - name);
> > > > > > > > >       return name;
> > > > > > > > >  }
> > > > > > > > > 
> > > > > > > > > @@ -5847,6 +5871,7 @@ static int sysfs_slab_alias(struct kmem_cache *s, const char *name)
> > > > > > > > >       al->name = name;
> > > > > > > > >       al->next = alias_list;
> > > > > > > > >       alias_list = al;
> > > > > > > > > +     kmsan_unpoison_shadow(al, sizeof(struct saved_alias));
> > > > > > > > >       return 0;
> > > > > > > > >  }
> > > > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > 
> > > 
> > > 
> 
> 
>

[RFC,v1,18/26] kmsan: mm: call KMSAN hooks from SLUB code

Commit Message

Comments

Patch