Re: [PATCH v2] mm: memcontrol: fix possible memcg leak due to interrupted reclaim

[Andrew Morton]

On Tue, 15 Dec 2015 15:31:37 +0300 Vladimir Davydov <vdavydov@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/