kasan: more tag based mode fixes

PATCH Superseded Andrey Konovalov 5 participants 17 messages Feb 11, 2019 – Feb 15, 2019

Subsystems (4)

Memory management Slab allocator KASAN Kmemleak

lore

Share & tools

lore

Andrey Konovalov

author

2019-02-11 22:00 UTC #0 lore raw

cover kasan: more tag based mode fixes

Andrey Konovalov (5):
kasan: fix assigning tags twice
kasan, kmemleak: pass tagged pointers to kmemleak
kmemleak: account for tagged pointers when calculating pointer range kasan, slub: move kasan_poison_slab hook before page_address kasan, slub: fix conflicts with CONFIG_SLAB_FREELIST_HARDENED

 mm/kasan/common.c | 29 +++++++++++++++++------------
 mm/kmemleak.c     | 10 +++++++---
 mm/slab.h         |  6 ++----
 mm/slab_common.c  |  2 +-
 mm/slub.c         | 32 +++++++++++++++-----------------
 5 files changed, 42 insertions(+), 37 deletions(-)

Signature · 1 line

2.20.1.791.gb4d0f1c61a-goog

Andrey Konovalov

author

2019-02-11 22:00 UTC #1 lore raw

PATCH 1/5 kasan: fix assigning tags twice

When an object is kmalloc()'ed, two hooks are called: kasan_slab_alloc() and kasan_kmalloc(). Right now we assign a tag twice, once in each of the hooks. Fix it by assigning a tag only in the former hook.

Signed-off-by Andrey Konovalov <[email protected]>

---
 mm/kasan/common.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

mm/kasan/common.c +17 ~~-12~~

