On Tue, Jun 02, 2020 at 04:15:18PM +0200, Vlastimil Babka wrote:
> SLUB_DEBUG creates several files under /sys/kernel/slab/<cache>/ that can be
> read to check if the respective debugging options are enabled for given cache.
> Some options, namely sanity_checks, trace, and failslab can be also enabled and
> disabled at runtime by writing into the files.
>
> The runtime toggling is racy. Some options disable __CMPXCHG_DOUBLE when
> enabled, which means that in case of concurrent allocations, some can still use
> __CMPXCHG_DOUBLE and some not, leading to potential corruption. The s->flags
> field is also not updated or checked atomically. The simplest solution is to
> remove the runtime toggling. The extended slub_debug boot parameter syntax
> introduced by earlier patch should allow to fine-tune the debugging
> configuration during boot with same granularity.
>
> Signed-off-by: Vlastimil Babka <vbabka@xxxxxxx>
Acked-by: Roman Gushchin <guro@xxxxxx>
Thanks!
> ---
> Documentation/vm/slub.rst | 7 ++---
> mm/slub.c | 62 ++-------------------------------------
> 2 files changed, 5 insertions(+), 64 deletions(-)
>
> diff --git a/Documentation/vm/slub.rst b/Documentation/vm/slub.rst
> index 61805e984a0d..f240292589bd 100644
> --- a/Documentation/vm/slub.rst
> +++ b/Documentation/vm/slub.rst
> @@ -115,11 +115,8 @@ If the file contains 1, the option is enabled, 0 means disabled. The debug
> T trace
> A failslab
>
> -The sanity_checks, trace and failslab files are writable, so writing 1 or 0
> -will enable or disable the option at runtime. The writes to trace and failslab
> -may return -EINVAL if the cache is subject to slab merging. Careful with
> -tracing: It may spew out lots of information and never stop if used on the
> -wrong slab.
> +Careful with tracing: It may spew out lots of information and never stop if
> +used on the wrong slab.
>
> Slab merging
> ============
> diff --git a/mm/slub.c b/mm/slub.c
> index 58c1e9e7b3b3..38dd6f3ebb04 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -5056,20 +5056,6 @@ static ssize_t show_slab_objects(struct kmem_cache *s,
> return x + sprintf(buf + x, "\n");
> }
>
> -#ifdef CONFIG_SLUB_DEBUG
> -static int any_slab_objects(struct kmem_cache *s)
> -{
> - int node;
> - struct kmem_cache_node *n;
> -
> - for_each_kmem_cache_node(s, node, n)
> - if (atomic_long_read(&n->total_objects))
> - return 1;
> -
> - return 0;
> -}
> -#endif
> -
> #define to_slab_attr(n) container_of(n, struct slab_attribute, attr)
> #define to_slab(n) container_of(n, struct kmem_cache, kobj)
>
> @@ -5291,43 +5277,13 @@ static ssize_t sanity_checks_show(struct kmem_cache *s, char *buf)
> {
> return sprintf(buf, "%d\n", !!(s->flags & SLAB_CONSISTENCY_CHECKS));
> }
> -
> -static ssize_t sanity_checks_store(struct kmem_cache *s,
> - const char *buf, size_t length)
> -{
> - s->flags &= ~SLAB_CONSISTENCY_CHECKS;
> - if (buf[0] == '1') {
> - s->flags &= ~__CMPXCHG_DOUBLE;
> - s->flags |= SLAB_CONSISTENCY_CHECKS;
> - }
> - return length;
> -}
> -SLAB_ATTR(sanity_checks);
> +SLAB_ATTR_RO(sanity_checks);
>
> static ssize_t trace_show(struct kmem_cache *s, char *buf)
> {
> return sprintf(buf, "%d\n", !!(s->flags & SLAB_TRACE));
> }
> -
> -static ssize_t trace_store(struct kmem_cache *s, const char *buf,
> - size_t length)
> -{
> - /*
> - * Tracing a merged cache is going to give confusing results
> - * as well as cause other issues like converting a mergeable
> - * cache into an umergeable one.
> - */
> - if (s->refcount > 1)
> - return -EINVAL;
> -
> - s->flags &= ~SLAB_TRACE;
> - if (buf[0] == '1') {
> - s->flags &= ~__CMPXCHG_DOUBLE;
> - s->flags |= SLAB_TRACE;
> - }
> - return length;
> -}
> -SLAB_ATTR(trace);
> +SLAB_ATTR_RO(trace);
>
> static ssize_t red_zone_show(struct kmem_cache *s, char *buf)
> {
> @@ -5391,19 +5347,7 @@ static ssize_t failslab_show(struct kmem_cache *s, char *buf)
> {
> return sprintf(buf, "%d\n", !!(s->flags & SLAB_FAILSLAB));
> }
> -
> -static ssize_t failslab_store(struct kmem_cache *s, const char *buf,
> - size_t length)
> -{
> - if (s->refcount > 1)
> - return -EINVAL;
> -
> - s->flags &= ~SLAB_FAILSLAB;
> - if (buf[0] == '1')
> - s->flags |= SLAB_FAILSLAB;
> - return length;
> -}
> -SLAB_ATTR(failslab);
> +SLAB_ATTR_RO(failslab);
> #endif
>
> static ssize_t shrink_show(struct kmem_cache *s, char *buf)
> --
> 2.26.2
>
>
