On Fri, Aug 26, 2022 at 11:09:11AM +0200, Vlastimil Babka wrote:
> For SLAB_TYPESAFE_BY_RCU caches we use call_rcu to perform empty slab
> freeing. The rcu callback rcu_free_slab() calls __free_slab() that
> currently includes checking the slab consistency for caches with
> SLAB_CONSISTENCY_CHECKS flags. This check needs the slab->objects field
> to be intact.
>
> Because in the next patch we want to allow rcu_head in struct slab to
> become larger in debug configurations and thus potentially overwrite
> more fields through a union than slab_list, we want to limit the fields
> used in rcu_free_slab(). Thus move the consistency checks to
> free_slab() before call_rcu(). This can be done safely even for
> SLAB_TYPESAFE_BY_RCU caches where accesses to the objects can still
> occur after freeing them.
>
> As a result, only the slab->slab_cache field has to be physically
> separate from rcu_head for the freeing callback to work. We also save
> some cycles in the rcu callback for caches with consistency checks
> enabled.
>
> Signed-off-by: Vlastimil Babka <vbabka@xxxxxxx>
> ---
> mm/slub.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index 862dbd9af4f5..d86be1b0d09f 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -2036,14 +2036,6 @@ static void __free_slab(struct kmem_cache *s, struct slab *slab)
> int order = folio_order(folio);
> int pages = 1 << order;
>
> - if (kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS)) {
> - void *p;
> -
> - slab_pad_check(s, slab);
> - for_each_object(p, s, slab_address(slab), slab->objects)
> - check_object(s, slab, p, SLUB_RED_INACTIVE);
> - }
> -
> __slab_clear_pfmemalloc(slab);
> __folio_clear_slab(folio);
> folio->mapping = NULL;
> @@ -2062,9 +2054,17 @@ static void rcu_free_slab(struct rcu_head *h)
>
> static void free_slab(struct kmem_cache *s, struct slab *slab)
> {
> - if (unlikely(s->flags & SLAB_TYPESAFE_BY_RCU)) {
> + if (kmem_cache_debug_flags(s, SLAB_CONSISTENCY_CHECKS)) {
> + void *p;
> +
> + slab_pad_check(s, slab);
> + for_each_object(p, s, slab_address(slab), slab->objects)
> + check_object(s, slab, p, SLUB_RED_INACTIVE);
> + }
> +
> + if (unlikely(s->flags & SLAB_TYPESAFE_BY_RCU))
> call_rcu(&slab->rcu_head, rcu_free_slab);
> - } else
> + else
> __free_slab(s, slab);
> }
So this allows corrupting 'counters' with patch 2.
The code looks still safe to me as we do only
redzone checking for SLAB_TYPESAFE_RCU caches.
Reviewed-by: Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx>
> --
> 2.37.2
>
--
Thanks,
Hyeonggon
