struct memcg_cache_params has a union. Different parts of this union
are used for root and non-root caches. A part with destroying work is
used only for non-root caches.
I fixed the same problem in another place v3.9-rc1-16204-gf101a94, but
didn't notice this one.
Cc: Glauber Costa <[email protected]>
Cc: Johannes Weiner <[email protected]>
Cc: Michal Hocko <[email protected]>
Cc: Balbir Singh <[email protected]>
Cc: KAMEZAWA Hiroyuki <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Konstantin Khlebnikov <[email protected]>
Cc: <[email protected]> [3.9.x]
Signed-off-by: Andrey Vagin <[email protected]>
---
mm/memcontrol.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index c290a1c..c5792a5 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3195,11 +3195,11 @@ int memcg_register_cache(struct mem_cgroup *memcg, struct kmem_cache *s,
if (!s->memcg_params)
return -ENOMEM;
- INIT_WORK(&s->memcg_params->destroy,
- kmem_cache_destroy_work_func);
if (memcg) {
s->memcg_params->memcg = memcg;
s->memcg_params->root_cache = root_cache;
+ INIT_WORK(&s->memcg_params->destroy,
+ kmem_cache_destroy_work_func);
} else
s->memcg_params->is_root_cache = true;
--
1.8.3.1
On Mon, 5 Aug 2013 20:09:40 +0400 Andrey Vagin <[email protected]> wrote:
> struct memcg_cache_params has a union. Different parts of this union
> are used for root and non-root caches. A part with destroying work is
> used only for non-root caches.
>
> I fixed the same problem in another place v3.9-rc1-16204-gf101a94, but
> didn't notice this one.
>
> Cc: <[email protected]> [3.9.x]
hm, why the cc:stable?
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3195,11 +3195,11 @@ int memcg_register_cache(struct mem_cgroup *memcg, struct kmem_cache *s,
> if (!s->memcg_params)
> return -ENOMEM;
>
> - INIT_WORK(&s->memcg_params->destroy,
> - kmem_cache_destroy_work_func);
> if (memcg) {
> s->memcg_params->memcg = memcg;
> s->memcg_params->root_cache = root_cache;
> + INIT_WORK(&s->memcg_params->destroy,
> + kmem_cache_destroy_work_func);
> } else
> s->memcg_params->is_root_cache = true;
So the bug here is that we'll scribble on some entries in
memcg_caches[]. Those scribbles may or may not be within the part of
that array which is actually used. If there's code which expects
memcg_caches[] entries to be zeroed at initialisation then yes, we have
a problem.
But I rather doubt whether this bug was causing runtime problems?
Presently memcg_register_cache() allocates too much memory for the
memcg_caches[] array. If that was fixed then this INIT_WORK() might
scribble into unknown memory, which is of course serious.
On Mon, Aug 05, 2013 at 01:05:30PM -0700, Andrew Morton wrote:
> On Mon, 5 Aug 2013 20:09:40 +0400 Andrey Vagin <[email protected]> wrote:
>
> > struct memcg_cache_params has a union. Different parts of this union
> > are used for root and non-root caches. A part with destroying work is
> > used only for non-root caches.
> >
> > I fixed the same problem in another place v3.9-rc1-16204-gf101a94, but
> > didn't notice this one.
> >
> > Cc: <[email protected]> [3.9.x]
>
> hm, why the cc:stable?
Because this patch fixes the kernel panic:
[ 46.848187] BUG: unable to handle kernel paging request at 000000fffffffeb8
[ 46.849026] IP: [<ffffffff811a484c>] kmem_cache_destroy_memcg_children+0x6c/0xc0
[ 46.849092] PGD 0
[ 46.849092] Oops: 0000 [#1] SMP
[ 46.849092] Modules linked in: vzethdev vznetdev pio_direct pfmt_raw pfmt_ploop1 ploop simfs ipt_MASQUERADE nf_conntrack_netbios_ns nf_conntrack_broadcast ip6table_mangle ip6t_REJECT nf_conntrack_ipv6 nf_defrag_ipv6 iptable_nat nf_nat_ipv4 nf_nat iptable_mangle nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack ebtable_filter ebtables ip6table_filter ip6_tables vzevent microcode joydev pcspkr virtio_balloon i2c_piix4 i2c_core virtio_net virtio_blk floppy
[ 46.849092] CPU 0
[ 46.849092] Pid: 6, comm: kworker/u:0 ve: 0 Not tainted 3.9.4+ #42 ovz.2.4 Red Hat KVM
[ 46.849092] RIP: 0010:[<ffffffff811a484c>] [<ffffffff811a484c>] kmem_cache_destroy_memcg_children+0x6c/0xc0
[ 46.849092] RSP: 0018:ffff88007c7dfcd8 EFLAGS: 00010206
[ 46.849092] RAX: ffff88007b65f180 RBX: 000000fffffffe00 RCX: 0000000000000004
[ 46.849092] RDX: 0000000000000005 RSI: ffff88007fc17cc8 RDI: ffffffff81c5e5a0
[ 46.849092] RBP: ffff88007c7dfcf8 R08: ffffea0001ee1b20 R09: 0000000000000000
[ 46.849092] R10: ffff88007fbe5fe0 R11: 0000000000000000 R12: 0000000000000005
[ 46.849092] R13: ffff88007b8c2400 R14: 0000000000000000 R15: ffff88007c008005
[ 46.849092] FS: 0000000000000000(0000) GS:ffff88007fc00000(0000) knlGS:0000000000000000
[ 46.849092] CS: 0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[ 46.849092] CR2: 000000fffffffeb8 CR3: 00000000375a3000 CR4: 00000000000006f0
[ 46.849092] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 46.849092] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[ 46.849092] Process kworker/u:0 (pid: 6, ve: 0, threadinfo ffff88007c7de000, task ffff88007c7e8000)
[ 46.849092] Stack:
[ 46.849092] ffff88007bc90870 ffff88007b8c2400 ffff88007bc90000 ffff88007bc90870
[ 46.849092] ffff88007c7dfd18 ffffffff81166a14 0000000080000003 ffff88007bc90000
[ 46.849092] ffff88007c7dfd48 ffffffffa007b3c8 ffff88007c7dfd48 ffff88007bc90000
[ 46.849092] Call Trace:
[ 46.849092] [<ffffffff81166a14>] kmem_cache_destroy+0x14/0xf0
[ 46.849092] [<ffffffffa007b3c8>] nf_conntrack_cleanup_net+0xf8/0x120 [nf_conntrack]
[ 46.849092] [<ffffffffa007d221>] nf_conntrack_pernet_exit+0x41/0x50 [nf_conntrack]
[ 46.849092] [<ffffffff81536779>] ops_exit_list+0x39/0x60
[ 46.849092] [<ffffffff81536cfb>] cleanup_net+0xfb/0x200
[ 46.849092] [<ffffffff8107f43b>] process_one_work+0x17b/0x3d0
[ 46.849092] [<ffffffff81082639>] worker_thread+0x119/0x380
[ 46.849092] [<ffffffff81082520>] ? manage_workers+0x350/0x350
[ 46.849092] [<ffffffff8108796e>] kthread+0xce/0xe0
[ 46.849092] [<ffffffff810878a0>] ? kthread_freezable_should_stop+0x70/0x70
[ 46.849092] [<ffffffff8163faec>] ret_from_fork+0x7c/0xb0
[ 46.849092] [<ffffffff810878a0>] ? kthread_freezable_should_stop+0x70/0x70
[ 46.849092] Code: fb 48 00 8b 1d 6e f0 1d 01 85 db 7e 4e 45 31 e4 0f 1f 80 00 00 00 00 49 8b 85 b8 00 00 00 49 63 d4 48 8b 5c d0 08 48 85 db 74 23 <48> 8b 83 b8 00 00 00 c6 40 28 00 48 8b bb b8 00 00 00 48 83 c7
This bug was added by v3.9-rc1-221-g15cf17d
>
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -3195,11 +3195,11 @@ int memcg_register_cache(struct mem_cgroup *memcg, struct kmem_cache *s,
> > if (!s->memcg_params)
> > return -ENOMEM;
> >
> > - INIT_WORK(&s->memcg_params->destroy,
> > - kmem_cache_destroy_work_func);
> > if (memcg) {
> > s->memcg_params->memcg = memcg;
> > s->memcg_params->root_cache = root_cache;
> > + INIT_WORK(&s->memcg_params->destroy,
> > + kmem_cache_destroy_work_func);
> > } else
> > s->memcg_params->is_root_cache = true;
>
> So the bug here is that we'll scribble on some entries in
> memcg_caches[]. Those scribbles may or may not be within the part of
> that array which is actually used. If there's code which expects
> memcg_caches[] entries to be zeroed at initialisation then yes, we have
> a problem.
INIT_WORK() sets s->memcg_params->memcg_caches[5] to 0xfffffffe00.
Look at kmem_cache_destroy_memcg_children()
for (i = 0; i < memcg_limited_groups_array_size; i++) {
c = s->memcg_params->memcg_caches[i];
if (!c)
continue;
...
c->memcg_params->dead = false;
This code tries dereference 0xfffffffe00->dead and the kernel panics
>
> But I rather doubt whether this bug was causing runtime problems?
>
>
> Presently memcg_register_cache() allocates too much memory for the
> memcg_caches[] array. If that was fixed then this INIT_WORK() might
> scribble into unknown memory, which is of course serious.
Looks like you find another bug:
struct memcg_cache_params {
bool is_root_cache;
union {
struct kmem_cache *memcg_caches[0];
struct {
struct mem_cgroup *memcg;
struct list_head list;
struct kmem_cache *root_cache;
bool dead;
atomic_t nr_pages;
struct work_struct destroy;
};
};
};
The size of this strcture is 80 bytes, then look at memcg_register_cache()
size_t size = sizeof(struct memcg_cache_params);
if (!memcg_kmem_enabled())
return 0;
if (!memcg)
size += memcg_limited_groups_array_size * sizeof(void *);
s->memcg_params = kzalloc(size, GFP_KERNEL);
Actually it allocates too much memory. It allocates memory as if struct
memcg_cache_params would have been written without union.
Actually you already suggested to rework this code
https://lkml.org/lkml/2013/5/28/585
Thanks.
On Tue, 6 Aug 2013 01:01:28 +0400 Andrew Vagin <[email protected]> wrote:
> On Mon, Aug 05, 2013 at 01:05:30PM -0700, Andrew Morton wrote:
> > On Mon, 5 Aug 2013 20:09:40 +0400 Andrey Vagin <[email protected]> wrote:
> >
> > > struct memcg_cache_params has a union. Different parts of this union
> > > are used for root and non-root caches. A part with destroying work is
> > > used only for non-root caches.
> > >
> > > I fixed the same problem in another place v3.9-rc1-16204-gf101a94, but
> > > didn't notice this one.
> > >
> > > Cc: <[email protected]> [3.9.x]
> >
> > hm, why the cc:stable?
>
> Because this patch fixes the kernel panic:
>
> [ 46.848187] BUG: unable to handle kernel paging request at 000000fffffffeb8
> [ 46.849026] IP: [<ffffffff811a484c>] kmem_cache_destroy_memcg_children+0x6c/0xc0
> [ 46.849092] PGD 0
> [ 46.849092] Oops: 0000 [#1] SMP
OK, pretty soon we'll have a changelog!
What does one do to trigger this oops? The bug has been there since
3.9, so the means-of-triggering must be quite special?
On Mon, Aug 05, 2013 at 02:16:09PM -0700, Andrew Morton wrote:
> On Tue, 6 Aug 2013 01:01:28 +0400 Andrew Vagin <[email protected]> wrote:
>
> > On Mon, Aug 05, 2013 at 01:05:30PM -0700, Andrew Morton wrote:
> > > On Mon, 5 Aug 2013 20:09:40 +0400 Andrey Vagin <[email protected]> wrote:
> > >
> > > > struct memcg_cache_params has a union. Different parts of this union
> > > > are used for root and non-root caches. A part with destroying work is
> > > > used only for non-root caches.
> > > >
> > > > I fixed the same problem in another place v3.9-rc1-16204-gf101a94, but
> > > > didn't notice this one.
> > > >
> > > > Cc: <[email protected]> [3.9.x]
> > >
> > > hm, why the cc:stable?
> >
> > Because this patch fixes the kernel panic:
> >
> > [ 46.848187] BUG: unable to handle kernel paging request at 000000fffffffeb8
> > [ 46.849026] IP: [<ffffffff811a484c>] kmem_cache_destroy_memcg_children+0x6c/0xc0
> > [ 46.849092] PGD 0
> > [ 46.849092] Oops: 0000 [#1] SMP
>
> OK, pretty soon we'll have a changelog!
Sorry, probably I had to write all these in the initial commit message. I
just thought that this patch is an additional part of v3.9-rc1-16204-gf101a94.
>
> What does one do to trigger this oops? The bug has been there since
> 3.9, so the means-of-triggering must be quite special?
I don't think that so many people use cgroups with limits of the kernel memory.
I use the vzctl utility to operate with containers. vzctl limits the
kernel memory of containers by default. A container should be started
and stoped a few times (five or four) to reproduce the bug.
And one more thing is that nf_conntrack should be loaded. It creates
a new kmem_cache for each network namespace.
Thanks