linux-kernel - Re: [PATCH v2 1/3] mm: memcontrol: fix swap counter leak on swapout from offline cgroup

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: <20160803100647.GH13263@esperanza>
Date:	Wed, 3 Aug 2016 13:06:47 +0300
From:	Vladimir Davydov <vdavydov@...tuozzo.com>
To:	Michal Hocko <mhocko@...nel.org>
CC:	Johannes Weiner <hannes@...xchg.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	<stable@...r.kernel.org>, <linux-mm@...ck.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/3] mm: memcontrol: fix swap counter leak on swapout
 from offline cgroup

On Tue, Aug 02, 2016 at 10:31:16PM +0200, Michal Hocko wrote:
> On Tue 02-08-16 13:33:37, Johannes Weiner wrote:
> > On Tue, Aug 02, 2016 at 06:00:26PM +0200, Michal Hocko wrote:
> > > On Tue 02-08-16 18:00:48, Vladimir Davydov wrote:
> > > > @@ -5767,15 +5785,20 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
> > > >  	if (!memcg)
> > > >  		return;
> > > >  
> > > > -	mem_cgroup_id_get(memcg);
> > > > -	oldid = swap_cgroup_record(entry, mem_cgroup_id(memcg));
> > > > +	swap_memcg = mem_cgroup_id_get_active(memcg);
> > > > +	oldid = swap_cgroup_record(entry, mem_cgroup_id(swap_memcg));
> > > >  	VM_BUG_ON_PAGE(oldid, page);
> > > > -	mem_cgroup_swap_statistics(memcg, true);
> > > > +	mem_cgroup_swap_statistics(swap_memcg, true);
> > > >  
> > > >  	page->mem_cgroup = NULL;
> > > >  
> > > >  	if (!mem_cgroup_is_root(memcg))
> > > >  		page_counter_uncharge(&memcg->memory, 1);
> > > > +	if (memcg != swap_memcg) {
> > > > +		if (!mem_cgroup_is_root(swap_memcg))
> > > > +			page_counter_charge(&swap_memcg->memsw, 1);
> > > > +		page_counter_uncharge(&memcg->memsw, 1);
> > > > +	}
> > > >  
> > > >  	/*
> > > >  	 * Interrupts should be disabled here because the caller holds the
> > > 
> > > The resulting code is a weird mixture of memcg and swap_memcg usage
> > > which is really confusing and error prone. Do we really have to do
> > > uncharge on an already offline memcg?
> > 
> > The charge is recursive and includes swap_memcg, i.e. live groups, so
> > the uncharge is necessary.
> 
> Hmm, the charge is recursive, alraight, but then I see only see only
> small sympathy for
>                if (!mem_cgroup_is_root(swap_memcg))
>                        page_counter_charge(&swap_memcg->memsw, 1);
>                page_counter_uncharge(&memcg->memsw, 1);
> 
> we first charge up the hierarchy just to uncharge the same balance from
> the lower. So the end result should be same, right? The only reason
> would be that we uncharge the lower layer as well. I do not remember
> details, but I do not remember we would be checking counters being 0 on
> exit.

We don't, but I think it would be nice to check the counters on css
free, as it might be helpful for debugging.

I thought about introducing page_counter_uncharge_until() to make this
code look more straightforward, but finally decided to leave it as it is
now, because this code is doomed to die anyway once the unified
hierarchy has settled in.