linux-kernel - Re: [PATCH] slub/slab: fix kmemleak didn't work on some case

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Date:	Mon, 8 Jun 2015 11:03:49 +0100
From:	Catalin Marinas <catalin.marinas@....com>
To:	"Liu, XinwuX" <xinwux.liu@...el.com>
Cc:	"cl@...ux-foundation.org" <cl@...ux-foundation.org>,
	"penberg@...nel.org" <penberg@...nel.org>,
	"mpm@...enic.com" <mpm@...enic.com>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"yanmin_zhang@...ux.intel.com" <yanmin_zhang@...ux.intel.com>,
	"He, Bo" <bo.he@...el.com>, "Chen, Lin Z" <lin.z.chen@...el.com>
Subject: Re: [PATCH] slub/slab: fix kmemleak didn't work on some case

On Mon, Jun 08, 2015 at 06:14:32AM +0100, Liu, XinwuX wrote:
> when kernel uses kmalloc to allocate memory, slub/slab will find
> a suitable kmem_cache. Ususally the cache's object size is often
> greater than requested size. There is unused space which contains
> dirty data. These dirty data might have pointers pointing to a block
> of leaked memory. Kernel wouldn't consider this memory as leaked when
> scanning kmemleak object.
> 
> The patch fixes it by clearing the unused memory.

In general, I'm not bothered about this. We may miss a leak or two but
in my experience they eventually show up at some point. Have you seen
any real leaks not being reported because of this? Note that we already
have a lot of non-pointer data that is scanned by kmemleak (it can't
distinguish which members are pointers in a data structure).

> mm/slab.c | 22 +++++++++++++++++++++-
> mm/slub.c | 35 +++++++++++++++++++++++++++++++++++
> 2 files changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/slab.c b/mm/slab.c
> index 7eb38dd..ef25e7d 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -3423,6 +3423,12 @@ kmem_cache_alloc_trace(struct kmem_cache *cachep, gfp_t flags, size_t size)
>                 ret = slab_alloc(cachep, flags, _RET_IP_);
> +#ifdef CONFIG_DEBUG_KMEMLEAK
> +             int delta = cachep->object_size - size;
> +
> +             if (ret && likely(!(flags & __GFP_ZERO)) && (delta > 0))
> +                             memset((void *)((char *)ret + size), 0, delta);
> +#endif

On the implementation side, there is too much code duplication. I would
rather add something like the kmemleak_erase(), e.g.
kmemleak_erase_range(addr, object_size, actual_size) which is an empty
static inline when !CONFIG_DEBUG_KMEMLEAK.

Kmemleak already has an API for similar cases, kmemleak_scan_area().
While this allocates an extra structure, it could be adapted to only
change some of the object properties. However, the rb tree lookup is
probably still slower than a memset().

-- 
Catalin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/