index 73c9cbfdedf4..09b534fbba17 100644
@@ -361,10 +361,15 @@ void kasan_poison_object_data(struct kmem_cache *cache, void *object)
  *    get different tags.
  */
 static u8 assign_tag(struct kmem_cache *cache, const void *object,
-			bool init, bool krealloc)
+			bool init, bool keep_tag)
 {
-	/* Reuse the same tag for krealloc'ed objects. */
-	if (krealloc)
+	/*
+	 * 1. When an object is kmalloc()'ed, two hooks are called:
+	 *    kasan_slab_alloc() and kasan_kmalloc(). We assign the
+	 *    tag only in the first one.
+	 * 2. We reuse the same tag for krealloc'ed objects.
+	 */
+	if (keep_tag)
 		return get_tag(object);
 
 	/*

@@ -405,12 +410,6 @@ void * __must_check kasan_init_slab_obj(struct kmem_cache *cache,
 	return (void *)object;
 }
 
-void * __must_check kasan_slab_alloc(struct kmem_cache *cache, void *object,
-					gfp_t flags)
-{
-	return kasan_kmalloc(cache, object, cache->object_size, flags);
-}
-
 static inline bool shadow_invalid(u8 tag, s8 shadow_byte)
 {
 	if (IS_ENABLED(CONFIG_KASAN_GENERIC))

@@ -467,7 +466,7 @@ bool kasan_slab_free(struct kmem_cache *cache, void *object, unsigned long ip)
 }
 
 static void *__kasan_kmalloc(struct kmem_cache *cache, const void *object,
-				size_t size, gfp_t flags, bool krealloc)
+				size_t size, gfp_t flags, bool keep_tag)
 {
 	unsigned long redzone_start;
 	unsigned long redzone_end;

@@ -485,7 +484,7 @@ static void *__kasan_kmalloc(struct kmem_cache *cache, const void *object,
 				KASAN_SHADOW_SCALE_SIZE);
 
 	if (IS_ENABLED(CONFIG_KASAN_SW_TAGS))
-		tag = assign_tag(cache, object, false, krealloc);
+		tag = assign_tag(cache, object, false, keep_tag);
 
 	/* Tag is ignored in set_tag without CONFIG_KASAN_SW_TAGS */
 	kasan_unpoison_shadow(set_tag(object, tag), size);

@@ -498,10 +497,16 @@ static void *__kasan_kmalloc(struct kmem_cache *cache, const void *object,
 	return set_tag(object, tag);
 }
 
+void * __must_check kasan_slab_alloc(struct kmem_cache *cache, void *object,
+					gfp_t flags)
+{
+	return __kasan_kmalloc(cache, object, cache->object_size, flags, false);
+}
+
 void * __must_check kasan_kmalloc(struct kmem_cache *cache, const void *object,
 				size_t size, gfp_t flags)
 {
-	return __kasan_kmalloc(cache, object, size, flags, false);
+	return __kasan_kmalloc(cache, object, size, flags, true);
 }
 EXPORT_SYMBOL(kasan_kmalloc);

Signature · 1 line

2.20.1.791.gb4d0f1c61a-goog

Andrey Konovalov

author

2019-02-11 22:00 UTC #2 lore raw

PATCH 2/5 kasan, kmemleak: pass tagged pointers to kmemleak

In reply to Andrey Konovalov

Right now we call kmemleak hooks before assigning tags to pointers in KASAN hooks. As a result, when an objects gets allocated, kmemleak sees a differently tagged pointer, compared to the one it sees when the object gets freed. Fix it by calling KASAN hooks before kmemleak's ones.

Reported-by Qian Cai <[email protected]>

Signed-off-by Andrey Konovalov <[email protected]>

---
 mm/slab.h        | 6 ++----
 mm/slab_common.c | 2 +-
 mm/slub.c        | 3 ++-
 3 files changed, 5 insertions(+), 6 deletions(-)

mm/slab.h +2 -4

index 4190c24ef0e9..638ea1b25d39 100644
@@ -437,11 +437,9 @@ static inline void slab_post_alloc_hook(struct kmem_cache *s, gfp_t flags,
 
 	flags &= gfp_allowed_mask;
 	for (i = 0; i < size; i++) {
-		void *object = p[i];
-
-		kmemleak_alloc_recursive(object, s->object_size, 1,
+		p[i] = kasan_slab_alloc(s, p[i], flags);
+		kmemleak_alloc_recursive(p[i], s->object_size, 1,
 					 s->flags, flags);
-		p[i] = kasan_slab_alloc(s, object, flags);
 	}
 
 	if (memcg_kmem_enabled())

mm/slab_common.c +1 -1

index 81732d05e74a..fe524c8d0246 100644
@@ -1228,8 +1228,8 @@ void *kmalloc_order(size_t size, gfp_t flags, unsigned int order)
 	flags |= __GFP_COMP;
 	page = alloc_pages(flags, order);
 	ret = page ? page_address(page) : NULL;
-	kmemleak_alloc(ret, size, 1, flags);
 	ret = kasan_kmalloc_large(ret, size, flags);
+	kmemleak_alloc(ret, size, 1, flags);
 	return ret;
 }
 EXPORT_SYMBOL(kmalloc_order);

mm/slub.c +2 -1

index 1e3d0ec4e200..4a3d7686902f 100644
@@ -1374,8 +1374,9 @@ static inline void dec_slabs_node(struct kmem_cache *s, int node,
  */
 static inline void *kmalloc_large_node_hook(void *ptr, size_t size, gfp_t flags)
 {
+	ptr = kasan_kmalloc_large(ptr, size, flags);
 	kmemleak_alloc(ptr, size, 1, flags);
-	return kasan_kmalloc_large(ptr, size, flags);
+	return ptr;
 }
 
 static __always_inline void kfree_hook(void *x)

Signature · 1 line

2.20.1.791.gb4d0f1c61a-goog

Vincenzo Frascino

reviewer

2019-02-12 15:57 UTC #3 lore raw

Re: [PATCH 2/5] kasan, kmemleak: pass tagged pointers to kmemleak

Quoted 5 lines · Andrey Konovalov

Right now we call kmemleak hooks before assigning tags to pointers in KASAN hooks. As a result, when an objects gets allocated, kmemleak sees a differently tagged pointer, compared to the one it sees when the object gets freed. Fix it by calling KASAN hooks before kmemleak's ones.

Nit: Could you please add comments to the the code? It should prevent that the code gets refactored in future, reintroducing the same issue.

Quoted 56 lines

Reported-by Qian Cai <[email protected]>

Signed-off-by Andrey Konovalov <[email protected]>

---
 mm/slab.h        | 6 ++----
 mm/slab_common.c | 2 +-
 mm/slub.c        | 3 ++-
 3 files changed, 5 insertions(+), 6 deletions(-)

mm/slab.h +2 -4

index 4190c24ef0e9..638ea1b25d39 100644
@@ -437,11 +437,9 @@ static inline void slab_post_alloc_hook(struct kmem_cache *s, gfp_t flags,
 
 	flags &= gfp_allowed_mask;
 	for (i = 0; i < size; i++) {
-		void *object = p[i];
-
-		kmemleak_alloc_recursive(object, s->object_size, 1,
+		p[i] = kasan_slab_alloc(s, p[i], flags);
+		kmemleak_alloc_recursive(p[i], s->object_size, 1,
 					 s->flags, flags);
-		p[i] = kasan_slab_alloc(s, object, flags);
 	}
 
 	if (memcg_kmem_enabled())

mm/slab_common.c +1 -1

index 81732d05e74a..fe524c8d0246 100644
@@ -1228,8 +1228,8 @@ void *kmalloc_order(size_t size, gfp_t flags, unsigned int order)
 	flags |= __GFP_COMP;
 	page = alloc_pages(flags, order);
 	ret = page ? page_address(page) : NULL;
-	kmemleak_alloc(ret, size, 1, flags);
 	ret = kasan_kmalloc_large(ret, size, flags);
+	kmemleak_alloc(ret, size, 1, flags);
 	return ret;
 }
 EXPORT_SYMBOL(kmalloc_order);

mm/slub.c +2 -1

index 1e3d0ec4e200..4a3d7686902f 100644
@@ -1374,8 +1374,9 @@ static inline void dec_slabs_node(struct kmem_cache *s, int node,
  */
 static inline void *kmalloc_large_node_hook(void *ptr, size_t size, gfp_t flags)
 {
+	ptr = kasan_kmalloc_large(ptr, size, flags);
 	kmemleak_alloc(ptr, size, 1, flags);
-	return kasan_kmalloc_large(ptr, size, flags);
+	return ptr;
 }
 
 static __always_inline void kfree_hook(void *x)

Signature · 2 lines

Regards,
Vincenzo

Andrey Konovalov

author

2019-02-13 13:07 UTC #4 lore raw

Re: [PATCH 2/5] kasan, kmemleak: pass tagged pointers to kmemleak

Quoted 10 lines · Vincenzo Frascino

Quoted 5 lines · Andrey Konovalov
Right now we call kmemleak hooks before assigning tags to pointers in KASAN hooks. As a result, when an objects gets allocated, kmemleak sees a differently tagged pointer, compared to the one it sees when the object gets freed. Fix it by calling KASAN hooks before kmemleak's ones.
Nit: Could you please add comments to the the code? It should prevent that the code gets refactored in future, reintroducing the same issue.

Sure, I'll send v2 with comments, thanks!

Quoted 68 lines

Quoted 47 lines

Reported-by Qian Cai <[email protected]>

Signed-off-by Andrey Konovalov <[email protected]>

---
 mm/slab.h        | 6 ++----
 mm/slab_common.c | 2 +-
 mm/slub.c        | 3 ++-
 3 files changed, 5 insertions(+), 6 deletions(-)

mm/slab.h +2 -4

index 4190c24ef0e9..638ea1b25d39 100644
@@ -437,11 +437,9 @@ static inline void slab_post_alloc_hook(struct kmem_cache *s, gfp_t flags,
 
      flags &= gfp_allowed_mask;
      for (i = 0; i < size; i++) {
-             void *object = p[i];
-
-             kmemleak_alloc_recursive(object, s->object_size, 1,
+             p[i] = kasan_slab_alloc(s, p[i], flags);
+             kmemleak_alloc_recursive(p[i], s->object_size, 1,
                                       s->flags, flags);
-             p[i] = kasan_slab_alloc(s, object, flags);
      }
 
      if (memcg_kmem_enabled())

mm/slab_common.c +1 -1

index 81732d05e74a..fe524c8d0246 100644
@@ -1228,8 +1228,8 @@ void *kmalloc_order(size_t size, gfp_t flags, unsigned int order)
      flags |= __GFP_COMP;
      page = alloc_pages(flags, order);
      ret = page ? page_address(page) : NULL;
-     kmemleak_alloc(ret, size, 1, flags);
      ret = kasan_kmalloc_large(ret, size, flags);
+     kmemleak_alloc(ret, size, 1, flags);
      return ret;
 }
 EXPORT_SYMBOL(kmalloc_order);

mm/slub.c +0 -0

index 1e3d0ec4e200..4a3d7686902f 100644

mm/slub.c static inline void dec_slabs_node(struct kmem_cache *s, int node, · line 1382

Quoted 10 lines

mm/slub.c +2 -1

@@ -1374,8 +1374,9 @@ static inline void dec_slabs_node(struct kmem_cache *s, int node,
  */
 static inline void *kmalloc_large_node_hook(void *ptr, size_t size, gfp_t flags)
 {
+     ptr = kasan_kmalloc_large(ptr, size, flags);
      kmemleak_alloc(ptr, size, 1, flags);
-     return kasan_kmalloc_large(ptr, size, flags);
+     return ptr;
 }
 
 static __always_inline void kfree_hook(void *x)

--
Regards,
Vincenzo

--
You received this message because you are subscribed to the Google Groups "kasan-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected].
To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/f57831be-c57a-4a9e-992e-1f193866467b%40arm.com.
For more options, visit https://groups.google.com/d/optout.

Andrey Konovalov

author

2019-02-11 22:00 UTC #5 lore raw

PATCH 3/5 kmemleak: account for tagged pointers when calculating pointer range

In reply to Andrey Konovalov

kmemleak keeps two global variables, min_addr and max_addr, which store the range of valid (encountered by kmemleak) pointer values, which it later uses to speed up pointer lookup when scanning blocks.

With tagged pointers this range will get bigger than it needs to be. This patch makes kmemleak untag pointers before saving them to min_addr and max_addr and when performing a lookup.

Signed-off-by Andrey Konovalov <[email protected]>

---
 mm/kmemleak.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

mm/kmemleak.c +7 -3

index f9d9dc250428..707fa5579f66 100644
@@ -574,6 +574,7 @@ static struct kmemleak_object *create_object(unsigned long ptr, size_t size,
 	unsigned long flags;
 	struct kmemleak_object *object, *parent;
 	struct rb_node **link, *rb_parent;
+	unsigned long untagged_ptr;
 
 	object = kmem_cache_alloc(object_cache, gfp_kmemleak_mask(gfp));
 	if (!object) {

@@ -619,8 +620,9 @@ static struct kmemleak_object *create_object(unsigned long ptr, size_t size,
 
 	write_lock_irqsave(&kmemleak_lock, flags);
 
-	min_addr = min(min_addr, ptr);
-	max_addr = max(max_addr, ptr + size);
+	untagged_ptr = (unsigned long)kasan_reset_tag((void *)ptr);
+	min_addr = min(min_addr, untagged_ptr);
+	max_addr = max(max_addr, untagged_ptr + size);
 	link = &object_tree_root.rb_node;
 	rb_parent = NULL;
 	while (*link) {

@@ -1333,6 +1335,7 @@ static void scan_block(void *_start, void *_end,
 	unsigned long *start = PTR_ALIGN(_start, BYTES_PER_POINTER);
 	unsigned long *end = _end - (BYTES_PER_POINTER - 1);
 	unsigned long flags;
+	unsigned long untagged_ptr;
 
 	read_lock_irqsave(&kmemleak_lock, flags);
 	for (ptr = start; ptr < end; ptr++) {

@@ -1347,7 +1350,8 @@ static void scan_block(void *_start, void *_end,
 		pointer = *ptr;
 		kasan_enable_current();
 
-		if (pointer < min_addr || pointer >= max_addr)
+		untagged_ptr = (unsigned long)kasan_reset_tag((void *)pointer);
+		if (untagged_ptr < min_addr || untagged_ptr >= max_addr)
 			continue;
 
 		/*

Signature · 1 line

2.20.1.791.gb4d0f1c61a-goog

Catalin Marinas

maintainer Acked-by on v1 3/5

2019-02-15 14:05 UTC #6 lore raw

Re: [PATCH 3/5] kmemleak: account for tagged pointers when calculating pointer range

Quoted 9 lines · Andrey Konovalov

kmemleak keeps two global variables, min_addr and max_addr, which store the range of valid (encountered by kmemleak) pointer values, which it later uses to speed up pointer lookup when scanning blocks.
With tagged pointers this range will get bigger than it needs to be. This patch makes kmemleak untag pointers before saving them to min_addr and max_addr and when performing a lookup.
Signed-off-by Andrey Konovalov <[email protected]>

Acked-by Catalin Marinas <[email protected]>

Andrey Konovalov

author

2019-02-11 22:00 UTC #7 lore raw

PATCH 4/5 kasan, slub: move kasan_poison_slab hook before page_address

In reply to Andrey Konovalov

With tag based KASAN page_address() looks at the page flags to see whether the resulting pointer needs to have a tag set. Since we don't want to set a tag when page_address() is called on SLAB pages, we call page_kasan_tag_reset() in kasan_poison_slab(). However in allocate_slab() page_address() is called before kasan_poison_slab(). Fix it by changing the order.

Signed-off-by Andrey Konovalov <[email protected]>

---
 mm/slub.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

mm/slub.c +6 -3

index 4a3d7686902f..ce874a5c9ee7 100644
@@ -1642,12 +1642,15 @@ static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)
 	if (page_is_pfmemalloc(page))
 		SetPageSlabPfmemalloc(page);
 
+	kasan_poison_slab(page);
+
 	start = page_address(page);
 
-	if (unlikely(s->flags & SLAB_POISON))
+	if (unlikely(s->flags & SLAB_POISON)) {
+		metadata_access_enable();
 		memset(start, POISON_INUSE, PAGE_SIZE << order);
-
-	kasan_poison_slab(page);
+		metadata_access_disable();
+	}
 
 	shuffle = shuffle_freelist(s, page);

Signature · 1 line

2.20.1.791.gb4d0f1c61a-goog

Andrew Morton

maintainer

2019-02-12 21:12 UTC #8 lore raw

Re: [PATCH 4/5] kasan, slub: move kasan_poison_slab hook before page_address

Quoted 19 lines · On Mon, 11 Feb 2019 22:59:53 +0100 Andrey Konovalov <[email protected]>…

With tag based KASAN page_address() looks at the page flags to see whether the resulting pointer needs to have a tag set. Since we don't want to set a tag when page_address() is called on SLAB pages, we call page_kasan_tag_reset() in kasan_poison_slab(). However in allocate_slab() page_address() is called before kasan_poison_slab(). Fix it by changing the order.
...
mm/slub.c +2 -0
@@ -1642,12 +1642,15 @@ static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)
 	if (page_is_pfmemalloc(page))
 		SetPageSlabPfmemalloc(page);
 
+	kasan_poison_slab(page);
+
 	start = page_address(page);
 

mm/slub.c static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node) · line 1655

Quoted 10 lines

mm/slub.c +4 -3

-	if (unlikely(s->flags & SLAB_POISON))
+	if (unlikely(s->flags & SLAB_POISON)) {
+		metadata_access_enable();
 		memset(start, POISON_INUSE, PAGE_SIZE << order);
-
-	kasan_poison_slab(page);
+		metadata_access_disable();
+	}
 
 	shuffle = shuffle_freelist(s, page);

This doesn't compile when CONFIG_SLUB_DEBUG=n. Please review carefully:

mm/slub.c renamed from mm/slub.c~kasan-slub-move-kasan_poison_slab-hook-before-page_address-fix +8 -0

@@ -1357,6 +1357,14 @@ slab_flags_t kmem_cache_flags(unsigned i
 
 #define disable_higher_order_debug 0
 
+static inline void metadata_access_enable(void)
+{
+}
+
+static inline void metadata_access_disable(void)
+{
+}
+
 static inline unsigned long slabs_node(struct kmem_cache *s, int node)
 							{ return 0; }
 static inline unsigned long node_nr_slabs(struct kmem_cache_node *n)

Andrey Konovalov

author

2019-02-13 13:25 UTC #9 lore raw

Re: [PATCH 4/5] kasan, slub: move kasan_poison_slab hook before page_address

Quoted 31 lines · Andrew Morton

Quoted 28 lines · On Mon, 11 Feb 2019 22:59:53 +0100 Andrey Konovalov <[email protected]>…
With tag based KASAN page_address() looks at the page flags to see whether the resulting pointer needs to have a tag set. Since we don't want to set a tag when page_address() is called on SLAB pages, we call page_kasan_tag_reset() in kasan_poison_slab(). However in allocate_slab() page_address() is called before kasan_poison_slab(). Fix it by changing the order.
...
mm/slub.c +6 -3
@@ -1642,12 +1642,15 @@ static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)
      if (page_is_pfmemalloc(page))
              SetPageSlabPfmemalloc(page);
 
+     kasan_poison_slab(page);
+
      start = page_address(page);
 
-     if (unlikely(s->flags & SLAB_POISON))
+     if (unlikely(s->flags & SLAB_POISON)) {
+             metadata_access_enable();
              memset(start, POISON_INUSE, PAGE_SIZE << order);
-
-     kasan_poison_slab(page);
+             metadata_access_disable();
+     }
 

Quoted 3 lines

mm/slub.c static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node) · line 1655
Quoted 1 line
mm/slub.c +0 -0
      shuffle = shuffle_freelist(s, page);
This doesn't compile when CONFIG_SLUB_DEBUG=n. Please review carefully:

Sorry, missed this. I think it makes more sense to move this memset into another function CONFIG_SLUB_DEBUG ifdef, since all other poisoning code is also there. I'll send a v2.

Quoted 20 lines

mm/slub.c renamed from mm/slub.c~kasan-slub-move-kasan_poison_slab-hook-before-page_address-fix +8 -0

@@ -1357,6 +1357,14 @@ slab_flags_t kmem_cache_flags(unsigned i
 
 #define disable_higher_order_debug 0
 
+static inline void metadata_access_enable(void)
+{
+}
+
+static inline void metadata_access_disable(void)
+{
+}
+
 static inline unsigned long slabs_node(struct kmem_cache *s, int node)
                                                        { return 0; }
 static inline unsigned long node_nr_slabs(struct kmem_cache_node *n)

Andrey Konovalov

author

2019-02-11 22:00 UTC #10 lore raw

PATCH 5/5 kasan, slub: fix conflicts with CONFIG_SLAB_FREELIST_HARDENED

In reply to Andrey Konovalov

CONFIG_SLAB_FREELIST_HARDENED hashes freelist pointer with the address of the object where the pointer gets stored. With tag based KASAN we don't account for that when building freelist, as we call set_freepointer() with the first argument untagged. This patch changes the code to properly propagate tags throughout the loop.

Reported-by Qian Cai <[email protected]>

Signed-off-by Andrey Konovalov <[email protected]>

---
 mm/slub.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

mm/slub.c +7 ~~-13~~

index ce874a5c9ee7..0d32f8d30752 100644
@@ -303,11 +303,6 @@ static inline void set_freepointer(struct kmem_cache *s, void *object, void *fp)
 		__p < (__addr) + (__objects) * (__s)->size; \
 		__p += (__s)->size)
 
-#define for_each_object_idx(__p, __idx, __s, __addr, __objects) \
-	for (__p = fixup_red_left(__s, __addr), __idx = 1; \
-		__idx <= __objects; \
-		__p += (__s)->size, __idx++)
-
 /* Determine object index from a given position */
 static inline unsigned int slab_index(void *p, struct kmem_cache *s, void *addr)
 {

@@ -1655,17 +1650,16 @@ static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)
 	shuffle = shuffle_freelist(s, page);
 
 	if (!shuffle) {
-		for_each_object_idx(p, idx, s, start, page->objects) {
-			if (likely(idx < page->objects)) {
-				next = p + s->size;
-				next = setup_object(s, page, next);
-				set_freepointer(s, p, next);
-			} else
-				set_freepointer(s, p, NULL);
-		}
 		start = fixup_red_left(s, start);
 		start = setup_object(s, page, start);
 		page->freelist = start;
+		for (idx = 0, p = start; idx < page->objects - 1; idx++) {
+			next = p + s->size;
+			next = setup_object(s, page, next);
+			set_freepointer(s, p, next);
+			p = next;
+		}
+		set_freepointer(s, p, NULL);
 	}
 
 	page->inuse = page->objects;

Signature · 1 line

2.20.1.791.gb4d0f1c61a-goog

Qian Cai

2019-02-12 02:43 UTC #11 lore raw

Re: [PATCH 5/5] kasan, slub: fix conflicts with CONFIG_SLAB_FREELIST_HARDENED

Quoted 44 lines · Andrey Konovalov

Reported-by Qian Cai <[email protected]>

Signed-off-by Andrey Konovalov <[email protected]>

---
 mm/slub.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

mm/slub.c +0 ~~-13~~

index ce874a5c9ee7..0d32f8d30752 100644
@@ -303,11 +303,6 @@ static inline void set_freepointer(struct kmem_cache *s, void *object, void *fp)
 		__p < (__addr) + (__objects) * (__s)->size; \
 		__p += (__s)->size)
 
-#define for_each_object_idx(__p, __idx, __s, __addr, __objects) \
-	for (__p = fixup_red_left(__s, __addr), __idx = 1; \
-		__idx <= __objects; \
-		__p += (__s)->size, __idx++)
-
 /* Determine object index from a given position */
 static inline unsigned int slab_index(void *p, struct kmem_cache *s, void *addr)
 {

@@ -1655,17 +1650,16 @@ static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)
 	shuffle = shuffle_freelist(s, page);
 
 	if (!shuffle) {
-		for_each_object_idx(p, idx, s, start, page->objects) {
-			if (likely(idx < page->objects)) {
-				next = p + s->size;
-				next = setup_object(s, page, next);
-				set_freepointer(s, p, next);
-			} else
-				set_freepointer(s, p, NULL);
-		}
 		start = fixup_red_left(s, start);
 		start = setup_object(s, page, start);
 		page->freelist = start;

mm/slub.c static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node) · line 1665

Quoted 10 lines

mm/slub.c +7 -0

+		for (idx = 0, p = start; idx < page->objects - 1; idx++) {
+			next = p + s->size;
+			next = setup_object(s, page, next);
+			set_freepointer(s, p, next);
+			p = next;
+		}
+		set_freepointer(s, p, NULL);
 	}
 
 	page->inuse = page->objects;

Well, this one patch does not work here, as it throws endless errors below during boot. Still need this patch to fix it.

https://marc.info/?l=linux-mm&m=154955366113951&w=2

[   85.744772] BUG kmemleak_object (Tainted: G    B        L   ): Freepointer
corrupt
[   85.744776]
-----------------------------------------------------------------------------
[   85.744776]
[   85.744788] INFO: Allocated in create_object+0x88/0x9c8 age=2564 cpu=153 pid=1
[   85.744797] 	kmem_cache_alloc+0x39c/0x4ec
[   85.744803] 	create_object+0x88/0x9c8
[   85.744811] 	kmemleak_alloc+0xbc/0x180
[   85.744818] 	kmem_cache_alloc+0x3ec/0x4ec
[   85.744825] 	acpi_ut_create_generic_state+0x64/0xc4
[   85.744832] 	acpi_ut_create_pkg_state+0x24/0x1c8
[   85.744840] 	acpi_ut_walk_package_tree+0x268/0x564
[   85.744848] 	acpi_ns_init_one_package+0x80/0x114
[   85.744856] 	acpi_ns_init_one_object+0x214/0x3d8
[   85.744862] 	acpi_ns_walk_namespace+0x288/0x384
[   85.744869] 	acpi_walk_namespace+0xac/0xe8
[   85.744877] 	acpi_ns_initialize_objects+0x50/0x98
[   85.744883] 	acpi_load_tables+0xac/0x120
[   85.744891] 	acpi_init+0x128/0x850
[   85.744898] 	do_one_initcall+0x3ac/0x8c0
[   85.744906] 	kernel_init_freeable+0xcdc/0x1104
[   85.744916] INFO: Freed in free_object_rcu+0x200/0x228 age=3 cpu=153 pid=0
[   85.744923] 	free_object_rcu+0x200/0x228
[   85.744931] 	rcu_process_callbacks+0xb00/0x12c0
[   85.744937] 	__do_softirq+0x644/0xfd0
[   85.744944] 	irq_exit+0x29c/0x370
[   85.744952] 	__handle_domain_irq+0xe0/0x1c4
[   85.744958] 	gic_handle_irq+0x1c4/0x3b0
[   85.744964] 	el1_irq+0xb0/0x140
[   85.744971] 	arch_cpu_idle+0x26c/0x594
[   85.744978] 	default_idle_call+0x44/0x5c
[   85.744985] 	do_idle+0x180/0x260
[   85.744993] 	cpu_startup_entry+0x24/0x28
[   85.745001] 	secondary_start_kernel+0x36c/0x440
[   85.745009] INFO: Slab 0x(____ptrval____) objects=91 used=0
fp=0x(____ptrval____) flags=0x17ffffffc000200
[   85.745015] INFO: Object 0x(____ptrval____) @offset=35296 fp=0x(____ptrval____)

kkkkk4.226750] Redzone (____ptrval____): bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb bb ................
[ 84.22[ 84.226765] ORedzone (____ptrptrval____): 5a worker/223:0 Tainted: G
B L 5.0.0-rc6+ #36

[   84.226790] Hardware name: HPE Apollo 70             /C01_APACHE_MB         ,
BIOS L50_5.13_1.0.6 07/10/2018
[   84.226798] Workqueue: events free_obj_work
[   84.226802] Call trace:
[   84.226809]  dump_backtrace+0x0/0x450
[   84.226815]  show_stack+0x20/0x2c
[   84.226822]  __dump_stack+0x20/0x28
[   84.226828]  dump_stack+0xa0/0xfc
[   84.226835]  print_trailer+0x1a8/0x1bc
[   84.226842]  object_err+0x40/0x50
[   84.226848]  check_object+0x214/0x2b8
[   84.226854]  __free_slab+0x9c/0x31c
[   84.226860]  discard_slab+0x78/0xa8
[   84.226866]  kmem_cache_free+0x99c/0x9f0
[   84.226873]  free_obj_work+0x92c/0xa44
[   84.226879]  process_one_work+0x894/0x1280
[   84.226885]  worker_thread+0x684/0xa1c
[   84.226892]  kthread+0x2cc/0x2e8
[   84.226898]  ret_from_fork+0x10/0x18
[   84.229197]

Andrey Konovalov

author

2019-02-12 13:27 UTC #12 lore raw

Re: [PATCH 5/5] kasan, slub: fix conflicts with CONFIG_SLAB_FREELIST_HARDENED

Quoted 56 lines · Qian Cai

Quoted 52 lines · Andrey Konovalov

Reported-by Qian Cai <[email protected]>

Signed-off-by Andrey Konovalov <[email protected]>

---
 mm/slub.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

mm/slub.c +7 ~~-13~~

index ce874a5c9ee7..0d32f8d30752 100644
@@ -303,11 +303,6 @@ static inline void set_freepointer(struct kmem_cache *s, void *object, void *fp)
              __p < (__addr) + (__objects) * (__s)->size; \
              __p += (__s)->size)
 
-#define for_each_object_idx(__p, __idx, __s, __addr, __objects) \
-     for (__p = fixup_red_left(__s, __addr), __idx = 1; \
-             __idx <= __objects; \
-             __p += (__s)->size, __idx++)
-
 /* Determine object index from a given position */
 static inline unsigned int slab_index(void *p, struct kmem_cache *s, void *addr)
 {

@@ -1655,17 +1650,16 @@ static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node)
      shuffle = shuffle_freelist(s, page);
 
      if (!shuffle) {
-             for_each_object_idx(p, idx, s, start, page->objects) {
-                     if (likely(idx < page->objects)) {
-                             next = p + s->size;
-                             next = setup_object(s, page, next);
-                             set_freepointer(s, p, next);
-                     } else
-                             set_freepointer(s, p, NULL);
-             }
              start = fixup_red_left(s, start);
              start = setup_object(s, page, start);
              page->freelist = start;
+             for (idx = 0, p = start; idx < page->objects - 1; idx++) {
+                     next = p + s->size;
+                     next = setup_object(s, page, next);
+                     set_freepointer(s, p, next);
+                     p = next;
+             }
+             set_freepointer(s, p, NULL);
      }

Quoted 5 lines

mm/slub.c static struct page *allocate_slab(struct kmem_cache *s, gfp_t flags, int node) · line 1665
Quoted 2 lines
mm/slub.c +0 -0
      page->inuse = page->objects;
Well, this one patch does not work here, as it throws endless errors below during boot. Still need this patch to fix it.

Hm, did you apply all 6 patches (the one that you sent and these five)?

Quoted 66 lines

https://marc.info/?l=linux-mm&m=154955366113951&w=2

[   85.744772] BUG kmemleak_object (Tainted: G    B        L   ): Freepointer
corrupt
[   85.744776]
-----------------------------------------------------------------------------
[   85.744776]
[   85.744788] INFO: Allocated in create_object+0x88/0x9c8 age=2564 cpu=153 pid=1
[   85.744797]  kmem_cache_alloc+0x39c/0x4ec
[   85.744803]  create_object+0x88/0x9c8
[   85.744811]  kmemleak_alloc+0xbc/0x180
[   85.744818]  kmem_cache_alloc+0x3ec/0x4ec
[   85.744825]  acpi_ut_create_generic_state+0x64/0xc4
[   85.744832]  acpi_ut_create_pkg_state+0x24/0x1c8
[   85.744840]  acpi_ut_walk_package_tree+0x268/0x564
[   85.744848]  acpi_ns_init_one_package+0x80/0x114
[   85.744856]  acpi_ns_init_one_object+0x214/0x3d8
[   85.744862]  acpi_ns_walk_namespace+0x288/0x384
[   85.744869]  acpi_walk_namespace+0xac/0xe8
[   85.744877]  acpi_ns_initialize_objects+0x50/0x98
[   85.744883]  acpi_load_tables+0xac/0x120
[   85.744891]  acpi_init+0x128/0x850
[   85.744898]  do_one_initcall+0x3ac/0x8c0
[   85.744906]  kernel_init_freeable+0xcdc/0x1104
[   85.744916] INFO: Freed in free_object_rcu+0x200/0x228 age=3 cpu=153 pid=0
[   85.744923]  free_object_rcu+0x200/0x228
[   85.744931]  rcu_process_callbacks+0xb00/0x12c0
[   85.744937]  __do_softirq+0x644/0xfd0
[   85.744944]  irq_exit+0x29c/0x370
[   85.744952]  __handle_domain_irq+0xe0/0x1c4
[   85.744958]  gic_handle_irq+0x1c4/0x3b0
[   85.744964]  el1_irq+0xb0/0x140
[   85.744971]  arch_cpu_idle+0x26c/0x594
[   85.744978]  default_idle_call+0x44/0x5c
[   85.744985]  do_idle+0x180/0x260
[   85.744993]  cpu_startup_entry+0x24/0x28
[   85.745001]  secondary_start_kernel+0x36c/0x440
[   85.745009] INFO: Slab 0x(____ptrval____) objects=91 used=0
fp=0x(____ptrval____) flags=0x17ffffffc000200
[   85.745015] INFO: Object 0x(____ptrval____) @offset=35296 fp=0x(____ptrval____)

[   84.226790] Hardware name: HPE Apollo 70             /C01_APACHE_MB         ,
BIOS L50_5.13_1.0.6 07/10/2018
[   84.226798] Workqueue: events free_obj_work
[   84.226802] Call trace:
[   84.226809]  dump_backtrace+0x0/0x450
[   84.226815]  show_stack+0x20/0x2c
[   84.226822]  __dump_stack+0x20/0x28
[   84.226828]  dump_stack+0xa0/0xfc
[   84.226835]  print_trailer+0x1a8/0x1bc
[   84.226842]  object_err+0x40/0x50
[   84.226848]  check_object+0x214/0x2b8
[   84.226854]  __free_slab+0x9c/0x31c
[   84.226860]  discard_slab+0x78/0xa8
[   84.226866]  kmem_cache_free+0x99c/0x9f0
[   84.226873]  free_obj_work+0x92c/0xa44
[   84.226879]  process_one_work+0x894/0x1280
[   84.226885]  worker_thread+0x684/0xa1c
[   84.226892]  kthread+0x2cc/0x2e8
[   84.226898]  ret_from_fork+0x10/0x18
[   84.229197]

Qian Cai

2019-02-12 13:43 UTC #13 lore raw

Re: [PATCH 5/5] kasan, slub: fix conflicts with CONFIG_SLAB_FREELIST_HARDENED

Quoted 1 line · Andrey Konovalov

Hm, did you apply all 6 patches (the one that you sent and these five)

Yes.

Andrey Konovalov

author

2019-02-12 14:42 UTC #14 lore raw

Re: [PATCH 5/5] kasan, slub: fix conflicts with CONFIG_SLAB_FREELIST_HARDENED

Quoted 6 lines · Qian Cai

Quoted 1 line · Andrey Konovalov
Hm, did you apply all 6 patches (the one that you sent and these five)
Yes.

I'm failing to reproduce this in QEMU. You're still using the same config, right? Could you share whole dmesg until the first BUG?

Qian Cai

2019-02-12 16:07 UTC #15 lore raw

Re: [PATCH 5/5] kasan, slub: fix conflicts with CONFIG_SLAB_FREELIST_HARDENED

Quoted 11 lines · Andrey Konovalov

Quoted 6 lines · Qian Cai
Quoted 1 line · Andrey Konovalov
Hm, did you apply all 6 patches (the one that you sent and these five)
Yes.
I'm failing to reproduce this in QEMU. You're still using the same config, right? Could you share whole dmesg until the first BUG?

Yes, same config and,

https://git.sr.ht/~cai/linux-debug/tree/master/dmesg

Qian Cai

2019-02-13 02:15 UTC #16 lore raw

Re: [PATCH 5/5] kasan, slub: fix conflicts with CONFIG_SLAB_FREELIST_HARDENED

Quoted 2 lines · Qian Cai

https://git.sr.ht/~cai/linux-debug/tree/master/dmesg

FYI, I just send a patch to take care of this.
https://marc.info/?l=linux-mm&m=155002356527913&w=2