Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753866AbbLQXCT (ORCPT ); Thu, 17 Dec 2015 18:02:19 -0500 Received: from mail.linuxfoundation.org ([140.211.169.12]:44023 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751972AbbLQXCS (ORCPT ); Thu, 17 Dec 2015 18:02:18 -0500 Date: Thu, 17 Dec 2015 15:02:17 -0800 From: Andrew Morton To: Vladimir Davydov Cc: Johannes Weiner , , "Michal Hocko" , , Subject: Re: [PATCH v2] mm: memcontrol: fix possible memcg leak due to interrupted reclaim Message-Id: <20151217150217.a02c264ce9b5335b02bae888@linux-foundation.org> In-Reply-To: <1450182697-11049-1-git-send-email-vdavydov@virtuozzo.com> References: <1450182697-11049-1-git-send-email-vdavydov@virtuozzo.com> X-Mailer: Sylpheed 3.4.1 (GTK+ 2.24.23; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3005 Lines: 71 On Tue, 15 Dec 2015 15:31:37 +0300 Vladimir Davydov wrote: > Memory cgroup reclaim can be interrupted with mem_cgroup_iter_break() > once enough pages have been reclaimed, in which case, in contrast to a > full round-trip over a cgroup sub-tree, the current position stored in > mem_cgroup_reclaim_iter of the target cgroup does not get invalidated > and so is left holding the reference to the last scanned cgroup. If the > target cgroup does not get scanned again (we might have just reclaimed > the last page or all processes might exit and free their memory > voluntary), we will leak it, because there is nobody to put the > reference held by the iterator. > > The problem is easy to reproduce by running the following command > sequence in a loop: > > mkdir /sys/fs/cgroup/memory/test > echo 100M > /sys/fs/cgroup/memory/test/memory.limit_in_bytes > echo $$ > /sys/fs/cgroup/memory/test/cgroup.procs > memhog 150M > echo $$ > /sys/fs/cgroup/memory/cgroup.procs > rmdir test > > The cgroups generated by it will never get freed. > > This patch fixes this issue by making mem_cgroup_iter avoid taking > reference to the current position. In order not to hit use-after-free > bug while running reclaim in parallel with cgroup deletion, we make use > of ->css_released cgroup callback to clear references to the dying > cgroup in all reclaim iterators that might refer to it. This callback is > called right before scheduling rcu work which will free css, so if we > access iter->position from rcu read section, we might be sure it won't > go away under us. > > ... > > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -859,14 +859,20 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *root, > if (prev && reclaim->generation != iter->generation) > goto out_unlock; > > - do { > + while (1) { > pos = READ_ONCE(iter->position); > + if (!pos || css_tryget(&pos->css)) > + break; > /* > - * A racing update may change the position and > - * put the last reference, hence css_tryget(), > - * or retry to see the updated position. > + * css reference reached zero, so iter->position will > + * be cleared by ->css_released. However, we should not > + * rely on this happening soon, because ->css_released > + * is called from a work queue, and by busy-waiting we > + * might block it. So we clear iter->position right > + * away. > */ > - } while (pos && !css_tryget(&pos->css)); > + cmpxchg(&iter->position, pos, NULL); > + } It's peculiar to use cmpxchg() without actually checking that it did anything. Should we use xchg() here? And why aren't we using plain old "=", come to that? Perhaps it just needs a comment to defog things. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/