Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] slab: fix a crash by reading /proc/slab_allocators
To:     "Tobin C. Harding" <me@tobin.cc>
Cc:     akpm@linux-foundation.org, cl@linux.com, penberg@kernel.org,
        rientjes@google.com, iamjoonsoo.kim@lge.com, tj@kernel.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org
References: <20190406225901.35465-1-cai@lca.pw>
 <20190408015917.GA633@eros.localdomain>
From:   Qian Cai <cai@lca.pw>
Message-ID: <57f7ef12-9330-a535-64c9-6bf17382d5fc@lca.pw>
Date:   Sun, 7 Apr 2019 22:18:45 -0400
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:60.0)
 Gecko/20100101 Thunderbird/60.3.3
MIME-Version: 1.0
In-Reply-To: <20190408015917.GA633@eros.localdomain>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 4/7/19 9:59 PM, Tobin C. Harding wrote:
> On Sat, Apr 06, 2019 at 06:59:01PM -0400, Qian Cai wrote:
>> The commit 510ded33e075 ("slab: implement slab_root_caches list")
>> changes the name of the list node within "struct kmem_cache" from
>> "list" to "root_caches_node"
> 
> Are you sure? It looks to me like it adds a member to the memcg_cache_array
> 
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index a0cc7a77cda2..af1a5bef80f4 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -556,6 +556,8 @@ struct memcg_cache_array {
>   *             used to index child cachces during allocation and cleared
>   *             early during shutdown.
>   * 
> + * @root_caches_node: List node for slab_root_caches list.
> + * 
>   * @children:  List of all child caches.  While the child caches are also
>   *             reachable through @memcg_caches, a child cache remains on
>   *             this list until it is actually destroyed.
> @@ -573,6 +575,7 @@ struct memcg_cache_params {
>         union { 
>                 struct {
>                         struct memcg_cache_array __rcu *memcg_caches;
> +                       struct list_head __root_caches_node;
>                         struct list_head children;
>                 };
> 
> And then defines 'root_caches_node' to be 'memcg_params.__root_caches_node'
> if we have CONFIG_MEMCG otherwise defines 'root_caches_node' to be 'list'
> 
> 
>> but leaks_show() still use the "list"
> 
> I believe it should since 'list' is used to add to slab_caches list.

See the offensive commit 510ded33e075 which changed those.

@@ -1136,12 +1146,12 @@ static void print_slabinfo_header(struct seq_file *m)
 void *slab_start(struct seq_file *m, loff_t *pos)
 {
        mutex_lock(&slab_mutex);
-       return seq_list_start(&slab_caches, *pos);
+       return seq_list_start(&slab_root_caches, *pos);
 }

 void *slab_next(struct seq_file *m, void *p, loff_t *pos)
 {
-       return seq_list_next(p, &slab_caches, pos);
+       return seq_list_next(p, &slab_root_caches, pos);
 }

and then memcg_link_cache() does,

if (is_root_cache(s)) {
	list_add(&s->root_caches_node, &slab_root_caches);

memcg_unlink_cache() does,

if (is_root_cache(s)) {
	list_del(&s->root_caches_node);

It also changed /proc/slabinfo but forgot to change /proc/slab_allocators.

@@ -1193,12 +1203,11 @@ static void cache_show(struct kmem_cache *s, struct
seq_file *m)

 static int slab_show(struct seq_file *m, void *p)
 {
-       struct kmem_cache *s = list_entry(p, struct kmem_cache, list);
+       struct kmem_cache *s = list_entry(p, struct kmem_cache, root_caches_node);

> 
>> which causes a crash when reading /proc/slab_allocators.
> 
> I was unable to reproduce this crash, I built with
> 
> # CONFIG_MEMCG is not set
> CONFIG_SLAB=y
> CONFIG_SLAB_MERGE_DEFAULT=y
> CONFIG_SLAB_FREELIST_RANDOM=y
> CONFIG_DEBUG_SLAB=y
> CONFIG_DEBUG_SLAB_LEAK=y
> 
> I then booted in Qemu and successfully ran 
> $ cat slab_allocators
> 
> Perhaps you could post your config?

Yes, it won't be reproducible without CONFIG_MEMCG=y, because it has,

/* If !memcg, all caches are root. */
#define slab_root_caches       slab_caches
#define root_caches_node       list

Anyway,

https://git.sr.ht/~cai/linux-debug/blob/master/config