Re: [PATCH RFC 04/10] mm, kfence: insert KFENCE hooks for SLAB

Re: [PATCH RFC 04/10] mm, kfence: insert KFENCE hooks for SLAB

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 7981 bytes --]

CC: kbuild-all(a)lists.01.org
In-Reply-To: <20200907134055.2878499-5-elver@google.com>
References: <20200907134055.2878499-5-elver@google.com>
TO: Marco Elver <elver@google.com>

Hi Marco,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on hnaz-linux-mm/master]
[also build test WARNING on arm64/for-next/core linus/master v5.9-rc4 next-20200903]
[cannot apply to tip/x86/core]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Marco-Elver/KFENCE-A-low-overhead-sampling-based-memory-safety-error-detector/20200907-214229
base:   https://github.com/hnaz/linux-mm master
:::::: branch date: 12 hours ago
:::::: commit date: 12 hours ago
compiler: powerpc-linux-gcc (GCC) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


cppcheck warnings: (new ones prefixed by >>)

   mm/slab.c:906:6: warning: Variable 'ret' is reassigned a value before the old one has been used. [redundantAssignment]
    ret = init_cache_node(cachep, node, gfp);
        ^
   mm/slab.c:886:0: note: Variable 'ret' is reassigned a value before the old one has been used.
    int ret = -ENOMEM;
   ^
   mm/slab.c:906:6: note: Variable 'ret' is reassigned a value before the old one has been used.
    ret = init_cache_node(cachep, node, gfp);
        ^
   mm/slab.c:3316:7: warning: Redundant assignment of 'objp' to itself. [selfAssignment]
    objp = cache_alloc_debugcheck_after(cachep, flags, objp, caller);
         ^
   mm/slab.c:3454:7: warning: Redundant assignment of 'objp' to itself. [selfAssignment]
    objp = cache_free_debugcheck(cachep, objp, caller);
         ^
   mm/slab.c:3514:8: warning: Redundant assignment of 'p[i]' to itself. [selfAssignment]
     p[i] = cache_alloc_debugcheck_after(s, flags, p[i], caller);
          ^
   mm/slab.c:3342:16: warning: Local variable page shadows outer variable [shadowVar]
     struct page *page;
                  ^
   mm/slab.c:3336:15: note: Shadowed declaration
    struct page *page;
                 ^
   mm/slab.c:3342:16: note: Shadow variable
     struct page *page;
                  ^
>> mm/slab.c:3872:12: warning: Same expression on both sides of '&&' because 'limit' and 'shared' represent the same value. [knownConditionTrueFalse]
    if (limit && shared && batchcount)
              ^
   mm/slab.c:3864:14: note: 'limit' is assigned value '0' here.
    int limit = 0;
                ^
   mm/slab.c:3865:15: note: 'shared' is assigned value '0' here.
    int shared = 0;
                 ^
   mm/slab.c:3872:12: note: Same expression on both sides of '&&' because 'limit' and 'shared' represent the same value.
    if (limit && shared && batchcount)
              ^
>> mm/slab.c:3872:22: warning: Same expression on both sides of '&&' because 'batchcount' and 'shared' represent the same value. [knownConditionTrueFalse]
    if (limit && shared && batchcount)
                        ^
   mm/slab.c:3866:19: note: 'batchcount' is assigned value '0' here.
    int batchcount = 0;
                     ^
   mm/slab.c:3865:15: note: 'shared' is assigned value '0' here.
    int shared = 0;
                 ^
   mm/slab.c:3872:22: note: Same expression on both sides of '&&' because 'batchcount' and 'shared' represent the same value.
    if (limit && shared && batchcount)
                        ^

# https://github.com/0day-ci/linux/commit/62475f64212ac70b313ca25aeaf677151914ac00
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Marco-Elver/KFENCE-A-low-overhead-sampling-based-memory-safety-error-detector/20200907-214229
git checkout 62475f64212ac70b313ca25aeaf677151914ac00
vim +3872 mm/slab.c

^1da177e4c3f41 Linus Torvalds    2005-04-16  3859  
18004c5d4084d9 Christoph Lameter 2012-07-06  3860  /* Called with slab_mutex held always */
83b519e8b9572c Pekka Enberg      2009-06-10  3861  static int enable_cpucache(struct kmem_cache *cachep, gfp_t gfp)
^1da177e4c3f41 Linus Torvalds    2005-04-16  3862  {
^1da177e4c3f41 Linus Torvalds    2005-04-16  3863  	int err;
943a451a87d229 Glauber Costa     2012-12-18  3864  	int limit = 0;
943a451a87d229 Glauber Costa     2012-12-18  3865  	int shared = 0;
943a451a87d229 Glauber Costa     2012-12-18  3866  	int batchcount = 0;
943a451a87d229 Glauber Costa     2012-12-18  3867  
7c00fce98c3e15 Thomas Garnier    2016-07-26  3868  	err = cache_random_seq_create(cachep, cachep->num, gfp);
c7ce4f60ac199f Thomas Garnier    2016-05-19  3869  	if (err)
c7ce4f60ac199f Thomas Garnier    2016-05-19  3870  		goto end;
c7ce4f60ac199f Thomas Garnier    2016-05-19  3871  
943a451a87d229 Glauber Costa     2012-12-18 @3872  	if (limit && shared && batchcount)
943a451a87d229 Glauber Costa     2012-12-18  3873  		goto skip_setup;
a737b3e2fcf96f Andrew Morton     2006-03-22  3874  	/*
a737b3e2fcf96f Andrew Morton     2006-03-22  3875  	 * The head array serves three purposes:
^1da177e4c3f41 Linus Torvalds    2005-04-16  3876  	 * - create a LIFO ordering, i.e. return objects that are cache-warm
^1da177e4c3f41 Linus Torvalds    2005-04-16  3877  	 * - reduce the number of spinlock operations.
^1da177e4c3f41 Linus Torvalds    2005-04-16  3878  	 * - reduce the number of linked list operations on the slab and
^1da177e4c3f41 Linus Torvalds    2005-04-16  3879  	 *   bufctl chains: array operations are cheaper.
^1da177e4c3f41 Linus Torvalds    2005-04-16  3880  	 * The numbers are guessed, we should auto-tune as described by
^1da177e4c3f41 Linus Torvalds    2005-04-16  3881  	 * Bonwick.
^1da177e4c3f41 Linus Torvalds    2005-04-16  3882  	 */
3b0efdfa1e7193 Christoph Lameter 2012-06-13  3883  	if (cachep->size > 131072)
^1da177e4c3f41 Linus Torvalds    2005-04-16  3884  		limit = 1;
3b0efdfa1e7193 Christoph Lameter 2012-06-13  3885  	else if (cachep->size > PAGE_SIZE)
^1da177e4c3f41 Linus Torvalds    2005-04-16  3886  		limit = 8;
3b0efdfa1e7193 Christoph Lameter 2012-06-13  3887  	else if (cachep->size > 1024)
^1da177e4c3f41 Linus Torvalds    2005-04-16  3888  		limit = 24;
3b0efdfa1e7193 Christoph Lameter 2012-06-13  3889  	else if (cachep->size > 256)
^1da177e4c3f41 Linus Torvalds    2005-04-16  3890  		limit = 54;
^1da177e4c3f41 Linus Torvalds    2005-04-16  3891  	else
^1da177e4c3f41 Linus Torvalds    2005-04-16  3892  		limit = 120;
^1da177e4c3f41 Linus Torvalds    2005-04-16  3893  
a737b3e2fcf96f Andrew Morton     2006-03-22  3894  	/*
a737b3e2fcf96f Andrew Morton     2006-03-22  3895  	 * CPU bound tasks (e.g. network routing) can exhibit cpu bound
^1da177e4c3f41 Linus Torvalds    2005-04-16  3896  	 * allocation behaviour: Most allocs on one cpu, most free operations
^1da177e4c3f41 Linus Torvalds    2005-04-16  3897  	 * on another cpu. For these cases, an efficient object passing between
^1da177e4c3f41 Linus Torvalds    2005-04-16  3898  	 * cpus is necessary. This is provided by a shared array. The array
^1da177e4c3f41 Linus Torvalds    2005-04-16  3899  	 * replaces Bonwick's magazine layer.
^1da177e4c3f41 Linus Torvalds    2005-04-16  3900  	 * On uniprocessor, it's functionally equivalent (but less efficient)
^1da177e4c3f41 Linus Torvalds    2005-04-16  3901  	 * to a larger limit. Thus disabled by default.
^1da177e4c3f41 Linus Torvalds    2005-04-16  3902  	 */
^1da177e4c3f41 Linus Torvalds    2005-04-16  3903  	shared = 0;
3b0efdfa1e7193 Christoph Lameter 2012-06-13  3904  	if (cachep->size <= PAGE_SIZE && num_possible_cpus() > 1)
^1da177e4c3f41 Linus Torvalds    2005-04-16  3905  		shared = 8;
^1da177e4c3f41 Linus Torvalds    2005-04-16  3906  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[PATCH RFC 00/10] KFENCE: A low-overhead sampling-based memory safety error detector

[Marco Elver]

This adds the Kernel Electric-Fence (KFENCE) infrastructure. KFENCE is a
low-overhead sampling-based memory safety error detector of heap
use-after-free, invalid-free, and out-of-bounds access errors.  This
series enables KFENCE for the x86 and arm64 architectures, and adds
KFENCE hooks to the SLAB and SLUB allocators.

KFENCE is designed to be enabled in production kernels, and has near
zero performance overhead. Compared to KASAN, KFENCE trades performance
for precision. The main motivation behind KFENCE's design, is that with
enough total uptime KFENCE will detect bugs in code paths not typically
exercised by non-production test workloads. One way to quickly achieve a
large enough total uptime is when the tool is deployed across a large
fleet of machines.

KFENCE objects each reside on a dedicated page, at either the left or
right page boundaries. The pages to the left and right of the object
page are "guard pages", whose attributes are changed to a protected
state, and cause page faults on any attempted access to them. Such page
faults are then intercepted by KFENCE, which handles the fault
gracefully by reporting a memory access error.

Guarded allocations are set up based on a sample interval (can be set
via kfence.sample_interval). After expiration of the sample interval, a
guarded allocation from the KFENCE object pool is returned to the main
allocator (SLAB or SLUB). At this point, the timer is reset, and the
next allocation is set up after the expiration of the interval.

To enable/disable a KFENCE allocation through the main allocator's
fast-path without overhead, KFENCE relies on static branches via the
static keys infrastructure. The static branch is toggled to redirect the
allocation to KFENCE.

We have verified by running synthetic benchmarks (sysbench I/O,
hackbench) that a kernel with KFENCE is performance-neutral compared to
a non-KFENCE baseline kernel.

KFENCE is inspired by GWP-ASan [1], a userspace tool with similar
properties. The name "KFENCE" is a homage to the Electric Fence Malloc
Debugger [2].

For more details, see Documentation/dev-tools/kfence.rst added in the
series -- also viewable here:

	https://raw.githubusercontent.com/google/kasan/kfence/Documentation/dev-tools/kfence.rst

[1] http://llvm.org/docs/GwpAsan.html
[2] https://linux.die.net/man/3/efence

Alexander Potapenko (6):
  mm: add Kernel Electric-Fence infrastructure
  x86, kfence: enable KFENCE for x86
  mm, kfence: insert KFENCE hooks for SLAB
  mm, kfence: insert KFENCE hooks for SLUB
  kfence, kasan: make KFENCE compatible with KASAN
  kfence, kmemleak: make KFENCE compatible with KMEMLEAK

Marco Elver (4):
  arm64, kfence: enable KFENCE for ARM64
  kfence, lockdep: make KFENCE compatible with lockdep
  kfence, Documentation: add KFENCE documentation
  kfence: add test suite

 Documentation/dev-tools/index.rst  |   1 +
 Documentation/dev-tools/kfence.rst | 285 +++++++++++
 MAINTAINERS                        |  11 +
 arch/arm64/Kconfig                 |   1 +
 arch/arm64/include/asm/kfence.h    |  39 ++
 arch/arm64/mm/fault.c              |   4 +
 arch/x86/Kconfig                   |   2 +
 arch/x86/include/asm/kfence.h      |  60 +++
 arch/x86/mm/fault.c                |   4 +
 include/linux/kfence.h             | 174 +++++++
 init/main.c                        |   2 +
 kernel/locking/lockdep.c           |   8 +
 lib/Kconfig.debug                  |   1 +
 lib/Kconfig.kfence                 |  70 +++
 mm/Makefile                        |   1 +
 mm/kasan/common.c                  |   7 +
 mm/kfence/Makefile                 |   6 +
 mm/kfence/core.c                   | 730 +++++++++++++++++++++++++++
 mm/kfence/kfence-test.c            | 777 +++++++++++++++++++++++++++++
 mm/kfence/kfence.h                 | 104 ++++
 mm/kfence/report.c                 | 201 ++++++++
 mm/kmemleak.c                      |  11 +
 mm/slab.c                          |  46 +-
 mm/slab_common.c                   |   6 +-
 mm/slub.c                          |  72 ++-
 25 files changed, 2591 insertions(+), 32 deletions(-)
 create mode 100644 Documentation/dev-tools/kfence.rst
 create mode 100644 arch/arm64/include/asm/kfence.h
 create mode 100644 arch/x86/include/asm/kfence.h
 create mode 100644 include/linux/kfence.h
 create mode 100644 lib/Kconfig.kfence
 create mode 100644 mm/kfence/Makefile
 create mode 100644 mm/kfence/core.c
 create mode 100644 mm/kfence/kfence-test.c
 create mode 100644 mm/kfence/kfence.h
 create mode 100644 mm/kfence/report.c

-- 
2.28.0.526.ge36021eeef-goog

[PATCH RFC 04/10] mm, kfence: insert KFENCE hooks for SLAB

[Marco Elver]

From: Alexander Potapenko <glider@google.com>

Inserts KFENCE hooks into the SLAB allocator.

We note the addition of the 'orig_size' argument to slab_alloc*()
functions, to be able to pass the originally requested size to KFENCE.
When KFENCE is disabled, there is no additional overhead, since these
functions are __always_inline.

Co-developed-by: Marco Elver <elver@google.com>
Signed-off-by: Marco Elver <elver@google.com>
Signed-off-by: Alexander Potapenko <glider@google.com>
---
 mm/slab.c        | 46 ++++++++++++++++++++++++++++++++++------------
 mm/slab_common.c |  6 +++++-
 2 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index 3160dff6fd76..30aba06ae02b 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -100,6 +100,7 @@
 #include	<linux/seq_file.h>
 #include	<linux/notifier.h>
 #include	<linux/kallsyms.h>
+#include	<linux/kfence.h>
 #include	<linux/cpu.h>
 #include	<linux/sysctl.h>
 #include	<linux/module.h>
@@ -3206,7 +3207,7 @@ static void *____cache_alloc_node(struct kmem_cache *cachep, gfp_t flags,
 }
 
 static __always_inline void *
-slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
+slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid, size_t orig_size,
 		   unsigned long caller)
 {
 	unsigned long save_flags;
@@ -3219,6 +3220,10 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
 	if (unlikely(!cachep))
 		return NULL;
 
+	ptr = kfence_alloc(cachep, orig_size, flags);
+	if (unlikely(ptr))
+		goto out_hooks;
+
 	cache_alloc_debugcheck_before(cachep, flags);
 	local_irq_save(save_flags);
 
@@ -3251,6 +3256,7 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
 	if (unlikely(slab_want_init_on_alloc(flags, cachep)) && ptr)
 		memset(ptr, 0, cachep->object_size);
 
+out_hooks:
 	slab_post_alloc_hook(cachep, objcg, flags, 1, &ptr);
 	return ptr;
 }
