2021-03-05 00:57:43

by Marco Elver

[permalink] [raw]
Subject: [PATCH mm] kfence, slab: fix cache_alloc_debugcheck_after() for bulk allocations

cache_alloc_debugcheck_after() performs checks on an object, including
adjusting the returned pointer. None of this should apply to KFENCE
objects. While for non-bulk allocations, the checks are skipped when we
allocate via KFENCE, for bulk allocations cache_alloc_debugcheck_after()
is called via cache_alloc_debugcheck_after_bulk().

Fix it by skipping cache_alloc_debugcheck_after() for KFENCE objects.

Signed-off-by: Marco Elver <[email protected]>
---
mm/slab.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/slab.c b/mm/slab.c
index 51fd424e0d6d..ae651bf540b7 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -2992,7 +2992,7 @@ static void *cache_alloc_debugcheck_after(struct kmem_cache *cachep,
gfp_t flags, void *objp, unsigned long caller)
{
WARN_ON_ONCE(cachep->ctor && (flags & __GFP_ZERO));
- if (!objp)
+ if (!objp || is_kfence_address(objp))
return objp;
if (cachep->flags & SLAB_POISON) {
check_poison_obj(cachep, objp);
--
2.30.1.766.gb4fecdf3b7-goog


2021-03-05 00:57:55

by Alexander Potapenko

[permalink] [raw]
Subject: Re: [PATCH mm] kfence, slab: fix cache_alloc_debugcheck_after() for bulk allocations

On Thu, Mar 4, 2021 at 9:53 PM Marco Elver <[email protected]> wrote:
>
> cache_alloc_debugcheck_after() performs checks on an object, including
> adjusting the returned pointer. None of this should apply to KFENCE
> objects. While for non-bulk allocations, the checks are skipped when we
> allocate via KFENCE, for bulk allocations cache_alloc_debugcheck_after()
> is called via cache_alloc_debugcheck_after_bulk().

@Andrew, is this code used by anyone?
As far as I understand, it cannot be enabled by any config option, so
nobody really tests it.
If it is still needed, shall we promote #if DEBUGs in slab.c to a
separate config option, or maybe this code can be safely removed?


Alex

2021-03-05 01:33:16

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH mm] kfence, slab: fix cache_alloc_debugcheck_after() for bulk allocations

On Thu, 4 Mar 2021 22:05:48 +0100 Alexander Potapenko <[email protected]> wrote:

> On Thu, Mar 4, 2021 at 9:53 PM Marco Elver <[email protected]> wrote:
> >
> > cache_alloc_debugcheck_after() performs checks on an object, including
> > adjusting the returned pointer. None of this should apply to KFENCE
> > objects. While for non-bulk allocations, the checks are skipped when we
> > allocate via KFENCE, for bulk allocations cache_alloc_debugcheck_after()
> > is called via cache_alloc_debugcheck_after_bulk().
>
> @Andrew, is this code used by anyone?
> As far as I understand, it cannot be enabled by any config option, so
> nobody really tests it.
> If it is still needed, shall we promote #if DEBUGs in slab.c to a
> separate config option, or maybe this code can be safely removed?

It's all used:

#ifdef CONFIG_DEBUG_SLAB
#define DEBUG 1
#define STATS 1
#define FORCED_DEBUG 1
#else
#define DEBUG 0
#define STATS 0
#define FORCED_DEBUG 0
#endif

2021-03-05 08:59:44

by Alexander Potapenko

[permalink] [raw]
Subject: Re: [PATCH mm] kfence, slab: fix cache_alloc_debugcheck_after() for bulk allocations

On Fri, Mar 5, 2021 at 2:31 AM Andrew Morton <[email protected]> wrote:
>
> On Thu, 4 Mar 2021 22:05:48 +0100 Alexander Potapenko <[email protected]> wrote:
>
> > On Thu, Mar 4, 2021 at 9:53 PM Marco Elver <[email protected]> wrote:
> > >
> > > cache_alloc_debugcheck_after() performs checks on an object, including
> > > adjusting the returned pointer. None of this should apply to KFENCE
> > > objects. While for non-bulk allocations, the checks are skipped when we
> > > allocate via KFENCE, for bulk allocations cache_alloc_debugcheck_after()
> > > is called via cache_alloc_debugcheck_after_bulk().
> >
> > @Andrew, is this code used by anyone?
> > As far as I understand, it cannot be enabled by any config option, so
> > nobody really tests it.
> > If it is still needed, shall we promote #if DEBUGs in slab.c to a
> > separate config option, or maybe this code can be safely removed?
>
> It's all used:

Got it, sorry for being too hasty!