Re: [RFC PATCH 4/5] mm, slub: make remaining slub_debug related attributes read-only

[Roman Gushchin]

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
> 
>