Re: cgroup pointed by sock is leaked on mode switch

From: Zefan Li
Date: Wed May 06 2020 - 03:51:46 EST


On 2020/5/6 10:16, Zefan Li wrote:
On 2020/5/6 9:50, Yang Yingliang wrotee:
+cc lizefan@xxxxxxxxxx

On 2020/5/6 0:06, Tejun Heo wrote:
Hello, Yang.

On Sat, May 02, 2020 at 06:27:21PM +0800, Yang Yingliang wrote:
I find the number nr_dying_descendants is increasing:
linux-dVpNUK:~ # find /sys/fs/cgroup/ -name cgroup.stat -exec grep
'^nr_dying_descendants [^0]'Â {} +
/sys/fs/cgroup/unified/cgroup.stat:nr_dying_descendants 80
/sys/fs/cgroup/unified/system.slice/cgroup.stat:nr_dying_descendants 1
/sys/fs/cgroup/unified/system.slice/system-hostos.slice/cgroup.stat:nr_dying_descendants
1
/sys/fs/cgroup/unified/lxc/cgroup.stat:nr_dying_descendants 79
/sys/fs/cgroup/unified/lxc/5f1fdb8c54fa40c3e599613dab6e4815058b76ebada8a27bc1fe80c0d4801764/cgroup.stat:nr_dying_descendants
78
/sys/fs/cgroup/unified/lxc/5f1fdb8c54fa40c3e599613dab6e4815058b76ebada8a27bc1fe80c0d4801764/system.slice/cgroup.stat:nr_dying_descendants
78
Those numbers are nowhere close to causing oom issues. There are some
aspects of page and other cache draining which is being improved but unless
you're seeing numbers multiple orders of magnitude higher, this isn't the
source of your problem.

The situation is as same as the commit bd1060a1d671 ("sock, cgroup: add
sock->sk_cgroup") describes.
"On mode switch, cgroup references which are already being pointed to by
socks may be leaked."
I'm doubtful that you're hitting that issue. Mode switching means memcg
being switched between cgroup1 and cgroup2 hierarchies, which is unlikely to
be what's happening when you're launching docker containers.

The first step would be identifying where memory is going and finding out
whether memcg is actually being switched between cgroup1 and 2 - look at the
hierarchy number in /proc/cgroups, if that's switching between 0 and
someting not zero, it is switching.


I think there's a bug here which can lead to unlimited memory leak.
This should reproduce the bug:

ÂÂ # mount -t cgroup -o netprio xxx /cgroup/netprio
ÂÂ # mkdir /cgroup/netprio/xxx
ÂÂ # echo PID > /cgroup/netprio/xxx/tasks
ÂÂ /* this PID process starts to do some network thing and then exits */
ÂÂ # rmdir /cgroup/netprio/xxx
ÂÂ /* now this cgroup will never be freed */


Correction (still not tested):

# mount -t cgroup2 none /cgroup/v2
# mkdir /cgroup/v2/xxx
# echo PID > /cgroup/v2/xxx/cgroup.procs
/* this PID process starts to do some network thing */

# mount -t cgroup -o netprio xxx /cgroup/netprio
# mkdir /cgroup/netprio/xxx
# echo PID > /cgroup/netprio/xxx/tasks
...
/* the PID process exits */

rmdir /cgroup/netprio/xxx
rmdir /cgroup/v2/xxx
/* now looks like this v2 cgroup will never be freed */

Look at the code:

static inline void sock_update_netprioidx(struct sock_cgroup_data *skcd)
{
ÂÂÂÂ...
ÂÂÂÂsock_cgroup_set_prioidx(skcd, task_netprioidx(current));
}

static inline void sock_cgroup_set_prioidx(struct sock_cgroup_data *skcd,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ u16 prioidx)
{
ÂÂÂÂ...
ÂÂÂÂif (sock_cgroup_prioidx(&skcd_buf) == prioidx)
ÂÂÂÂÂÂÂ return ;
ÂÂÂÂ...
ÂÂÂÂskcd_buf.prioidx = prioidx;
ÂÂÂÂWRITE_ONCE(skcd->val, skcd_buf.val);
}

task_netprioidx() will be the cgrp id of xxx which is not 1, but
sock_cgroup_prioidx(&skcd_buf) is 1 because it thought it's in v2 mode.
Now we have a memory leak.

I think the eastest fix is to do the mode switch here:

diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c
index b905747..2397866 100644
--- a/net/core/netprio_cgroup.c
+++ b/net/core/netprio_cgroup.c
@@ -240,6 +240,8 @@ static void net_prio_attach(struct cgroup_taskset *tset)
ÂÂÂÂÂÂÂ struct task_struct *p;
ÂÂÂÂÂÂÂ struct cgroup_subsys_state *css;

+ÂÂÂÂÂÂ cgroup_sk_alloc_disable();
+
ÂÂÂÂÂÂÂ cgroup_taskset_for_each(p, css, tset) {
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ void *v = (void *)(unsigned long)css->cgroup->id;