| 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
| ||
|
Message-ID: <CAJuCfpFXTDnqZphJ1z37KkqMt5nuOqaoWMJMRBOkA8Dnf1Dh2g@mail.gmail.com>
Date: Thu, 29 Aug 2024 09:00:37 -0700
From: Suren Baghdasaryan <surenb@...gle.com>
To: Kees Cook <kees@...nel.org>
Cc: Vlastimil Babka <vbabka@...e.cz>, Kent Overstreet <kent.overstreet@...ux.dev>,
Christoph Lameter <cl@...ux.com>, Pekka Enberg <penberg@...nel.org>, David Rientjes <rientjes@...gle.com>,
Joonsoo Kim <iamjoonsoo.kim@....com>, Andrew Morton <akpm@...ux-foundation.org>,
Roman Gushchin <roman.gushchin@...ux.dev>, Hyeonggon Yoo <42.hyeyoo@...il.com>, linux-mm@...ck.org,
"GONG, Ruiqi" <gongruiqi@...weicloud.com>, Jann Horn <jannh@...gle.com>,
Matteo Rizzo <matteorizzo@...gle.com>, jvoisin <julien.voisin@...tri.org>,
Xiu Jianfeng <xiujianfeng@...wei.com>, linux-kernel@...r.kernel.org,
linux-hardening@...r.kernel.org
Subject: Re: [PATCH 4/5] alloc_tag: Track fixed vs dynamic sized kmalloc calls
On Fri, Aug 9, 2024 at 12:33 AM Kees Cook <kees@...nel.org> wrote:
>
> For slab allocations, record whether the call site is using a fixed
> size (i.e. compile time constant) or a dynamic size. Report the results
> in /proc/allocinfo.
>
> Improvements needed:
> - examine realloc routines for needed coverage
>
> Signed-off-by: Kees Cook <kees@...nel.org>
> ---
> Cc: Suren Baghdasaryan <surenb@...gle.com>
> Cc: Kent Overstreet <kent.overstreet@...ux.dev>
> Cc: Vlastimil Babka <vbabka@...e.cz>
> Cc: Christoph Lameter <cl@...ux.com>
> Cc: Pekka Enberg <penberg@...nel.org>
> Cc: David Rientjes <rientjes@...gle.com>
> Cc: Joonsoo Kim <iamjoonsoo.kim@....com>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Roman Gushchin <roman.gushchin@...ux.dev>
> Cc: Hyeonggon Yoo <42.hyeyoo@...il.com>
> Cc: linux-mm@...ck.org
> ---
> include/linux/alloc_tag.h | 30 ++++++++++++++++++++++++++----
> include/linux/slab.h | 16 ++++++++--------
> lib/alloc_tag.c | 8 ++++++++
> mm/Kconfig | 8 ++++++++
> 4 files changed, 50 insertions(+), 12 deletions(-)
>
> diff --git a/include/linux/alloc_tag.h b/include/linux/alloc_tag.h
> index 8c61ccd161ba..f5d8c5849b82 100644
> --- a/include/linux/alloc_tag.h
> +++ b/include/linux/alloc_tag.h
> @@ -20,6 +20,19 @@ struct alloc_tag_counters {
> u64 calls;
> };
>
> +#ifdef CONFIG_SLAB_PER_SITE
> +struct alloc_meta {
> + /* 0 means non-slab, SIZE_MAX means dynamic, and everything else is fixed-size. */
> + size_t sized;
> +};
> +#define ALLOC_META_INIT(_size) { \
> + .sized = (__builtin_constant_p(_size) ? (_size) : SIZE_MAX), \
> + }
> +#else
> +struct alloc_meta { };
> +#define ALLOC_META_INIT(_size) { }
> +#endif
> +
> /*
> * An instance of this structure is created in a special ELF section at every
> * allocation callsite. At runtime, the special section is treated as
> @@ -27,6 +40,7 @@ struct alloc_tag_counters {
> */
> struct alloc_tag {
> struct codetag ct;
> + struct alloc_meta meta;
> struct alloc_tag_counters __percpu *counters;
> } __aligned(8);
>
> @@ -74,19 +88,21 @@ static inline struct alloc_tag *ct_to_alloc_tag(struct codetag *ct)
> */
> DECLARE_PER_CPU(struct alloc_tag_counters, _shared_alloc_tag);
>
> -#define DEFINE_ALLOC_TAG(_alloc_tag) \
> +#define DEFINE_ALLOC_TAG(_alloc_tag, _meta_init) \
> static struct alloc_tag _alloc_tag __used __aligned(8) \
> __section("alloc_tags") = { \
> .ct = CODE_TAG_INIT, \
> + .meta = _meta_init, \
> .counters = &_shared_alloc_tag };
>
> #else /* ARCH_NEEDS_WEAK_PER_CPU */
>
> -#define DEFINE_ALLOC_TAG(_alloc_tag) \
> +#define DEFINE_ALLOC_TAG(_alloc_tag, _meta_init) \
> static DEFINE_PER_CPU(struct alloc_tag_counters, _alloc_tag_cntr); \
> static struct alloc_tag _alloc_tag __used __aligned(8) \
> __section("alloc_tags") = { \
> .ct = CODE_TAG_INIT, \
> + .meta = _meta_init, \
> .counters = &_alloc_tag_cntr };
>
> #endif /* ARCH_NEEDS_WEAK_PER_CPU */
> @@ -191,7 +207,7 @@ static inline void alloc_tag_sub(union codetag_ref *ref, size_t bytes)
>
> #else /* CONFIG_MEM_ALLOC_PROFILING */
>
> -#define DEFINE_ALLOC_TAG(_alloc_tag)
> +#define DEFINE_ALLOC_TAG(_alloc_tag, _meta_init)
> static inline bool mem_alloc_profiling_enabled(void) { return false; }
> static inline void alloc_tag_add(union codetag_ref *ref, struct alloc_tag *tag,
> size_t bytes) {}
> @@ -210,8 +226,14 @@ static inline void alloc_tag_sub(union codetag_ref *ref, size_t bytes) {}
>
> #define alloc_hooks(_do_alloc) \
> ({ \
> - DEFINE_ALLOC_TAG(_alloc_tag); \
> + DEFINE_ALLOC_TAG(_alloc_tag, { }); \
> alloc_hooks_tag(&_alloc_tag, _do_alloc); \
> })
>
> +#define alloc_sized_hooks(_do_alloc, _size, ...) \
> +({ \
> + DEFINE_ALLOC_TAG(_alloc_tag, ALLOC_META_INIT(_size)); \
> + alloc_hooks_tag(&_alloc_tag, _do_alloc(_size, __VA_ARGS__)); \
> +})
> +
> #endif /* _LINUX_ALLOC_TAG_H */
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 86cb61a0102c..314d24c79e05 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -541,7 +541,7 @@ static_assert(PAGE_SHIFT <= 20);
> */
> void *kmem_cache_alloc_noprof(struct kmem_cache *cachep,
> gfp_t flags) __assume_slab_alignment __malloc;
> -#define kmem_cache_alloc(...) alloc_hooks(kmem_cache_alloc_noprof(__VA_ARGS__))
> +#define kmem_cache_alloc(...) alloc_hooks(kmem_cache_alloc_noprof(__VA_ARGS__))
nit: seems like an unnecessary churn.
>
> void *kmem_cache_alloc_lru_noprof(struct kmem_cache *s, struct list_lru *lru,
> gfp_t gfpflags) __assume_slab_alignment __malloc;
> @@ -685,7 +685,7 @@ static __always_inline __alloc_size(1) void *kmalloc_noprof(size_t size, gfp_t f
> }
> return __kmalloc_noprof(size, flags);
> }
> -#define kmalloc(...) alloc_hooks(kmalloc_noprof(__VA_ARGS__))
> +#define kmalloc(size, ...) alloc_sized_hooks(kmalloc_noprof, size, __VA_ARGS__)
>
> #define kmem_buckets_alloc(_b, _size, _flags) \
> alloc_hooks(__kmalloc_node_noprof(PASS_BUCKET_PARAMS(_size, _b), _flags, NUMA_NO_NODE))
> @@ -708,7 +708,7 @@ static __always_inline __alloc_size(1) void *kmalloc_node_noprof(size_t size, gf
> }
> return __kmalloc_node_noprof(PASS_BUCKET_PARAMS(size, NULL), flags, node);
> }
> -#define kmalloc_node(...) alloc_hooks(kmalloc_node_noprof(__VA_ARGS__))
> +#define kmalloc_node(size, ...) alloc_sized_hooks(kmalloc_node_noprof, size, __VA_ARGS__)
>
> /**
> * kmalloc_array - allocate memory for an array.
> @@ -726,7 +726,7 @@ static inline __alloc_size(1, 2) void *kmalloc_array_noprof(size_t n, size_t siz
> return kmalloc_noprof(bytes, flags);
> return kmalloc_noprof(bytes, flags);
> }
> -#define kmalloc_array(...) alloc_hooks(kmalloc_array_noprof(__VA_ARGS__))
> +#define kmalloc_array(...) alloc_hooks(kmalloc_array_noprof(__VA_ARGS__))
ditto.
>
> /**
> * krealloc_array - reallocate memory for an array.
> @@ -761,8 +761,8 @@ void *__kmalloc_node_track_caller_noprof(DECL_BUCKET_PARAMS(size, b), gfp_t flag
> unsigned long caller) __alloc_size(1);
> #define kmalloc_node_track_caller_noprof(size, flags, node, caller) \
> __kmalloc_node_track_caller_noprof(PASS_BUCKET_PARAMS(size, NULL), flags, node, caller)
> -#define kmalloc_node_track_caller(...) \
> - alloc_hooks(kmalloc_node_track_caller_noprof(__VA_ARGS__, _RET_IP_))
> +#define kmalloc_node_track_caller(size, ...) \
> + alloc_sized_hooks(kmalloc_node_track_caller_noprof, size, __VA_ARGS__, _RET_IP_)
>
> /*
> * kmalloc_track_caller is a special version of kmalloc that records the
> @@ -807,13 +807,13 @@ static inline __alloc_size(1) void *kzalloc_noprof(size_t size, gfp_t flags)
> {
> return kmalloc_noprof(size, flags | __GFP_ZERO);
> }
> -#define kzalloc(...) alloc_hooks(kzalloc_noprof(__VA_ARGS__))
> +#define kzalloc(size, ...) alloc_sized_hooks(kzalloc_noprof, size, __VA_ARGS__)
> #define kzalloc_node(_size, _flags, _node) kmalloc_node(_size, (_flags)|__GFP_ZERO, _node)
>
> void *__kvmalloc_node_noprof(DECL_BUCKET_PARAMS(size, b), gfp_t flags, int node) __alloc_size(1);
> #define kvmalloc_node_noprof(size, flags, node) \
> __kvmalloc_node_noprof(PASS_BUCKET_PARAMS(size, NULL), flags, node)
> -#define kvmalloc_node(...) alloc_hooks(kvmalloc_node_noprof(__VA_ARGS__))
> +#define kvmalloc_node(size, ...) alloc_sized_hooks(kvmalloc_node_noprof, size, __VA_ARGS__)
>
> #define kvmalloc(_size, _flags) kvmalloc_node(_size, _flags, NUMA_NO_NODE)
> #define kvmalloc_noprof(_size, _flags) kvmalloc_node_noprof(_size, _flags, NUMA_NO_NODE)
> diff --git a/lib/alloc_tag.c b/lib/alloc_tag.c
> index 81e5f9a70f22..6d2cb72bf269 100644
> --- a/lib/alloc_tag.c
> +++ b/lib/alloc_tag.c
> @@ -78,6 +78,14 @@ static void alloc_tag_to_text(struct seq_buf *out, struct codetag *ct)
>
> seq_buf_printf(out, "%12lli %8llu ", bytes, counter.calls);
> codetag_to_text(out, ct);
> +#ifdef CONFIG_SLAB_PER_SITE
> + seq_buf_putc(out, ' ');
> + seq_buf_printf(out, "size:%s(%zu) slab:%s",
> + tag->meta.sized == 0 ? "non-slab" :
"non-slab" term sounds overly specific and we might extend this to
some other allocations as well in the future. I would suggest
"unknown" instead.
> + tag->meta.sized == SIZE_MAX ? "dynamic" : "fixed",
> + tag->meta.sized == SIZE_MAX ? 0 : tag->meta.sized,
> + tag->meta.cache ? "ready" : "unused");
I don't see "struct alloc_meta" having a "cache" member...
Since you are changing the format of this file, you want to also bump
up the file version inside print_allocinfo_header().
> +#endif
> seq_buf_putc(out, ' ');
> seq_buf_putc(out, '\n');
> }
> diff --git a/mm/Kconfig b/mm/Kconfig
> index b72e7d040f78..855c63c3270d 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -296,6 +296,14 @@ config SLAB_BUCKETS
>
> If unsure, say Y.
>
> +config SLAB_PER_SITE
> + bool "Separate slab allocations by call size"
> + depends on !SLUB_TINY
> + default SLAB_FREELIST_HARDENED
> + select SLAB_BUCKETS
> + help
> + Track sizes of kmalloc() call sites.
> +
> config SLUB_STATS
> default n
> bool "Enable performance statistics"
> --
> 2.34.1
>
Powered by blists - more mailing lists