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 stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html