On Tue, Jun 02, 2020 at 04:15:16PM +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. > The options can be also toggled at runtime by writing into the files. Some of > those, namely red_zone, poison, and store_user can be toggled only when no > objects yet exist in the cache. > > Vijayanand reports [1] that there is a problem with freelist randomization if > changing the debugging option's state results in different number of objects > per page, and the random sequence cache needs thus needs to be recomputed. > > However, another problem is that the check for "no objects yet exist in the > cache" is racy, as noted by Jann [2] and fixing that would add overhead or > otherwise complicate the allocation/freeing paths. Thus it would be much > simpler just to remove the runtime toggling support. The documentation > describes it's "In case you forgot to enable debugging on the kernel command > line", but the neccessity of having no objects limits its usefulness anyway for > many caches. > > Vijayanand describes an use case [3] where debugging is enabled for all but > zram caches for memory overhead reasons, and using the runtime toggles was the > only way to achieve such configuration. After the previous patch it's now > possible to do that directly from the kernel boot option, so we can remove the > dangerous runtime toggles by making the /sys attribute files read-only. > > While updating it, also improve the documentation of the debugging /sys files. > > [1] https://lkml.kernel.org/r/1580379523-32272-1-git-send-email-vjitta@xxxxxxxxxxxxxx > [2] https://lore.kernel.org/r/CAG48ez31PP--h6_FzVyfJ4H86QYczAFPdxtJHUEEan+7VJETAQ@xxxxxxxxxxxxxx > [3] https://lore.kernel.org/r/1383cd32-1ddc-4dac-b5f8-9c42282fa81c@xxxxxxxxxxxxxx > > Reported-by: Vijayanand Jitta <vjitta@xxxxxxxxxxxxxx> > Reported-by: Jann Horn <jannh@xxxxxxxxxx> > Signed-off-by: Vlastimil Babka <vbabka@xxxxxxx> Acked-by: Roman Gushchin <guro@xxxxxx> Thanks! > --- > Documentation/vm/slub.rst | 28 ++++++++++++++---------- > mm/slub.c | 46 +++------------------------------------ > 2 files changed, 20 insertions(+), 54 deletions(-) > > diff --git a/Documentation/vm/slub.rst b/Documentation/vm/slub.rst > index f1154f707e06..61805e984a0d 100644 > --- a/Documentation/vm/slub.rst > +++ b/Documentation/vm/slub.rst > @@ -100,20 +100,26 @@ except some that are deemed too performance critical and don't need to be > > slub_debug=FZ;-,zs_handle,zspage > > -In case you forgot to enable debugging on the kernel command line: It is > -possible to enable debugging manually when the kernel is up. Look at the > -contents of:: > +The state of each debug option for a slab can be found in the respective files > +under:: > > /sys/kernel/slab/<slab name>/ > > -Look at the writable files. Writing 1 to them will enable the > -corresponding debug option. All options can be set on a slab that does > -not contain objects. If the slab already contains objects then sanity checks > -and tracing may only be enabled. The other options may cause the realignment > -of objects. > - > -Careful with tracing: It may spew out lots of information and never stop if > -used on the wrong slab. > +If the file contains 1, the option is enabled, 0 means disabled. The debug > +options from the ``slub_debug`` parameter translate to the following files:: > + > + F sanity_checks > + Z red_zone > + P poison > + U store_user > + 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. > > Slab merging > ============ > diff --git a/mm/slub.c b/mm/slub.c > index 47430aad9a65..ac198202dbb0 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -5351,61 +5351,21 @@ static ssize_t red_zone_show(struct kmem_cache *s, char *buf) > return sprintf(buf, "%d\n", !!(s->flags & SLAB_RED_ZONE)); > } > > -static ssize_t red_zone_store(struct kmem_cache *s, > - const char *buf, size_t length) > -{ > - if (any_slab_objects(s)) > - return -EBUSY; > - > - s->flags &= ~SLAB_RED_ZONE; > - if (buf[0] == '1') { > - s->flags |= SLAB_RED_ZONE; > - } > - calculate_sizes(s, -1); > - return length; > -} > -SLAB_ATTR(red_zone); > +SLAB_ATTR_RO(red_zone); > > static ssize_t poison_show(struct kmem_cache *s, char *buf) > { > return sprintf(buf, "%d\n", !!(s->flags & SLAB_POISON)); > } > > -static ssize_t poison_store(struct kmem_cache *s, > - const char *buf, size_t length) > -{ > - if (any_slab_objects(s)) > - return -EBUSY; > - > - s->flags &= ~SLAB_POISON; > - if (buf[0] == '1') { > - s->flags |= SLAB_POISON; > - } > - calculate_sizes(s, -1); > - return length; > -} > -SLAB_ATTR(poison); > +SLAB_ATTR_RO(poison); > > static ssize_t store_user_show(struct kmem_cache *s, char *buf) > { > return sprintf(buf, "%d\n", !!(s->flags & SLAB_STORE_USER)); > } > > -static ssize_t store_user_store(struct kmem_cache *s, > - const char *buf, size_t length) > -{ > - if (any_slab_objects(s)) > - return -EBUSY; > - > - s->flags &= ~SLAB_STORE_USER; > - if (buf[0] == '1') { > - s->flags &= ~__CMPXCHG_DOUBLE; > - s->flags |= SLAB_STORE_USER; > - } > - calculate_sizes(s, -1); > - return length; > -} > -SLAB_ATTR(store_user); > +SLAB_ATTR_RO(store_user); > > static ssize_t validate_show(struct kmem_cache *s, char *buf) > { > -- > 2.26.2 > >