@@ -3288,7 +3294,7 @@ __do_cache_alloc(struct kmem_cache *cachep, gfp_t flags)
 #endif /* CONFIG_NUMA */
 
 static __always_inline void *
-slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
+slab_alloc(struct kmem_cache *cachep, gfp_t flags, size_t orig_size, unsigned long caller)
 {
 	unsigned long save_flags;
 	void *objp;
@@ -3299,6 +3305,10 @@ slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
 	if (unlikely(!cachep))
 		return NULL;
 
+	objp = kfence_alloc(cachep, orig_size, flags);
+	if (unlikely(objp))
+		goto leave;
+
 	cache_alloc_debugcheck_before(cachep, flags);
 	local_irq_save(save_flags);
 	objp = __do_cache_alloc(cachep, flags);
@@ -3309,6 +3319,7 @@ slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
 	if (unlikely(slab_want_init_on_alloc(flags, cachep)) && objp)
 		memset(objp, 0, cachep->object_size);
 
+leave:
 	slab_post_alloc_hook(cachep, objcg, flags, 1, &objp);
 	return objp;
 }
@@ -3414,6 +3425,11 @@ static void cache_flusharray(struct kmem_cache *cachep, struct array_cache *ac)
 static __always_inline void __cache_free(struct kmem_cache *cachep, void *objp,
 					 unsigned long caller)
 {
+	if (kfence_free(objp)) {
+		kmemleak_free_recursive(objp, cachep->flags);
+		return;
+	}
+
 	/* Put the object into the quarantine, don't touch it for now. */
 	if (kasan_slab_free(cachep, objp, _RET_IP_))
 		return;
@@ -3479,7 +3495,7 @@ void ___cache_free(struct kmem_cache *cachep, void *objp,
  */
 void *kmem_cache_alloc(struct kmem_cache *cachep, gfp_t flags)
 {
-	void *ret = slab_alloc(cachep, flags, _RET_IP_);
+	void *ret = slab_alloc(cachep, flags, cachep->object_size, _RET_IP_);
 
 	trace_kmem_cache_alloc(_RET_IP_, ret,
 			       cachep->object_size, cachep->size, flags);
@@ -3512,7 +3528,7 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
 
 	local_irq_disable();
 	for (i = 0; i < size; i++) {
-		void *objp = __do_cache_alloc(s, flags);
+		void *objp = kfence_alloc(s, s->object_size, flags) ?: __do_cache_alloc(s, flags);
 
 		if (unlikely(!objp))
 			goto error;
@@ -3545,7 +3561,7 @@ kmem_cache_alloc_trace(struct kmem_cache *cachep, gfp_t flags, size_t size)
 {
 	void *ret;
 
-	ret = slab_alloc(cachep, flags, _RET_IP_);
+	ret = slab_alloc(cachep, flags, size, _RET_IP_);
 
 	ret = kasan_kmalloc(cachep, ret, size, flags);
 	trace_kmalloc(_RET_IP_, ret,
@@ -3571,7 +3587,7 @@ EXPORT_SYMBOL(kmem_cache_alloc_trace);
  */
 void *kmem_cache_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid)
 {
-	void *ret = slab_alloc_node(cachep, flags, nodeid, _RET_IP_);
+	void *ret = slab_alloc_node(cachep, flags, nodeid, cachep->object_size, _RET_IP_);
 
 	trace_kmem_cache_alloc_node(_RET_IP_, ret,
 				    cachep->object_size, cachep->size,
@@ -3589,7 +3605,7 @@ void *kmem_cache_alloc_node_trace(struct kmem_cache *cachep,
 {
 	void *ret;
 
-	ret = slab_alloc_node(cachep, flags, nodeid, _RET_IP_);
+	ret = slab_alloc_node(cachep, flags, nodeid, size, _RET_IP_);
 
 	ret = kasan_kmalloc(cachep, ret, size, flags);
 	trace_kmalloc_node(_RET_IP_, ret,
@@ -3650,7 +3666,7 @@ static __always_inline void *__do_kmalloc(size_t size, gfp_t flags,
 	cachep = kmalloc_slab(size, flags);
 	if (unlikely(ZERO_OR_NULL_PTR(cachep)))
 		return cachep;
-	ret = slab_alloc(cachep, flags, caller);
+	ret = slab_alloc(cachep, flags, size, caller);
 
 	ret = kasan_kmalloc(cachep, ret, size, flags);
 	trace_kmalloc(caller, ret,
@@ -4138,18 +4154,24 @@ void __check_heap_object(const void *ptr, unsigned long n, struct page *page,
 			 bool to_user)
 {
 	struct kmem_cache *cachep;
-	unsigned int objnr;
+	unsigned int objnr = 0;
 	unsigned long offset;
+	bool is_kfence = is_kfence_address(ptr);
 
 	ptr = kasan_reset_tag(ptr);
 
 	/* Find and validate object. */
 	cachep = page->slab_cache;
-	objnr = obj_to_index(cachep, page, (void *)ptr);
-	BUG_ON(objnr >= cachep->num);
+	if (!is_kfence) {
+		objnr = obj_to_index(cachep, page, (void *)ptr);
+		BUG_ON(objnr >= cachep->num);
+	}
 
 	/* Find offset within object. */
-	offset = ptr - index_to_obj(cachep, page, objnr) - obj_offset(cachep);
+	if (is_kfence_address(ptr))
+		offset = ptr - kfence_object_start(ptr);
+	else
+		offset = ptr - index_to_obj(cachep, page, objnr) - obj_offset(cachep);
 
 	/* Allow address range falling entirely within usercopy region. */
 	if (offset >= cachep->useroffset &&
diff --git a/mm/slab_common.c b/mm/slab_common.c
index f9ccd5dc13f3..6e35e273681a 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -12,6 +12,7 @@
 #include <linux/memory.h>
 #include <linux/cache.h>
 #include <linux/compiler.h>
+#include <linux/kfence.h>
 #include <linux/module.h>
 #include <linux/cpu.h>
 #include <linux/uaccess.h>
@@ -448,6 +449,9 @@ static int shutdown_cache(struct kmem_cache *s)
 	/* free asan quarantined objects */
 	kasan_cache_shutdown(s);
 
+	if (!kfence_shutdown_cache(s))
+		return -EBUSY;
+
 	if (__kmem_cache_shutdown(s) != 0)
 		return -EBUSY;
 
@@ -1171,7 +1175,7 @@ size_t ksize(const void *objp)
 	if (unlikely(ZERO_OR_NULL_PTR(objp)) || !__kasan_check_read(objp, 1))
 		return 0;
 
-	size = __ksize(objp);
+	size = kfence_ksize(objp) ?: __ksize(objp);
 	/*
 	 * We assume that ksize callers could use whole allocated area,
 	 * so we need to unpoison this area.
-- 
2.28.0.526.ge36021eeef-goog

[PATCH RFC 04/10] mm, kfence: insert KFENCE hooks for SLAB

[Marco Elver]

From: Alexander Potapenko <glider@google.com>

Inserts KFENCE hooks into the SLAB allocator.

We note the addition of the 'orig_size' argument to slab_alloc*()
functions, to be able to pass the originally requested size to KFENCE.
When KFENCE is disabled, there is no additional overhead, since these
functions are __always_inline.

Co-developed-by: Marco Elver <elver@google.com>
Signed-off-by: Marco Elver <elver@google.com>
Signed-off-by: Alexander Potapenko <glider@google.com>
---
 mm/slab.c        | 46 ++++++++++++++++++++++++++++++++++------------
 mm/slab_common.c |  6 +++++-
 2 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index 3160dff6fd76..30aba06ae02b 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -100,6 +100,7 @@
 #include	<linux/seq_file.h>
 #include	<linux/notifier.h>
 #include	<linux/kallsyms.h>
+#include	<linux/kfence.h>
 #include	<linux/cpu.h>
 #include	<linux/sysctl.h>
 #include	<linux/module.h>
@@ -3206,7 +3207,7 @@ static void *____cache_alloc_node(struct kmem_cache *cachep, gfp_t flags,
 }
 
 static __always_inline void *
-slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
+slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid, size_t orig_size,
 		   unsigned long caller)
 {
 	unsigned long save_flags;
@@ -3219,6 +3220,10 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
 	if (unlikely(!cachep))
 		return NULL;
 
+	ptr = kfence_alloc(cachep, orig_size, flags);
+	if (unlikely(ptr))
+		goto out_hooks;
+
 	cache_alloc_debugcheck_before(cachep, flags);
 	local_irq_save(save_flags);
 
@@ -3251,6 +3256,7 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
 	if (unlikely(slab_want_init_on_alloc(flags, cachep)) && ptr)
 		memset(ptr, 0, cachep->object_size);
 
+out_hooks:
 	slab_post_alloc_hook(cachep, objcg, flags, 1, &ptr);
 	return ptr;
 }
@@ -3288,7 +3294,7 @@ __do_cache_alloc(struct kmem_cache *cachep, gfp_t flags)
 #endif /* CONFIG_NUMA */
 
 static __always_inline void *
-slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
+slab_alloc(struct kmem_cache *cachep, gfp_t flags, size_t orig_size, unsigned long caller)
 {
 	unsigned long save_flags;
 	void *objp;
@@ -3299,6 +3305,10 @@ slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
 	if (unlikely(!cachep))
 		return NULL;
 
+	objp = kfence_alloc(cachep, orig_size, flags);
+	if (unlikely(objp))
+		goto leave;
+
 	cache_alloc_debugcheck_before(cachep, flags);
 	local_irq_save(save_flags);
 	objp = __do_cache_alloc(cachep, flags);
@@ -3309,6 +3319,7 @@ slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
 	if (unlikely(slab_want_init_on_alloc(flags, cachep)) && objp)
 		memset(objp, 0, cachep->object_size);
 
+leave:
 	slab_post_alloc_hook(cachep, objcg, flags, 1, &objp);
 	return objp;
 }
@@ -3414,6 +3425,11 @@ static void cache_flusharray(struct kmem_cache *cachep, struct array_cache *ac)
 static __always_inline void __cache_free(struct kmem_cache *cachep, void *objp,
 					 unsigned long caller)
 {
+	if (kfence_free(objp)) {
+		kmemleak_free_recursive(objp, cachep->flags);
+		return;
+	}
+
 	/* Put the object into the quarantine, don't touch it for now. */
 	if (kasan_slab_free(cachep, objp, _RET_IP_))
 		return;
@@ -3479,7 +3495,7 @@ void ___cache_free(struct kmem_cache *cachep, void *objp,
  */
 void *kmem_cache_alloc(struct kmem_cache *cachep, gfp_t flags)
 {
-	void *ret = slab_alloc(cachep, flags, _RET_IP_);
+	void *ret = slab_alloc(cachep, flags, cachep->object_size, _RET_IP_);
 
 	trace_kmem_cache_alloc(_RET_IP_, ret,
 			       cachep->object_size, cachep->size, flags);
@@ -3512,7 +3528,7 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
 
 	local_irq_disable();
 	for (i = 0; i < size; i++) {
-		void *objp = __do_cache_alloc(s, flags);
+		void *objp = kfence_alloc(s, s->object_size, flags) ?: __do_cache_alloc(s, flags);
 
 		if (unlikely(!objp))
 			goto error;
@@ -3545,7 +3561,7 @@ kmem_cache_alloc_trace(struct kmem_cache *cachep, gfp_t flags, size_t size)
 {
 	void *ret;
 
-	ret = slab_alloc(cachep, flags, _RET_IP_);
+	ret = slab_alloc(cachep, flags, size, _RET_IP_);
 
 	ret = kasan_kmalloc(cachep, ret, size, flags);
 	trace_kmalloc(_RET_IP_, ret,
@@ -3571,7 +3587,7 @@ EXPORT_SYMBOL(kmem_cache_alloc_trace);
  */
 void *kmem_cache_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid)
 {
-	void *ret = slab_alloc_node(cachep, flags, nodeid, _RET_IP_);
+	void *ret = slab_alloc_node(cachep, flags, nodeid, cachep->object_size, _RET_IP_);
 
 	trace_kmem_cache_alloc_node(_RET_IP_, ret,
 				    cachep->object_size, cachep->size,
@@ -3589,7 +3605,7 @@ void *kmem_cache_alloc_node_trace(struct kmem_cache *cachep,
 {
 	void *ret;
 
-	ret = slab_alloc_node(cachep, flags, nodeid, _RET_IP_);
+	ret = slab_alloc_node(cachep, flags, nodeid, size, _RET_IP_);
 
 	ret = kasan_kmalloc(cachep, ret, size, flags);
 	trace_kmalloc_node(_RET_IP_, ret,
@@ -3650,7 +3666,7 @@ static __always_inline void *__do_kmalloc(size_t size, gfp_t flags,
 	cachep = kmalloc_slab(size, flags);
 	if (unlikely(ZERO_OR_NULL_PTR(cachep)))
 		return cachep;
-	ret = slab_alloc(cachep, flags, caller);
+	ret = slab_alloc(cachep, flags, size, caller);
 
 	ret = kasan_kmalloc(cachep, ret, size, flags);
 	trace_kmalloc(caller, ret,
@@ -4138,18 +4154,24 @@ void __check_heap_object(const void *ptr, unsigned long n, struct page *page,
 			 bool to_user)
 {
 	struct kmem_cache *cachep;
-	unsigned int objnr;
+	unsigned int objnr = 0;
 	unsigned long offset;
+	bool is_kfence = is_kfence_address(ptr);
 
 	ptr = kasan_reset_tag(ptr);
 
 	/* Find and validate object. */
 	cachep = page->slab_cache;
-	objnr = obj_to_index(cachep, page, (void *)ptr);
-	BUG_ON(objnr >= cachep->num);
+	if (!is_kfence) {
+		objnr = obj_to_index(cachep, page, (void *)ptr);
+		BUG_ON(objnr >= cachep->num);
+	}
 
 	/* Find offset within object. */
-	offset = ptr - index_to_obj(cachep, page, objnr) - obj_offset(cachep);
+	if (is_kfence_address(ptr))
+		offset = ptr - kfence_object_start(ptr);
+	else
+		offset = ptr - index_to_obj(cachep, page, objnr) - obj_offset(cachep);
 
 	/* Allow address range falling entirely within usercopy region. */
 	if (offset >= cachep->useroffset &&
diff --git a/mm/slab_common.c b/mm/slab_common.c
index f9ccd5dc13f3..6e35e273681a 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -12,6 +12,7 @@
 #include <linux/memory.h>
 #include <linux/cache.h>
 #include <linux/compiler.h>
+#include <linux/kfence.h>
 #include <linux/module.h>
 #include <linux/cpu.h>
 #include <linux/uaccess.h>
@@ -448,6 +449,9 @@ static int shutdown_cache(struct kmem_cache *s)
 	/* free asan quarantined objects */
 	kasan_cache_shutdown(s);
 
+	if (!kfence_shutdown_cache(s))
+		return -EBUSY;
+
 	if (__kmem_cache_shutdown(s) != 0)
 		return -EBUSY;
 
@@ -1171,7 +1175,7 @@ size_t ksize(const void *objp)
 	if (unlikely(ZERO_OR_NULL_PTR(objp)) || !__kasan_check_read(objp, 1))
 		return 0;
 
-	size = __ksize(objp);
+	size = kfence_ksize(objp) ?: __ksize(objp);
 	/*
 	 * We assume that ksize callers could use whole allocated area,
 	 * so we need to unpoison this area.
-- 
2.28.0.526.ge36021eeef-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH RFC 04/10] mm, kfence: insert KFENCE hooks for SLAB

[Marco Elver]

From: Alexander Potapenko <glider@google.com>

Inserts KFENCE hooks into the SLAB allocator.

We note the addition of the 'orig_size' argument to slab_alloc*()
functions, to be able to pass the originally requested size to KFENCE.
When KFENCE is disabled, there is no additional overhead, since these
functions are __always_inline.

Co-developed-by: Marco Elver <elver@google.com>
Signed-off-by: Marco Elver <elver@google.com>
Signed-off-by: Alexander Potapenko <glider@google.com>
---
 mm/slab.c        | 46 ++++++++++++++++++++++++++++++++++------------
 mm/slab_common.c |  6 +++++-
 2 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index 3160dff6fd76..30aba06ae02b 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -100,6 +100,7 @@
 #include	<linux/seq_file.h>
 #include	<linux/notifier.h>
 #include	<linux/kallsyms.h>
+#include	<linux/kfence.h>
 #include	<linux/cpu.h>
 #include	<linux/sysctl.h>
 #include	<linux/module.h>
@@ -3206,7 +3207,7 @@ static void *____cache_alloc_node(struct kmem_cache *cachep, gfp_t flags,
 }
 
 static __always_inline void *
-slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
+slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid, size_t orig_size,
 		   unsigned long caller)
 {
 	unsigned long save_flags;
@@ -3219,6 +3220,10 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
 	if (unlikely(!cachep))
 		return NULL;
 
+	ptr = kfence_alloc(cachep, orig_size, flags);
+	if (unlikely(ptr))
+		goto out_hooks;
+
 	cache_alloc_debugcheck_before(cachep, flags);
 	local_irq_save(save_flags);
 
@@ -3251,6 +3256,7 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
 	if (unlikely(slab_want_init_on_alloc(flags, cachep)) && ptr)
 		memset(ptr, 0, cachep->object_size);
 
+out_hooks:
 	slab_post_alloc_hook(cachep, objcg, flags, 1, &ptr);
 	return ptr;
 }
@@ -3288,7 +3294,7 @@ __do_cache_alloc(struct kmem_cache *cachep, gfp_t flags)
 #endif /* CONFIG_NUMA */
 
 static __always_inline void *
-slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
+slab_alloc(struct kmem_cache *cachep, gfp_t flags, size_t orig_size, unsigned long caller)
 {
 	unsigned long save_flags;
 	void *objp;
@@ -3299,6 +3305,10 @@ slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
 	if (unlikely(!cachep))
 		return NULL;
 
+	objp = kfence_alloc(cachep, orig_size, flags);
+	if (unlikely(objp))
+		goto leave;
+
 	cache_alloc_debugcheck_before(cachep, flags);
 	local_irq_save(save_flags);
 	objp = __do_cache_alloc(cachep, flags);
@@ -3309,6 +3319,7 @@ slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
 	if (unlikely(slab_want_init_on_alloc(flags, cachep)) && objp)
 		memset(objp, 0, cachep->object_size);
 
+leave:
 	slab_post_alloc_hook(cachep, objcg, flags, 1, &objp);
 	return objp;
 }
@@ -3414,6 +3425,11 @@ static void cache_flusharray(struct kmem_cache *cachep, struct array_cache *ac)
 static __always_inline void __cache_free(struct kmem_cache *cachep, void *objp,
 					 unsigned long caller)
 {
+	if (kfence_free(objp)) {
+		kmemleak_free_recursive(objp, cachep->flags);
+		return;
+	}
+
 	/* Put the object into the quarantine, don't touch it for now. */
 	if (kasan_slab_free(cachep, objp, _RET_IP_))
 		return;
@@ -3479,7 +3495,7 @@ void ___cache_free(struct kmem_cache *cachep, void *objp,
  */
 void *kmem_cache_alloc(struct kmem_cache *cachep, gfp_t flags)
 {
-	void *ret = slab_alloc(cachep, flags, _RET_IP_);
+	void *ret = slab_alloc(cachep, flags, cachep->object_size, _RET_IP_);
 
 	trace_kmem_cache_alloc(_RET_IP_, ret,
 			       cachep->object_size, cachep->size, flags);
@@ -3512,7 +3528,7 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
 
 	local_irq_disable();
 	for (i = 0; i < size; i++) {
-		void *objp = __do_cache_alloc(s, flags);
+		void *objp = kfence_alloc(s, s->object_size, flags) ?: __do_cache_alloc(s, flags);
 
 		if (unlikely(!objp))
 			goto error;
@@ -3545,7 +3561,7 @@ kmem_cache_alloc_trace(struct kmem_cache *cachep, gfp_t flags, size_t size)
 {
 	void *ret;
 
-	ret = slab_alloc(cachep, flags, _RET_IP_);
+	ret = slab_alloc(cachep, flags, size, _RET_IP_);
 
 	ret = kasan_kmalloc(cachep, ret, size, flags);
 	trace_kmalloc(_RET_IP_, ret,
@@ -3571,7 +3587,7 @@ EXPORT_SYMBOL(kmem_cache_alloc_trace);
  */
 void *kmem_cache_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid)
 {
-	void *ret = slab_alloc_node(cachep, flags, nodeid, _RET_IP_);
+	void *ret = slab_alloc_node(cachep, flags, nodeid, cachep->object_size, _RET_IP_);
 
 	trace_kmem_cache_alloc_node(_RET_IP_, ret,
 				    cachep->object_size, cachep->size,
@@ -3589,7 +3605,7 @@ void *kmem_cache_alloc_node_trace(struct kmem_cache *cachep,
 {
 	void *ret;
 
-	ret = slab_alloc_node(cachep, flags, nodeid, _RET_IP_);
+	ret = slab_alloc_node(cachep, flags, nodeid, size, _RET_IP_);
 
 	ret = kasan_kmalloc(cachep, ret, size, flags);
 	trace_kmalloc_node(_RET_IP_, ret,
@@ -3650,7 +3666,7 @@ static __always_inline void *__do_kmalloc(size_t size, gfp_t flags,
 	cachep = kmalloc_slab(size, flags);
 	if (unlikely(ZERO_OR_NULL_PTR(cachep)))
 		return cachep;
-	ret = slab_alloc(cachep, flags, caller);
+	ret = slab_alloc(cachep, flags, size, caller);
 
 	ret = kasan_kmalloc(cachep, ret, size, flags);
 	trace_kmalloc(caller, ret,
@@ -4138,18 +4154,24 @@ void __check_heap_object(const void *ptr, unsigned long n, struct page *page,
 			 bool to_user)
 {
 	struct kmem_cache *cachep;
-	unsigned int objnr;
+	unsigned int objnr = 0;
 	unsigned long offset;
+	bool is_kfence = is_kfence_address(ptr);
 
 	ptr = kasan_reset_tag(ptr);
 
 	/* Find and validate object. */
 	cachep = page->slab_cache;
-	objnr = obj_to_index(cachep, page, (void *)ptr);
-	BUG_ON(objnr >= cachep->num);
+	if (!is_kfence) {
+		objnr = obj_to_index(cachep, page, (void *)ptr);
+		BUG_ON(objnr >= cachep->num);
+	}
 
 	/* Find offset within object. */
-	offset = ptr - index_to_obj(cachep, page, objnr) - obj_offset(cachep);
+	if (is_kfence_address(ptr))
+		offset = ptr - kfence_object_start(ptr);
+	else
+		offset = ptr - index_to_obj(cachep, page, objnr) - obj_offset(cachep);
 
 	/* Allow address range falling entirely within usercopy region. */
 	if (offset >= cachep->useroffset &&
diff --git a/mm/slab_common.c b/mm/slab_common.c
index f9ccd5dc13f3..6e35e273681a 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -12,6 +12,7 @@
 #include <linux/memory.h>
 #include <linux/cache.h>
 #include <linux/compiler.h>
+#include <linux/kfence.h>
 #include <linux/module.h>
 #include <linux/cpu.h>
 #include <linux/uaccess.h>
@@ -448,6 +449,9 @@ static int shutdown_cache(struct kmem_cache *s)
 	/* free asan quarantined objects */
 	kasan_cache_shutdown(s);
 
+	if (!kfence_shutdown_cache(s))
+		return -EBUSY;
+
 	if (__kmem_cache_shutdown(s) != 0)
 		return -EBUSY;
 
@@ -1171,7 +1175,7 @@ size_t ksize(const void *objp)
 	if (unlikely(ZERO_OR_NULL_PTR(objp)) || !__kasan_check_read(objp, 1))
 		return 0;
 
-	size = __ksize(objp);
+	size = kfence_ksize(objp) ?: __ksize(objp);
 	/*
 	 * We assume that ksize callers could use whole allocated area,
 	 * so we need to unpoison this area.
-- 
2.28.0.526.ge36021eeef-goog

Re: [PATCH RFC 04/10] mm, kfence: insert KFENCE hooks for SLAB

[Dmitry Vyukov]

On Mon, Sep 7, 2020 at 3:41 PM Marco Elver <elver@google.com> wrote:
>
> From: Alexander Potapenko <glider@google.com>
>
> Inserts KFENCE hooks into the SLAB allocator.
>
> We note the addition of the 'orig_size' argument to slab_alloc*()
> functions, to be able to pass the originally requested size to KFENCE.
> When KFENCE is disabled, there is no additional overhead, since these
> functions are __always_inline.
>
> Co-developed-by: Marco Elver <elver@google.com>
> Signed-off-by: Marco Elver <elver@google.com>
> Signed-off-by: Alexander Potapenko <glider@google.com>
> ---
>  mm/slab.c        | 46 ++++++++++++++++++++++++++++++++++------------
>  mm/slab_common.c |  6 +++++-
>  2 files changed, 39 insertions(+), 13 deletions(-)
>
> diff --git a/mm/slab.c b/mm/slab.c
> index 3160dff6fd76..30aba06ae02b 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -100,6 +100,7 @@
>  #include       <linux/seq_file.h>
>  #include       <linux/notifier.h>
>  #include       <linux/kallsyms.h>
> +#include       <linux/kfence.h>
>  #include       <linux/cpu.h>
>  #include       <linux/sysctl.h>
>  #include       <linux/module.h>
> @@ -3206,7 +3207,7 @@ static void *____cache_alloc_node(struct kmem_cache *cachep, gfp_t flags,
>  }
>
>  static __always_inline void *
> -slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
> +slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid, size_t orig_size,
>                    unsigned long caller)
>  {
>         unsigned long save_flags;
> @@ -3219,6 +3220,10 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
>         if (unlikely(!cachep))
>                 return NULL;
>
> +       ptr = kfence_alloc(cachep, orig_size, flags);
> +       if (unlikely(ptr))
> +               goto out_hooks;
> +
>         cache_alloc_debugcheck_before(cachep, flags);
>         local_irq_save(save_flags);
>
> @@ -3251,6 +3256,7 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
>         if (unlikely(slab_want_init_on_alloc(flags, cachep)) && ptr)
>                 memset(ptr, 0, cachep->object_size);
>
> +out_hooks:
>         slab_post_alloc_hook(cachep, objcg, flags, 1, &ptr);
>         return ptr;
>  }
> @@ -3288,7 +3294,7 @@ __do_cache_alloc(struct kmem_cache *cachep, gfp_t flags)
>  #endif /* CONFIG_NUMA */
>
>  static __always_inline void *
> -slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
> +slab_alloc(struct kmem_cache *cachep, gfp_t flags, size_t orig_size, unsigned long caller)
>  {
>         unsigned long save_flags;
>         void *objp;
> @@ -3299,6 +3305,10 @@ slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
>         if (unlikely(!cachep))
>                 return NULL;
>
> +       objp = kfence_alloc(cachep, orig_size, flags);
> +       if (unlikely(objp))
> +               goto leave;
> +
>         cache_alloc_debugcheck_before(cachep, flags);
>         local_irq_save(save_flags);
>         objp = __do_cache_alloc(cachep, flags);
> @@ -3309,6 +3319,7 @@ slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
>         if (unlikely(slab_want_init_on_alloc(flags, cachep)) && objp)
>                 memset(objp, 0, cachep->object_size);
>
> +leave:
>         slab_post_alloc_hook(cachep, objcg, flags, 1, &objp);
>         return objp;
>  }
> @@ -3414,6 +3425,11 @@ static void cache_flusharray(struct kmem_cache *cachep, struct array_cache *ac)
>  static __always_inline void __cache_free(struct kmem_cache *cachep, void *objp,
>                                          unsigned long caller)
>  {
> +       if (kfence_free(objp)) {
> +               kmemleak_free_recursive(objp, cachep->flags);
> +               return;
> +       }
> +
>         /* Put the object into the quarantine, don't touch it for now. */
>         if (kasan_slab_free(cachep, objp, _RET_IP_))
>                 return;
> @@ -3479,7 +3495,7 @@ void ___cache_free(struct kmem_cache *cachep, void *objp,
>   */
>  void *kmem_cache_alloc(struct kmem_cache *cachep, gfp_t flags)
>  {
> -       void *ret = slab_alloc(cachep, flags, _RET_IP_);
> +       void *ret = slab_alloc(cachep, flags, cachep->object_size, _RET_IP_);


It's kinda minor, but since we are talking about malloc fast path:
will passing 0 instead of cachep->object_size (here and everywhere
else) and then using cachep->object_size on the slow path if 0 is
passed as size improve codegen?


>         trace_kmem_cache_alloc(_RET_IP_, ret,
>                                cachep->object_size, cachep->size, flags);
> @@ -3512,7 +3528,7 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
>
>         local_irq_disable();
>         for (i = 0; i < size; i++) {
> -               void *objp = __do_cache_alloc(s, flags);
> +               void *objp = kfence_alloc(s, s->object_size, flags) ?: __do_cache_alloc(s, flags);
>
>                 if (unlikely(!objp))
>                         goto error;
> @@ -3545,7 +3561,7 @@ kmem_cache_alloc_trace(struct kmem_cache *cachep, gfp_t flags, size_t size)
>  {
>         void *ret;
>
> -       ret = slab_alloc(cachep, flags, _RET_IP_);
> +       ret = slab_alloc(cachep, flags, size, _RET_IP_);
>
>         ret = kasan_kmalloc(cachep, ret, size, flags);
>         trace_kmalloc(_RET_IP_, ret,
> @@ -3571,7 +3587,7 @@ EXPORT_SYMBOL(kmem_cache_alloc_trace);
>   */
>  void *kmem_cache_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid)
>  {
> -       void *ret = slab_alloc_node(cachep, flags, nodeid, _RET_IP_);
> +       void *ret = slab_alloc_node(cachep, flags, nodeid, cachep->object_size, _RET_IP_);
>
>         trace_kmem_cache_alloc_node(_RET_IP_, ret,
>                                     cachep->object_size, cachep->size,
> @@ -3589,7 +3605,7 @@ void *kmem_cache_alloc_node_trace(struct kmem_cache *cachep,
>  {
>         void *ret;
>
> -       ret = slab_alloc_node(cachep, flags, nodeid, _RET_IP_);
> +       ret = slab_alloc_node(cachep, flags, nodeid, size, _RET_IP_);
>
>         ret = kasan_kmalloc(cachep, ret, size, flags);
>         trace_kmalloc_node(_RET_IP_, ret,
> @@ -3650,7 +3666,7 @@ static __always_inline void *__do_kmalloc(size_t size, gfp_t flags,
>         cachep = kmalloc_slab(size, flags);
>         if (unlikely(ZERO_OR_NULL_PTR(cachep)))
>                 return cachep;
> -       ret = slab_alloc(cachep, flags, caller);
> +       ret = slab_alloc(cachep, flags, size, caller);
>
>         ret = kasan_kmalloc(cachep, ret, size, flags);
>         trace_kmalloc(caller, ret,
> @@ -4138,18 +4154,24 @@ void __check_heap_object(const void *ptr, unsigned long n, struct page *page,
>                          bool to_user)
>  {
>         struct kmem_cache *cachep;
> -       unsigned int objnr;
> +       unsigned int objnr = 0;
>         unsigned long offset;
> +       bool is_kfence = is_kfence_address(ptr);
>
>         ptr = kasan_reset_tag(ptr);
>
>         /* Find and validate object. */
>         cachep = page->slab_cache;
> -       objnr = obj_to_index(cachep, page, (void *)ptr);
> -       BUG_ON(objnr >= cachep->num);
> +       if (!is_kfence) {
> +               objnr = obj_to_index(cachep, page, (void *)ptr);
> +               BUG_ON(objnr >= cachep->num);
> +       }
>
>         /* Find offset within object. */
> -       offset = ptr - index_to_obj(cachep, page, objnr) - obj_offset(cachep);
> +       if (is_kfence_address(ptr))
> +               offset = ptr - kfence_object_start(ptr);
> +       else
> +               offset = ptr - index_to_obj(cachep, page, objnr) - obj_offset(cachep);
>
>         /* Allow address range falling entirely within usercopy region. */
>         if (offset >= cachep->useroffset &&
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index f9ccd5dc13f3..6e35e273681a 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -12,6 +12,7 @@
>  #include <linux/memory.h>
>  #include <linux/cache.h>
>  #include <linux/compiler.h>
> +#include <linux/kfence.h>
>  #include <linux/module.h>
>  #include <linux/cpu.h>
>  #include <linux/uaccess.h>
> @@ -448,6 +449,9 @@ static int shutdown_cache(struct kmem_cache *s)
>         /* free asan quarantined objects */
>         kasan_cache_shutdown(s);
>
> +       if (!kfence_shutdown_cache(s))
> +               return -EBUSY;
> +
>         if (__kmem_cache_shutdown(s) != 0)
>                 return -EBUSY;
>
> @@ -1171,7 +1175,7 @@ size_t ksize(const void *objp)
>         if (unlikely(ZERO_OR_NULL_PTR(objp)) || !__kasan_check_read(objp, 1))
>                 return 0;
>
> -       size = __ksize(objp);
> +       size = kfence_ksize(objp) ?: __ksize(objp);
>         /*
>          * We assume that ksize callers could use whole allocated area,
>          * so we need to unpoison this area.
> --
> 2.28.0.526.ge36021eeef-goog
>

Re: [PATCH RFC 04/10] mm, kfence: insert KFENCE hooks for SLAB

[Dmitry Vyukov]

On Mon, Sep 7, 2020 at 3:41 PM Marco Elver <elver@google.com> wrote:
>
> From: Alexander Potapenko <glider@google.com>
>
> Inserts KFENCE hooks into the SLAB allocator.
>
> We note the addition of the 'orig_size' argument to slab_alloc*()
> functions, to be able to pass the originally requested size to KFENCE.
> When KFENCE is disabled, there is no additional overhead, since these
> functions are __always_inline.
>
> Co-developed-by: Marco Elver <elver@google.com>
> Signed-off-by: Marco Elver <elver@google.com>
> Signed-off-by: Alexander Potapenko <glider@google.com>
> ---
>  mm/slab.c        | 46 ++++++++++++++++++++++++++++++++++------------
>  mm/slab_common.c |  6 +++++-
>  2 files changed, 39 insertions(+), 13 deletions(-)
>
> diff --git a/mm/slab.c b/mm/slab.c
> index 3160dff6fd76..30aba06ae02b 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -100,6 +100,7 @@
>  #include       <linux/seq_file.h>
>  #include       <linux/notifier.h>
>  #include       <linux/kallsyms.h>
> +#include       <linux/kfence.h>
>  #include       <linux/cpu.h>
>  #include       <linux/sysctl.h>
>  #include       <linux/module.h>
> @@ -3206,7 +3207,7 @@ static void *____cache_alloc_node(struct kmem_cache *cachep, gfp_t flags,
>  }
>
>  static __always_inline void *
> -slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
> +slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid, size_t orig_size,
>                    unsigned long caller)
>  {
>         unsigned long save_flags;
> @@ -3219,6 +3220,10 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
>         if (unlikely(!cachep))
>                 return NULL;
>
> +       ptr = kfence_alloc(cachep, orig_size, flags);
> +       if (unlikely(ptr))
> +               goto out_hooks;
> +
>         cache_alloc_debugcheck_before(cachep, flags);
>         local_irq_save(save_flags);
>
> @@ -3251,6 +3256,7 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
>         if (unlikely(slab_want_init_on_alloc(flags, cachep)) && ptr)
>                 memset(ptr, 0, cachep->object_size);
>
> +out_hooks:
>         slab_post_alloc_hook(cachep, objcg, flags, 1, &ptr);
>         return ptr;
>  }
> @@ -3288,7 +3294,7 @@ __do_cache_alloc(struct kmem_cache *cachep, gfp_t flags)
>  #endif /* CONFIG_NUMA */
>
>  static __always_inline void *
> -slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
> +slab_alloc(struct kmem_cache *cachep, gfp_t flags, size_t orig_size, unsigned long caller)
>  {
>         unsigned long save_flags;
>         void *objp;
> @@ -3299,6 +3305,10 @@ slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
>         if (unlikely(!cachep))
>                 return NULL;
>
> +       objp = kfence_alloc(cachep, orig_size, flags);
> +       if (unlikely(objp))
> +               goto leave;
> +
>         cache_alloc_debugcheck_before(cachep, flags);
>         local_irq_save(save_flags);
>         objp = __do_cache_alloc(cachep, flags);
> @@ -3309,6 +3319,7 @@ slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
>         if (unlikely(slab_want_init_on_alloc(flags, cachep)) && objp)
>                 memset(objp, 0, cachep->object_size);
>
> +leave:
>         slab_post_alloc_hook(cachep, objcg, flags, 1, &objp);
>         return objp;
>  }
> @@ -3414,6 +3425,11 @@ static void cache_flusharray(struct kmem_cache *cachep, struct array_cache *ac)
>  static __always_inline void __cache_free(struct kmem_cache *cachep, void *objp,
>                                          unsigned long caller)
>  {
> +       if (kfence_free(objp)) {
> +               kmemleak_free_recursive(objp, cachep->flags);
> +               return;
> +       }
> +
>         /* Put the object into the quarantine, don't touch it for now. */
>         if (kasan_slab_free(cachep, objp, _RET_IP_))
>                 return;
> @@ -3479,7 +3495,7 @@ void ___cache_free(struct kmem_cache *cachep, void *objp,
>   */
>  void *kmem_cache_alloc(struct kmem_cache *cachep, gfp_t flags)
>  {
> -       void *ret = slab_alloc(cachep, flags, _RET_IP_);
> +       void *ret = slab_alloc(cachep, flags, cachep->object_size, _RET_IP_);


It's kinda minor, but since we are talking about malloc fast path:
will passing 0 instead of cachep->object_size (here and everywhere
else) and then using cachep->object_size on the slow path if 0 is
passed as size improve codegen?


>         trace_kmem_cache_alloc(_RET_IP_, ret,
>                                cachep->object_size, cachep->size, flags);
> @@ -3512,7 +3528,7 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
>
>         local_irq_disable();
>         for (i = 0; i < size; i++) {
> -               void *objp = __do_cache_alloc(s, flags);
> +               void *objp = kfence_alloc(s, s->object_size, flags) ?: __do_cache_alloc(s, flags);
>
>                 if (unlikely(!objp))
>                         goto error;
> @@ -3545,7 +3561,7 @@ kmem_cache_alloc_trace(struct kmem_cache *cachep, gfp_t flags, size_t size)
>  {
>         void *ret;
>
> -       ret = slab_alloc(cachep, flags, _RET_IP_);
> +       ret = slab_alloc(cachep, flags, size, _RET_IP_);
>
>         ret = kasan_kmalloc(cachep, ret, size, flags);
>         trace_kmalloc(_RET_IP_, ret,
> @@ -3571,7 +3587,7 @@ EXPORT_SYMBOL(kmem_cache_alloc_trace);
>   */
>  void *kmem_cache_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid)
>  {
> -       void *ret = slab_alloc_node(cachep, flags, nodeid, _RET_IP_);
> +       void *ret = slab_alloc_node(cachep, flags, nodeid, cachep->object_size, _RET_IP_);
>
>         trace_kmem_cache_alloc_node(_RET_IP_, ret,
>                                     cachep->object_size, cachep->size,
> @@ -3589,7 +3605,7 @@ void *kmem_cache_alloc_node_trace(struct kmem_cache *cachep,
>  {
>         void *ret;
>
> -       ret = slab_alloc_node(cachep, flags, nodeid, _RET_IP_);
> +       ret = slab_alloc_node(cachep, flags, nodeid, size, _RET_IP_);
>
>         ret = kasan_kmalloc(cachep, ret, size, flags);
>         trace_kmalloc_node(_RET_IP_, ret,
> @@ -3650,7 +3666,7 @@ static __always_inline void *__do_kmalloc(size_t size, gfp_t flags,
>         cachep = kmalloc_slab(size, flags);
>         if (unlikely(ZERO_OR_NULL_PTR(cachep)))
>                 return cachep;
> -       ret = slab_alloc(cachep, flags, caller);
> +       ret = slab_alloc(cachep, flags, size, caller);
>
>         ret = kasan_kmalloc(cachep, ret, size, flags);
>         trace_kmalloc(caller, ret,
> @@ -4138,18 +4154,24 @@ void __check_heap_object(const void *ptr, unsigned long n, struct page *page,
>                          bool to_user)
>  {
>         struct kmem_cache *cachep;
> -       unsigned int objnr;
> +       unsigned int objnr = 0;
>         unsigned long offset;
> +       bool is_kfence = is_kfence_address(ptr);
>
>         ptr = kasan_reset_tag(ptr);
>
>         /* Find and validate object. */
>         cachep = page->slab_cache;
> -       objnr = obj_to_index(cachep, page, (void *)ptr);
> -       BUG_ON(objnr >= cachep->num);
> +       if (!is_kfence) {
> +               objnr = obj_to_index(cachep, page, (void *)ptr);
> +               BUG_ON(objnr >= cachep->num);
> +       }
>
>         /* Find offset within object. */
> -       offset = ptr - index_to_obj(cachep, page, objnr) - obj_offset(cachep);
> +       if (is_kfence_address(ptr))
> +               offset = ptr - kfence_object_start(ptr);
> +       else
> +               offset = ptr - index_to_obj(cachep, page, objnr) - obj_offset(cachep);
>
>         /* Allow address range falling entirely within usercopy region. */
>         if (offset >= cachep->useroffset &&
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index f9ccd5dc13f3..6e35e273681a 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -12,6 +12,7 @@
>  #include <linux/memory.h>
>  #include <linux/cache.h>
>  #include <linux/compiler.h>
> +#include <linux/kfence.h>
>  #include <linux/module.h>
>  #include <linux/cpu.h>
>  #include <linux/uaccess.h>
> @@ -448,6 +449,9 @@ static int shutdown_cache(struct kmem_cache *s)
>         /* free asan quarantined objects */
>         kasan_cache_shutdown(s);
>
> +       if (!kfence_shutdown_cache(s))
> +               return -EBUSY;
> +
>         if (__kmem_cache_shutdown(s) != 0)
>                 return -EBUSY;
>
> @@ -1171,7 +1175,7 @@ size_t ksize(const void *objp)
>         if (unlikely(ZERO_OR_NULL_PTR(objp)) || !__kasan_check_read(objp, 1))
>                 return 0;
>
> -       size = __ksize(objp);
> +       size = kfence_ksize(objp) ?: __ksize(objp);
>         /*
>          * We assume that ksize callers could use whole allocated area,
>          * so we need to unpoison this area.
> --
> 2.28.0.526.ge36021eeef-goog
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC 04/10] mm, kfence: insert KFENCE hooks for SLAB

[Dmitry Vyukov]

On Mon, Sep 7, 2020 at 3:41 PM Marco Elver <elver@google.com> wrote:
>
> From: Alexander Potapenko <glider@google.com>
>
> Inserts KFENCE hooks into the SLAB allocator.
>
> We note the addition of the 'orig_size' argument to slab_alloc*()
> functions, to be able to pass the originally requested size to KFENCE.
> When KFENCE is disabled, there is no additional overhead, since these
> functions are __always_inline.
>
> Co-developed-by: Marco Elver <elver@google.com>
> Signed-off-by: Marco Elver <elver@google.com>
> Signed-off-by: Alexander Potapenko <glider@google.com>
> ---
>  mm/slab.c        | 46 ++++++++++++++++++++++++++++++++++------------
>  mm/slab_common.c |  6 +++++-
>  2 files changed, 39 insertions(+), 13 deletions(-)
>
> diff --git a/mm/slab.c b/mm/slab.c
> index 3160dff6fd76..30aba06ae02b 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -100,6 +100,7 @@
>  #include       <linux/seq_file.h>
>  #include       <linux/notifier.h>
>  #include       <linux/kallsyms.h>
> +#include       <linux/kfence.h>
>  #include       <linux/cpu.h>
>  #include       <linux/sysctl.h>
>  #include       <linux/module.h>
> @@ -3206,7 +3207,7 @@ static void *____cache_alloc_node(struct kmem_cache *cachep, gfp_t flags,
>  }
>
>  static __always_inline void *
> -slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
> +slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid, size_t orig_size,
>                    unsigned long caller)
>  {
>         unsigned long save_flags;
> @@ -3219,6 +3220,10 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
>         if (unlikely(!cachep))
>                 return NULL;
>
> +       ptr = kfence_alloc(cachep, orig_size, flags);
> +       if (unlikely(ptr))
> +               goto out_hooks;
> +
>         cache_alloc_debugcheck_before(cachep, flags);
>         local_irq_save(save_flags);
>
> @@ -3251,6 +3256,7 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
>         if (unlikely(slab_want_init_on_alloc(flags, cachep)) && ptr)
>                 memset(ptr, 0, cachep->object_size);
>
> +out_hooks:
>         slab_post_alloc_hook(cachep, objcg, flags, 1, &ptr);
>         return ptr;
>  }
> @@ -3288,7 +3294,7 @@ __do_cache_alloc(struct kmem_cache *cachep, gfp_t flags)
>  #endif /* CONFIG_NUMA */
>
>  static __always_inline void *
> -slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
> +slab_alloc(struct kmem_cache *cachep, gfp_t flags, size_t orig_size, unsigned long caller)
>  {
>         unsigned long save_flags;
>         void *objp;
> @@ -3299,6 +3305,10 @@ slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
>         if (unlikely(!cachep))
>                 return NULL;
>
> +       objp = kfence_alloc(cachep, orig_size, flags);
> +       if (unlikely(objp))
> +               goto leave;
> +
>         cache_alloc_debugcheck_before(cachep, flags);
>         local_irq_save(save_flags);
>         objp = __do_cache_alloc(cachep, flags);
> @@ -3309,6 +3319,7 @@ slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
>         if (unlikely(slab_want_init_on_alloc(flags, cachep)) && objp)
>                 memset(objp, 0, cachep->object_size);
>
> +leave:
>         slab_post_alloc_hook(cachep, objcg, flags, 1, &objp);
>         return objp;
>  }
> @@ -3414,6 +3425,11 @@ static void cache_flusharray(struct kmem_cache *cachep, struct array_cache *ac)
>  static __always_inline void __cache_free(struct kmem_cache *cachep, void *objp,
>                                          unsigned long caller)
>  {
> +       if (kfence_free(objp)) {
> +               kmemleak_free_recursive(objp, cachep->flags);
> +               return;
> +       }
> +
>         /* Put the object into the quarantine, don't touch it for now. */
>         if (kasan_slab_free(cachep, objp, _RET_IP_))
>                 return;
> @@ -3479,7 +3495,7 @@ void ___cache_free(struct kmem_cache *cachep, void *objp,
>   */
>  void *kmem_cache_alloc(struct kmem_cache *cachep, gfp_t flags)
>  {
> -       void *ret = slab_alloc(cachep, flags, _RET_IP_);
> +       void *ret = slab_alloc(cachep, flags, cachep->object_size, _RET_IP_);


It's kinda minor, but since we are talking about malloc fast path:
will passing 0 instead of cachep->object_size (here and everywhere
else) and then using cachep->object_size on the slow path if 0 is
passed as size improve codegen?


>         trace_kmem_cache_alloc(_RET_IP_, ret,
>                                cachep->object_size, cachep->size, flags);
> @@ -3512,7 +3528,7 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size,
>
>         local_irq_disable();
>         for (i = 0; i < size; i++) {
> -               void *objp = __do_cache_alloc(s, flags);
> +               void *objp = kfence_alloc(s, s->object_size, flags) ?: __do_cache_alloc(s, flags);
>
>                 if (unlikely(!objp))
>                         goto error;
> @@ -3545,7 +3561,7 @@ kmem_cache_alloc_trace(struct kmem_cache *cachep, gfp_t flags, size_t size)
>  {
>         void *ret;
>
> -       ret = slab_alloc(cachep, flags, _RET_IP_);
> +       ret = slab_alloc(cachep, flags, size, _RET_IP_);
>
>         ret = kasan_kmalloc(cachep, ret, size, flags);
>         trace_kmalloc(_RET_IP_, ret,
> @@ -3571,7 +3587,7 @@ EXPORT_SYMBOL(kmem_cache_alloc_trace);
>   */
>  void *kmem_cache_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid)
>  {
> -       void *ret = slab_alloc_node(cachep, flags, nodeid, _RET_IP_);
> +       void *ret = slab_alloc_node(cachep, flags, nodeid, cachep->object_size, _RET_IP_);
>
>         trace_kmem_cache_alloc_node(_RET_IP_, ret,
>                                     cachep->object_size, cachep->size,
> @@ -3589,7 +3605,7 @@ void *kmem_cache_alloc_node_trace(struct kmem_cache *cachep,
>  {
>         void *ret;
>
> -       ret = slab_alloc_node(cachep, flags, nodeid, _RET_IP_);
> +       ret = slab_alloc_node(cachep, flags, nodeid, size, _RET_IP_);
>
>         ret = kasan_kmalloc(cachep, ret, size, flags);
>         trace_kmalloc_node(_RET_IP_, ret,
> @@ -3650,7 +3666,7 @@ static __always_inline void *__do_kmalloc(size_t size, gfp_t flags,
>         cachep = kmalloc_slab(size, flags);
>         if (unlikely(ZERO_OR_NULL_PTR(cachep)))
>                 return cachep;
> -       ret = slab_alloc(cachep, flags, caller);
> +       ret = slab_alloc(cachep, flags, size, caller);
>
>         ret = kasan_kmalloc(cachep, ret, size, flags);
>         trace_kmalloc(caller, ret,
> @@ -4138,18 +4154,24 @@ void __check_heap_object(const void *ptr, unsigned long n, struct page *page,
>                          bool to_user)
>  {
>         struct kmem_cache *cachep;
> -       unsigned int objnr;
> +       unsigned int objnr = 0;
>         unsigned long offset;
> +       bool is_kfence = is_kfence_address(ptr);
>
>         ptr = kasan_reset_tag(ptr);
>
>         /* Find and validate object. */
>         cachep = page->slab_cache;
> -       objnr = obj_to_index(cachep, page, (void *)ptr);
> -       BUG_ON(objnr >= cachep->num);
> +       if (!is_kfence) {
> +               objnr = obj_to_index(cachep, page, (void *)ptr);
> +               BUG_ON(objnr >= cachep->num);
> +       }
>
>         /* Find offset within object. */
> -       offset = ptr - index_to_obj(cachep, page, objnr) - obj_offset(cachep);
> +       if (is_kfence_address(ptr))
> +               offset = ptr - kfence_object_start(ptr);
> +       else
> +               offset = ptr - index_to_obj(cachep, page, objnr) - obj_offset(cachep);
>
>         /* Allow address range falling entirely within usercopy region. */
>         if (offset >= cachep->useroffset &&
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index f9ccd5dc13f3..6e35e273681a 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -12,6 +12,7 @@
>  #include <linux/memory.h>
>  #include <linux/cache.h>
>  #include <linux/compiler.h>
> +#include <linux/kfence.h>
>  #include <linux/module.h>
>  #include <linux/cpu.h>
>  #include <linux/uaccess.h>
> @@ -448,6 +449,9 @@ static int shutdown_cache(struct kmem_cache *s)
>         /* free asan quarantined objects */
>         kasan_cache_shutdown(s);
>
> +       if (!kfence_shutdown_cache(s))
> +               return -EBUSY;
> +
>         if (__kmem_cache_shutdown(s) != 0)
>                 return -EBUSY;
>
> @@ -1171,7 +1175,7 @@ size_t ksize(const void *objp)
>         if (unlikely(ZERO_OR_NULL_PTR(objp)) || !__kasan_check_read(objp, 1))
>                 return 0;
>
> -       size = __ksize(objp);
> +       size = kfence_ksize(objp) ?: __ksize(objp);
>         /*
>          * We assume that ksize callers could use whole allocated area,
>          * so we need to unpoison this area.
> --
> 2.28.0.526.ge36021eeef-goog
>

Re: [PATCH RFC 04/10] mm, kfence: insert KFENCE hooks for SLAB

[Marco Elver]

On Fri, 11 Sep 2020 at 09:17, Dmitry Vyukov <dvyukov@google.com> wrote:
>
> On Mon, Sep 7, 2020 at 3:41 PM Marco Elver <elver@google.com> wrote:
> >
> > From: Alexander Potapenko <glider@google.com>
> >
> > Inserts KFENCE hooks into the SLAB allocator.
> >
> > We note the addition of the 'orig_size' argument to slab_alloc*()
> > functions, to be able to pass the originally requested size to KFENCE.
> > When KFENCE is disabled, there is no additional overhead, since these
> > functions are __always_inline.
> >
> > Co-developed-by: Marco Elver <elver@google.com>
> > Signed-off-by: Marco Elver <elver@google.com>
> > Signed-off-by: Alexander Potapenko <glider@google.com>
> > ---
> >  mm/slab.c        | 46 ++++++++++++++++++++++++++++++++++------------
> >  mm/slab_common.c |  6 +++++-
> >  2 files changed, 39 insertions(+), 13 deletions(-)
> >
> > diff --git a/mm/slab.c b/mm/slab.c
> > index 3160dff6fd76..30aba06ae02b 100644
> > --- a/mm/slab.c
> > +++ b/mm/slab.c
> > @@ -100,6 +100,7 @@
> >  #include       <linux/seq_file.h>
> >  #include       <linux/notifier.h>
> >  #include       <linux/kallsyms.h>
> > +#include       <linux/kfence.h>
> >  #include       <linux/cpu.h>
> >  #include       <linux/sysctl.h>
> >  #include       <linux/module.h>
> > @@ -3206,7 +3207,7 @@ static void *____cache_alloc_node(struct kmem_cache *cachep, gfp_t flags,
> >  }
> >
> >  static __always_inline void *
> > -slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
> > +slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid, size_t orig_size,
> >                    unsigned long caller)
> >  {
> >         unsigned long save_flags;
> > @@ -3219,6 +3220,10 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
> >         if (unlikely(!cachep))
> >                 return NULL;
> >
> > +       ptr = kfence_alloc(cachep, orig_size, flags);
> > +       if (unlikely(ptr))
> > +               goto out_hooks;
> > +
> >         cache_alloc_debugcheck_before(cachep, flags);
> >         local_irq_save(save_flags);
> >
> > @@ -3251,6 +3256,7 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
> >         if (unlikely(slab_want_init_on_alloc(flags, cachep)) && ptr)
> >                 memset(ptr, 0, cachep->object_size);
> >
> > +out_hooks:
> >         slab_post_alloc_hook(cachep, objcg, flags, 1, &ptr);
> >         return ptr;
> >  }
> > @@ -3288,7 +3294,7 @@ __do_cache_alloc(struct kmem_cache *cachep, gfp_t flags)
> >  #endif /* CONFIG_NUMA */
> >
> >  static __always_inline void *
> > -slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
> > +slab_alloc(struct kmem_cache *cachep, gfp_t flags, size_t orig_size, unsigned long caller)
> >  {
> >         unsigned long save_flags;
> >         void *objp;
> > @@ -3299,6 +3305,10 @@ slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
> >         if (unlikely(!cachep))
> >                 return NULL;
> >
> > +       objp = kfence_alloc(cachep, orig_size, flags);
> > +       if (unlikely(objp))
> > +               goto leave;
> > +
> >         cache_alloc_debugcheck_before(cachep, flags);
> >         local_irq_save(save_flags);
> >         objp = __do_cache_alloc(cachep, flags);
> > @@ -3309,6 +3319,7 @@ slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
> >         if (unlikely(slab_want_init_on_alloc(flags, cachep)) && objp)
> >                 memset(objp, 0, cachep->object_size);
> >
> > +leave:
> >         slab_post_alloc_hook(cachep, objcg, flags, 1, &objp);
> >         return objp;
> >  }
> > @@ -3414,6 +3425,11 @@ static void cache_flusharray(struct kmem_cache *cachep, struct array_cache *ac)
> >  static __always_inline void __cache_free(struct kmem_cache *cachep, void *objp,
> >                                          unsigned long caller)
> >  {
> > +       if (kfence_free(objp)) {
> > +               kmemleak_free_recursive(objp, cachep->flags);
> > +               return;
> > +       }
> > +
> >         /* Put the object into the quarantine, don't touch it for now. */
> >         if (kasan_slab_free(cachep, objp, _RET_IP_))
> >                 return;
> > @@ -3479,7 +3495,7 @@ void ___cache_free(struct kmem_cache *cachep, void *objp,
> >   */
> >  void *kmem_cache_alloc(struct kmem_cache *cachep, gfp_t flags)
> >  {
> > -       void *ret = slab_alloc(cachep, flags, _RET_IP_);
> > +       void *ret = slab_alloc(cachep, flags, cachep->object_size, _RET_IP_);
>
>
> It's kinda minor, but since we are talking about malloc fast path:
> will passing 0 instead of cachep->object_size (here and everywhere
> else) and then using cachep->object_size on the slow path if 0 is
> passed as size improve codegen?

It doesn't save us much, maybe 1 instruction based on what I'm looking
at right now. The main worry I have is that the 'orig_size' argument
is now part of slab_alloc, and changing its semantics may cause
problems in future if it's no longer just passed to kfence_alloc().
Today, we can do the 'size = size ?: cache->object_size' trick inside
kfence_alloc(), but at the cost breaking the intuitive semantics of
slab_alloc's orig_size argument for future users. Is it worth it?

Thanks,
-- Marco

Re: [PATCH RFC 04/10] mm, kfence: insert KFENCE hooks for SLAB

[Marco Elver]

On Fri, 11 Sep 2020 at 09:17, Dmitry Vyukov <dvyukov@google.com> wrote:
>
> On Mon, Sep 7, 2020 at 3:41 PM Marco Elver <elver@google.com> wrote:
> >
> > From: Alexander Potapenko <glider@google.com>
> >
> > Inserts KFENCE hooks into the SLAB allocator.
> >
> > We note the addition of the 'orig_size' argument to slab_alloc*()
> > functions, to be able to pass the originally requested size to KFENCE.
> > When KFENCE is disabled, there is no additional overhead, since these
> > functions are __always_inline.
> >
> > Co-developed-by: Marco Elver <elver@google.com>
> > Signed-off-by: Marco Elver <elver@google.com>
> > Signed-off-by: Alexander Potapenko <glider@google.com>
> > ---
> >  mm/slab.c        | 46 ++++++++++++++++++++++++++++++++++------------
> >  mm/slab_common.c |  6 +++++-
> >  2 files changed, 39 insertions(+), 13 deletions(-)
> >
> > diff --git a/mm/slab.c b/mm/slab.c
> > index 3160dff6fd76..30aba06ae02b 100644
> > --- a/mm/slab.c
> > +++ b/mm/slab.c
> > @@ -100,6 +100,7 @@
> >  #include       <linux/seq_file.h>
> >  #include       <linux/notifier.h>
> >  #include       <linux/kallsyms.h>
> > +#include       <linux/kfence.h>
> >  #include       <linux/cpu.h>
> >  #include       <linux/sysctl.h>
> >  #include       <linux/module.h>
> > @@ -3206,7 +3207,7 @@ static void *____cache_alloc_node(struct kmem_cache *cachep, gfp_t flags,
> >  }
> >
> >  static __always_inline void *
> > -slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
> > +slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid, size_t orig_size,
> >                    unsigned long caller)
> >  {
> >         unsigned long save_flags;
> > @@ -3219,6 +3220,10 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
> >         if (unlikely(!cachep))
> >                 return NULL;
> >
> > +       ptr = kfence_alloc(cachep, orig_size, flags);
> > +       if (unlikely(ptr))
> > +               goto out_hooks;
> > +
> >         cache_alloc_debugcheck_before(cachep, flags);
> >         local_irq_save(save_flags);
> >
> > @@ -3251,6 +3256,7 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
> >         if (unlikely(slab_want_init_on_alloc(flags, cachep)) && ptr)
> >                 memset(ptr, 0, cachep->object_size);
> >
> > +out_hooks:
> >         slab_post_alloc_hook(cachep, objcg, flags, 1, &ptr);
> >         return ptr;
> >  }
> > @@ -3288,7 +3294,7 @@ __do_cache_alloc(struct kmem_cache *cachep, gfp_t flags)
> >  #endif /* CONFIG_NUMA */
> >
> >  static __always_inline void *
> > -slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
> > +slab_alloc(struct kmem_cache *cachep, gfp_t flags, size_t orig_size, unsigned long caller)
> >  {
> >         unsigned long save_flags;
> >         void *objp;
> > @@ -3299,6 +3305,10 @@ slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
> >         if (unlikely(!cachep))
> >                 return NULL;
> >
> > +       objp = kfence_alloc(cachep, orig_size, flags);
> > +       if (unlikely(objp))
> > +               goto leave;
> > +
> >         cache_alloc_debugcheck_before(cachep, flags);
> >         local_irq_save(save_flags);
> >         objp = __do_cache_alloc(cachep, flags);
> > @@ -3309,6 +3319,7 @@ slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
> >         if (unlikely(slab_want_init_on_alloc(flags, cachep)) && objp)
> >                 memset(objp, 0, cachep->object_size);
> >
> > +leave:
> >         slab_post_alloc_hook(cachep, objcg, flags, 1, &objp);
> >         return objp;
> >  }
> > @@ -3414,6 +3425,11 @@ static void cache_flusharray(struct kmem_cache *cachep, struct array_cache *ac)
> >  static __always_inline void __cache_free(struct kmem_cache *cachep, void *objp,
> >                                          unsigned long caller)
> >  {
> > +       if (kfence_free(objp)) {
> > +               kmemleak_free_recursive(objp, cachep->flags);
> > +               return;
> > +       }
> > +
> >         /* Put the object into the quarantine, don't touch it for now. */
> >         if (kasan_slab_free(cachep, objp, _RET_IP_))
> >                 return;
> > @@ -3479,7 +3495,7 @@ void ___cache_free(struct kmem_cache *cachep, void *objp,
> >   */
> >  void *kmem_cache_alloc(struct kmem_cache *cachep, gfp_t flags)
> >  {
> > -       void *ret = slab_alloc(cachep, flags, _RET_IP_);
> > +       void *ret = slab_alloc(cachep, flags, cachep->object_size, _RET_IP_);
>
>
> It's kinda minor, but since we are talking about malloc fast path:
> will passing 0 instead of cachep->object_size (here and everywhere
> else) and then using cachep->object_size on the slow path if 0 is
> passed as size improve codegen?

It doesn't save us much, maybe 1 instruction based on what I'm looking
at right now. The main worry I have is that the 'orig_size' argument
is now part of slab_alloc, and changing its semantics may cause
problems in future if it's no longer just passed to kfence_alloc().
Today, we can do the 'size = size ?: cache->object_size' trick inside
kfence_alloc(), but at the cost breaking the intuitive semantics of
slab_alloc's orig_size argument for future users. Is it worth it?

Thanks,
-- Marco

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC 04/10] mm, kfence: insert KFENCE hooks for SLAB

[Marco Elver]

On Fri, 11 Sep 2020 at 09:17, Dmitry Vyukov <dvyukov@google.com> wrote:
>
> On Mon, Sep 7, 2020 at 3:41 PM Marco Elver <elver@google.com> wrote:
> >
> > From: Alexander Potapenko <glider@google.com>
> >
> > Inserts KFENCE hooks into the SLAB allocator.
> >
> > We note the addition of the 'orig_size' argument to slab_alloc*()
> > functions, to be able to pass the originally requested size to KFENCE.
> > When KFENCE is disabled, there is no additional overhead, since these
> > functions are __always_inline.
> >
> > Co-developed-by: Marco Elver <elver@google.com>
> > Signed-off-by: Marco Elver <elver@google.com>
> > Signed-off-by: Alexander Potapenko <glider@google.com>
> > ---
> >  mm/slab.c        | 46 ++++++++++++++++++++++++++++++++++------------
> >  mm/slab_common.c |  6 +++++-
> >  2 files changed, 39 insertions(+), 13 deletions(-)
> >
> > diff --git a/mm/slab.c b/mm/slab.c
> > index 3160dff6fd76..30aba06ae02b 100644
> > --- a/mm/slab.c
> > +++ b/mm/slab.c
> > @@ -100,6 +100,7 @@
> >  #include       <linux/seq_file.h>
> >  #include       <linux/notifier.h>
> >  #include       <linux/kallsyms.h>
> > +#include       <linux/kfence.h>
> >  #include       <linux/cpu.h>
> >  #include       <linux/sysctl.h>
> >  #include       <linux/module.h>
> > @@ -3206,7 +3207,7 @@ static void *____cache_alloc_node(struct kmem_cache *cachep, gfp_t flags,
> >  }
> >
> >  static __always_inline void *
> > -slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
> > +slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid, size_t orig_size,
> >                    unsigned long caller)
> >  {
> >         unsigned long save_flags;
> > @@ -3219,6 +3220,10 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
> >         if (unlikely(!cachep))
> >                 return NULL;
> >
> > +       ptr = kfence_alloc(cachep, orig_size, flags);
> > +       if (unlikely(ptr))
> > +               goto out_hooks;
> > +
> >         cache_alloc_debugcheck_before(cachep, flags);
> >         local_irq_save(save_flags);
> >
> > @@ -3251,6 +3256,7 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
> >         if (unlikely(slab_want_init_on_alloc(flags, cachep)) && ptr)
> >                 memset(ptr, 0, cachep->object_size);
> >
> > +out_hooks:
> >         slab_post_alloc_hook(cachep, objcg, flags, 1, &ptr);
> >         return ptr;
> >  }
> > @@ -3288,7 +3294,7 @@ __do_cache_alloc(struct kmem_cache *cachep, gfp_t flags)
> >  #endif /* CONFIG_NUMA */
> >
> >  static __always_inline void *
> > -slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
> > +slab_alloc(struct kmem_cache *cachep, gfp_t flags, size_t orig_size, unsigned long caller)
> >  {
> >         unsigned long save_flags;
> >         void *objp;
> > @@ -3299,6 +3305,10 @@ slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
> >         if (unlikely(!cachep))
> >                 return NULL;
> >
> > +       objp = kfence_alloc(cachep, orig_size, flags);
> > +       if (unlikely(objp))
> > +               goto leave;
> > +
> >         cache_alloc_debugcheck_before(cachep, flags);
> >         local_irq_save(save_flags);
> >         objp = __do_cache_alloc(cachep, flags);
> > @@ -3309,6 +3319,7 @@ slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
> >         if (unlikely(slab_want_init_on_alloc(flags, cachep)) && objp)
> >                 memset(objp, 0, cachep->object_size);
> >
> > +leave:
> >         slab_post_alloc_hook(cachep, objcg, flags, 1, &objp);
> >         return objp;
> >  }
> > @@ -3414,6 +3425,11 @@ static void cache_flusharray(struct kmem_cache *cachep, struct array_cache *ac)
> >  static __always_inline void __cache_free(struct kmem_cache *cachep, void *objp,
> >                                          unsigned long caller)
> >  {
> > +       if (kfence_free(objp)) {
> > +               kmemleak_free_recursive(objp, cachep->flags);
> > +               return;
> > +       }
> > +
> >         /* Put the object into the quarantine, don't touch it for now. */
> >         if (kasan_slab_free(cachep, objp, _RET_IP_))
> >                 return;
> > @@ -3479,7 +3495,7 @@ void ___cache_free(struct kmem_cache *cachep, void *objp,
> >   */
> >  void *kmem_cache_alloc(struct kmem_cache *cachep, gfp_t flags)
> >  {
> > -       void *ret = slab_alloc(cachep, flags, _RET_IP_);
> > +       void *ret = slab_alloc(cachep, flags, cachep->object_size, _RET_IP_);
>
>
> It's kinda minor, but since we are talking about malloc fast path:
> will passing 0 instead of cachep->object_size (here and everywhere
> else) and then using cachep->object_size on the slow path if 0 is
> passed as size improve codegen?

It doesn't save us much, maybe 1 instruction based on what I'm looking
at right now. The main worry I have is that the 'orig_size' argument
is now part of slab_alloc, and changing its semantics may cause
problems in future if it's no longer just passed to kfence_alloc().
Today, we can do the 'size = size ?: cache->object_size' trick inside
kfence_alloc(), but at the cost breaking the intuitive semantics of
slab_alloc's orig_size argument for future users. Is it worth it?

Thanks,
-- Marco

Re: [PATCH RFC 04/10] mm, kfence: insert KFENCE hooks for SLAB

[Dmitry Vyukov]

On Fri, Sep 11, 2020 at 2:24 PM Marco Elver <elver@google.com> wrote:
> > > From: Alexander Potapenko <glider@google.com>
> > >
> > > Inserts KFENCE hooks into the SLAB allocator.
> > >
> > > We note the addition of the 'orig_size' argument to slab_alloc*()
> > > functions, to be able to pass the originally requested size to KFENCE.
> > > When KFENCE is disabled, there is no additional overhead, since these
> > > functions are __always_inline.
> > >
> > > Co-developed-by: Marco Elver <elver@google.com>
> > > Signed-off-by: Marco Elver <elver@google.com>
> > > Signed-off-by: Alexander Potapenko <glider@google.com>
> > > ---
> > >  mm/slab.c        | 46 ++++++++++++++++++++++++++++++++++------------
> > >  mm/slab_common.c |  6 +++++-
> > >  2 files changed, 39 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/mm/slab.c b/mm/slab.c
> > > index 3160dff6fd76..30aba06ae02b 100644
> > > --- a/mm/slab.c
> > > +++ b/mm/slab.c
> > > @@ -100,6 +100,7 @@
> > >  #include       <linux/seq_file.h>
> > >  #include       <linux/notifier.h>
> > >  #include       <linux/kallsyms.h>
> > > +#include       <linux/kfence.h>
> > >  #include       <linux/cpu.h>
> > >  #include       <linux/sysctl.h>
> > >  #include       <linux/module.h>
> > > @@ -3206,7 +3207,7 @@ static void *____cache_alloc_node(struct kmem_cache *cachep, gfp_t flags,
> > >  }
> > >
> > >  static __always_inline void *
> > > -slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
> > > +slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid, size_t orig_size,
> > >                    unsigned long caller)
> > >  {
> > >         unsigned long save_flags;
> > > @@ -3219,6 +3220,10 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
> > >         if (unlikely(!cachep))
> > >                 return NULL;
> > >
> > > +       ptr = kfence_alloc(cachep, orig_size, flags);
> > > +       if (unlikely(ptr))
> > > +               goto out_hooks;
> > > +
> > >         cache_alloc_debugcheck_before(cachep, flags);
> > >         local_irq_save(save_flags);
> > >
> > > @@ -3251,6 +3256,7 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
> > >         if (unlikely(slab_want_init_on_alloc(flags, cachep)) && ptr)
> > >                 memset(ptr, 0, cachep->object_size);
> > >
> > > +out_hooks:
> > >         slab_post_alloc_hook(cachep, objcg, flags, 1, &ptr);
> > >         return ptr;
> > >  }
> > > @@ -3288,7 +3294,7 @@ __do_cache_alloc(struct kmem_cache *cachep, gfp_t flags)
> > >  #endif /* CONFIG_NUMA */
> > >
> > >  static __always_inline void *
> > > -slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
> > > +slab_alloc(struct kmem_cache *cachep, gfp_t flags, size_t orig_size, unsigned long caller)
> > >  {
> > >         unsigned long save_flags;
> > >         void *objp;
> > > @@ -3299,6 +3305,10 @@ slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
> > >         if (unlikely(!cachep))
> > >                 return NULL;
> > >
> > > +       objp = kfence_alloc(cachep, orig_size, flags);
> > > +       if (unlikely(objp))
> > > +               goto leave;
> > > +
> > >         cache_alloc_debugcheck_before(cachep, flags);
> > >         local_irq_save(save_flags);
> > >         objp = __do_cache_alloc(cachep, flags);
> > > @@ -3309,6 +3319,7 @@ slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
> > >         if (unlikely(slab_want_init_on_alloc(flags, cachep)) && objp)
> > >                 memset(objp, 0, cachep->object_size);
> > >
> > > +leave:
> > >         slab_post_alloc_hook(cachep, objcg, flags, 1, &objp);
> > >         return objp;
> > >  }
> > > @@ -3414,6 +3425,11 @@ static void cache_flusharray(struct kmem_cache *cachep, struct array_cache *ac)
> > >  static __always_inline void __cache_free(struct kmem_cache *cachep, void *objp,
> > >                                          unsigned long caller)
> > >  {
> > > +       if (kfence_free(objp)) {
> > > +               kmemleak_free_recursive(objp, cachep->flags);
> > > +               return;
> > > +       }
> > > +
> > >         /* Put the object into the quarantine, don't touch it for now. */
> > >         if (kasan_slab_free(cachep, objp, _RET_IP_))
> > >                 return;
> > > @@ -3479,7 +3495,7 @@ void ___cache_free(struct kmem_cache *cachep, void *objp,
> > >   */
> > >  void *kmem_cache_alloc(struct kmem_cache *cachep, gfp_t flags)
> > >  {
> > > -       void *ret = slab_alloc(cachep, flags, _RET_IP_);
> > > +       void *ret = slab_alloc(cachep, flags, cachep->object_size, _RET_IP_);
> >
> >
> > It's kinda minor, but since we are talking about malloc fast path:
> > will passing 0 instead of cachep->object_size (here and everywhere
> > else) and then using cachep->object_size on the slow path if 0 is
> > passed as size improve codegen?
>
> It doesn't save us much, maybe 1 instruction based on what I'm looking
> at right now. The main worry I have is that the 'orig_size' argument
> is now part of slab_alloc, and changing its semantics may cause
> problems in future if it's no longer just passed to kfence_alloc().
> Today, we can do the 'size = size ?: cache->object_size' trick inside
> kfence_alloc(), but at the cost breaking the intuitive semantics of
> slab_alloc's orig_size argument for future users. Is it worth it?

I don't have an answer to this question. I will leave this to others.
If nobody has strong support for changing semantics, let's leave it as
is. Maybe keep in mind as potential ballast.
FWIW most likely misuse of 0 size for other future purposes should
manifest itself in a quite straightforward way.

Re: [PATCH RFC 04/10] mm, kfence: insert KFENCE hooks for SLAB

[Dmitry Vyukov]

On Fri, Sep 11, 2020 at 2:24 PM Marco Elver <elver@google.com> wrote:
> > > From: Alexander Potapenko <glider@google.com>
> > >
> > > Inserts KFENCE hooks into the SLAB allocator.
> > >
> > > We note the addition of the 'orig_size' argument to slab_alloc*()
> > > functions, to be able to pass the originally requested size to KFENCE.
> > > When KFENCE is disabled, there is no additional overhead, since these
> > > functions are __always_inline.
> > >
> > > Co-developed-by: Marco Elver <elver@google.com>
> > > Signed-off-by: Marco Elver <elver@google.com>
> > > Signed-off-by: Alexander Potapenko <glider@google.com>
> > > ---
> > >  mm/slab.c        | 46 ++++++++++++++++++++++++++++++++++------------
> > >  mm/slab_common.c |  6 +++++-
> > >  2 files changed, 39 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/mm/slab.c b/mm/slab.c
> > > index 3160dff6fd76..30aba06ae02b 100644
> > > --- a/mm/slab.c
> > > +++ b/mm/slab.c
> > > @@ -100,6 +100,7 @@
> > >  #include       <linux/seq_file.h>
> > >  #include       <linux/notifier.h>
> > >  #include       <linux/kallsyms.h>
> > > +#include       <linux/kfence.h>
> > >  #include       <linux/cpu.h>
> > >  #include       <linux/sysctl.h>
> > >  #include       <linux/module.h>
> > > @@ -3206,7 +3207,7 @@ static void *____cache_alloc_node(struct kmem_cache *cachep, gfp_t flags,
> > >  }
> > >
> > >  static __always_inline void *
> > > -slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
> > > +slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid, size_t orig_size,
> > >                    unsigned long caller)
> > >  {
> > >         unsigned long save_flags;
> > > @@ -3219,6 +3220,10 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
> > >         if (unlikely(!cachep))
> > >                 return NULL;
> > >
> > > +       ptr = kfence_alloc(cachep, orig_size, flags);
> > > +       if (unlikely(ptr))
> > > +               goto out_hooks;
> > > +
> > >         cache_alloc_debugcheck_before(cachep, flags);
> > >         local_irq_save(save_flags);
> > >
> > > @@ -3251,6 +3256,7 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
> > >         if (unlikely(slab_want_init_on_alloc(flags, cachep)) && ptr)
> > >                 memset(ptr, 0, cachep->object_size);
> > >
> > > +out_hooks:
> > >         slab_post_alloc_hook(cachep, objcg, flags, 1, &ptr);
> > >         return ptr;
> > >  }
> > > @@ -3288,7 +3294,7 @@ __do_cache_alloc(struct kmem_cache *cachep, gfp_t flags)
> > >  #endif /* CONFIG_NUMA */
> > >
> > >  static __always_inline void *
> > > -slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
> > > +slab_alloc(struct kmem_cache *cachep, gfp_t flags, size_t orig_size, unsigned long caller)
> > >  {
> > >         unsigned long save_flags;
> > >         void *objp;
> > > @@ -3299,6 +3305,10 @@ slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
> > >         if (unlikely(!cachep))
> > >                 return NULL;
> > >
> > > +       objp = kfence_alloc(cachep, orig_size, flags);
> > > +       if (unlikely(objp))
> > > +               goto leave;
> > > +
> > >         cache_alloc_debugcheck_before(cachep, flags);
> > >         local_irq_save(save_flags);
> > >         objp = __do_cache_alloc(cachep, flags);
> > > @@ -3309,6 +3319,7 @@ slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
> > >         if (unlikely(slab_want_init_on_alloc(flags, cachep)) && objp)
> > >                 memset(objp, 0, cachep->object_size);
> > >
> > > +leave:
> > >         slab_post_alloc_hook(cachep, objcg, flags, 1, &objp);
> > >         return objp;
> > >  }
> > > @@ -3414,6 +3425,11 @@ static void cache_flusharray(struct kmem_cache *cachep, struct array_cache *ac)
> > >  static __always_inline void __cache_free(struct kmem_cache *cachep, void *objp,
> > >                                          unsigned long caller)
> > >  {
> > > +       if (kfence_free(objp)) {
> > > +               kmemleak_free_recursive(objp, cachep->flags);
> > > +               return;
> > > +       }
> > > +
> > >         /* Put the object into the quarantine, don't touch it for now. */
> > >         if (kasan_slab_free(cachep, objp, _RET_IP_))
> > >                 return;
> > > @@ -3479,7 +3495,7 @@ void ___cache_free(struct kmem_cache *cachep, void *objp,
> > >   */
> > >  void *kmem_cache_alloc(struct kmem_cache *cachep, gfp_t flags)
> > >  {
> > > -       void *ret = slab_alloc(cachep, flags, _RET_IP_);
> > > +       void *ret = slab_alloc(cachep, flags, cachep->object_size, _RET_IP_);
> >
> >
> > It's kinda minor, but since we are talking about malloc fast path:
> > will passing 0 instead of cachep->object_size (here and everywhere
> > else) and then using cachep->object_size on the slow path if 0 is
> > passed as size improve codegen?
>
> It doesn't save us much, maybe 1 instruction based on what I'm looking
> at right now. The main worry I have is that the 'orig_size' argument
> is now part of slab_alloc, and changing its semantics may cause
> problems in future if it's no longer just passed to kfence_alloc().
> Today, we can do the 'size = size ?: cache->object_size' trick inside
> kfence_alloc(), but at the cost breaking the intuitive semantics of
> slab_alloc's orig_size argument for future users. Is it worth it?

I don't have an answer to this question. I will leave this to others.
If nobody has strong support for changing semantics, let's leave it as
is. Maybe keep in mind as potential ballast.
FWIW most likely misuse of 0 size for other future purposes should
manifest itself in a quite straightforward way.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH RFC 04/10] mm, kfence: insert KFENCE hooks for SLAB

[Dmitry Vyukov]

On Fri, Sep 11, 2020 at 2:24 PM Marco Elver <elver@google.com> wrote:
> > > From: Alexander Potapenko <glider@google.com>
> > >
> > > Inserts KFENCE hooks into the SLAB allocator.
> > >
> > > We note the addition of the 'orig_size' argument to slab_alloc*()
> > > functions, to be able to pass the originally requested size to KFENCE.
> > > When KFENCE is disabled, there is no additional overhead, since these
> > > functions are __always_inline.
> > >
> > > Co-developed-by: Marco Elver <elver@google.com>
> > > Signed-off-by: Marco Elver <elver@google.com>
> > > Signed-off-by: Alexander Potapenko <glider@google.com>
> > > ---
> > >  mm/slab.c        | 46 ++++++++++++++++++++++++++++++++++------------
> > >  mm/slab_common.c |  6 +++++-
> > >  2 files changed, 39 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/mm/slab.c b/mm/slab.c
> > > index 3160dff6fd76..30aba06ae02b 100644
> > > --- a/mm/slab.c
> > > +++ b/mm/slab.c
> > > @@ -100,6 +100,7 @@
> > >  #include       <linux/seq_file.h>
> > >  #include       <linux/notifier.h>
> > >  #include       <linux/kallsyms.h>
> > > +#include       <linux/kfence.h>
> > >  #include       <linux/cpu.h>
> > >  #include       <linux/sysctl.h>
> > >  #include       <linux/module.h>
> > > @@ -3206,7 +3207,7 @@ static void *____cache_alloc_node(struct kmem_cache *cachep, gfp_t flags,
> > >  }
> > >
> > >  static __always_inline void *
> > > -slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
> > > +slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid, size_t orig_size,
> > >                    unsigned long caller)
> > >  {
> > >         unsigned long save_flags;
> > > @@ -3219,6 +3220,10 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
> > >         if (unlikely(!cachep))
> > >                 return NULL;
> > >
> > > +       ptr = kfence_alloc(cachep, orig_size, flags);
> > > +       if (unlikely(ptr))
> > > +               goto out_hooks;
> > > +
> > >         cache_alloc_debugcheck_before(cachep, flags);
> > >         local_irq_save(save_flags);
> > >
> > > @@ -3251,6 +3256,7 @@ slab_alloc_node(struct kmem_cache *cachep, gfp_t flags, int nodeid,
> > >         if (unlikely(slab_want_init_on_alloc(flags, cachep)) && ptr)
> > >                 memset(ptr, 0, cachep->object_size);
> > >
> > > +out_hooks:
> > >         slab_post_alloc_hook(cachep, objcg, flags, 1, &ptr);
> > >         return ptr;
> > >  }
> > > @@ -3288,7 +3294,7 @@ __do_cache_alloc(struct kmem_cache *cachep, gfp_t flags)
> > >  #endif /* CONFIG_NUMA */
> > >
> > >  static __always_inline void *
> > > -slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
> > > +slab_alloc(struct kmem_cache *cachep, gfp_t flags, size_t orig_size, unsigned long caller)
> > >  {
> > >         unsigned long save_flags;
> > >         void *objp;
> > > @@ -3299,6 +3305,10 @@ slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
> > >         if (unlikely(!cachep))
> > >                 return NULL;
> > >
> > > +       objp = kfence_alloc(cachep, orig_size, flags);
> > > +       if (unlikely(objp))
> > > +               goto leave;
> > > +
> > >         cache_alloc_debugcheck_before(cachep, flags);
> > >         local_irq_save(save_flags);
> > >         objp = __do_cache_alloc(cachep, flags);
> > > @@ -3309,6 +3319,7 @@ slab_alloc(struct kmem_cache *cachep, gfp_t flags, unsigned long caller)
> > >         if (unlikely(slab_want_init_on_alloc(flags, cachep)) && objp)
> > >                 memset(objp, 0, cachep->object_size);
> > >
> > > +leave:
> > >         slab_post_alloc_hook(cachep, objcg, flags, 1, &objp);
> > >         return objp;
> > >  }
> > > @@ -3414,6 +3425,11 @@ static void cache_flusharray(struct kmem_cache *cachep, struct array_cache *ac)
> > >  static __always_inline void __cache_free(struct kmem_cache *cachep, void *objp,
> > >                                          unsigned long caller)
> > >  {
> > > +       if (kfence_free(objp)) {
> > > +               kmemleak_free_recursive(objp, cachep->flags);
> > > +               return;
> > > +       }
> > > +
> > >         /* Put the object into the quarantine, don't touch it for now. */
> > >         if (kasan_slab_free(cachep, objp, _RET_IP_))
> > >                 return;
> > > @@ -3479,7 +3495,7 @@ void ___cache_free(struct kmem_cache *cachep, void *objp,
> > >   */
> > >  void *kmem_cache_alloc(struct kmem_cache *cachep, gfp_t flags)
> > >  {
> > > -       void *ret = slab_alloc(cachep, flags, _RET_IP_);
> > > +       void *ret = slab_alloc(cachep, flags, cachep->object_size, _RET_IP_);
> >
> >
> > It's kinda minor, but since we are talking about malloc fast path:
> > will passing 0 instead of cachep->object_size (here and everywhere
> > else) and then using cachep->object_size on the slow path if 0 is
> > passed as size improve codegen?
>
> It doesn't save us much, maybe 1 instruction based on what I'm looking
> at right now. The main worry I have is that the 'orig_size' argument
> is now part of slab_alloc, and changing its semantics may cause
> problems in future if it's no longer just passed to kfence_alloc().
> Today, we can do the 'size = size ?: cache->object_size' trick inside
> kfence_alloc(), but at the cost breaking the intuitive semantics of
> slab_alloc's orig_size argument for future users. Is it worth it?

I don't have an answer to this question. I will leave this to others.
If nobody has strong support for changing semantics, let's leave it as
is. Maybe keep in mind as potential ballast.
FWIW most likely misuse of 0 size for other future purposes should
manifest itself in a quite straightforward way.