linux-kernel - Re: [PATCH] [RFC] kmemcg: remove union from memcg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20130812152812.915d5e8ebe5467586a457eb0@linux-foundation.org>
Date:	Mon, 12 Aug 2013 15:28:12 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Andrey Vagin <avagin@...nvz.org>
Cc:	linux-mm@...ck.org, Glauber Costa <glommer@...allels.com>,
	cgroups@...r.kernel.org, linux-kernel@...r.kernel.org,
	Christoph Lameter <cl@...ux-foundation.org>,
	Pekka Enberg <penberg@...nel.org>,
	Matt Mackall <mpm@...enic.com>,
	Johannes Weiner <hannes@...xchg.org>,
	Michal Hocko <mhocko@...e.cz>,
	Balbir Singh <bsingharora@...il.com>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
Subject: Re: [PATCH] [RFC] kmemcg: remove union from memcg_params

On Fri,  9 Aug 2013 00:51:26 +0400 Andrey Vagin <avagin@...nvz.org> wrote:

> 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;
>                 };
>         };
> };
> 
> This union is a bit dangerous. //Andrew Morton
> 
> The first problem was fixed in v3.10-rc5-67-gf101a94.
> The second problem is that the size of memory for root
> caches is calculated incorrectly:
> 
> 	ssize_t size = memcg_caches_array_size(num_groups);
> 
> 	size *= sizeof(void *);
> 	size += sizeof(struct memcg_cache_params);
> 
> The last line should be fixed like this:
> 	size += offsetof(struct memcg_cache_params, memcg_caches)
> 
> Andrew suggested to rework this code without union and
> this patch tries to do that.

hm, did I?

> This patch removes is_root_cache and union. The size of the
> memcg_cache_params structure is not changed.
> 

It's a bit sad to consume more space because we're sucky programmers. 
It would be better to retain the union and to stop writing buggy code
to handle it!

Maybe there are things we can do to reduce the likelihood of people
mishandling the union - don't use anonymous fields, name each member,
access it via helper functions, etc.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/