Bug in netprio_cgroup and netcls_cgroup ?

BUG Li Zefan 3 participants 12 messages Jan 21, 2013 – Jan 25, 2013

Subsystems (3)

Networking Network drivers cgroup

lore

Share & tools

lore

Li Zefan

author

2013-01-21 06:08 UTC #0 lore raw

I'm not a network developer, so correct me if I'm wrong.

Since commit 406a3c638ce8b17d9704052c07955490f732c2b8
("net: netprio_cgroup: rework update socket logic"), sock->sk->sk_cgrp_prioidx is set when the socket is created, and won't be updated unless the task is moved to another cgroup.

Now the problem is, a socket can be _shared_ by multiple processes (fork, SCM_RIGHT). If we place those processes in different cgroups, and each cgroup has different configs, but all of the processes will send data via this socket with the same network priority.

Similar with cls cgroup.

Daniel Wagner

2013-01-21 08:50 UTC #1 lore raw

Hi Li,

Quoted 11 lines · Li Zefan

I'm not a network developer, so correct me if I'm wrong.
Since commit 7955490f732c2b8
("net: netprio_cgroup: rework update socket logic"), sock->sk->sk_cgrp_prioidx is set when the socket is created, and won't be updated unless the task is moved to another cgroup.
Now the problem is, a socket can be _shared_ by multiple processes (fork, SCM_RIGHT). If we place those processes in different cgroups, and each cgroup has different configs, but all of the processes will send data via this socket with the same network priority.

Wouldn't that be addressed by 48a87cc26c13b68f6cce4e9d769fcb17a6b3e4b8

net: netprio: fd passed in SCM_RIGHTS datagram not set correctly

A socket fd passed in a SCM_RIGHTS datagram was not getting updated with the new tasks cgrp prioidx. This leaves IO on the socket tagged with the old tasks priority.

To fix this add a check in the scm recvmsg path to update the sock cgrp prioidx with the new tasks value.

As I read this this should work for net_prio.

Quoted 1 line

Similar with cls cgroup.

but not for cls_cgroup because I forgot to apply that fix as well. Just a sec, about to
send a patch for this.

Thanks for looking into this!

cheers,
daniel

Li Zefan

author

2013-01-21 09:01 UTC #2 lore raw

Quoted 19 lines · Daniel Wagner

Hi Li,
Quoted 11 lines · Li Zefan
I'm not a network developer, so correct me if I'm wrong.
Since commit 7955490f732c2b8
("net: netprio_cgroup: rework update socket logic"), sock->sk->sk_cgrp_prioidx is set when the socket is created, and won't be updated unless the task is moved to another cgroup.
Now the problem is, a socket can be _shared_ by multiple processes (fork, SCM_RIGHT). If we place those processes in different cgroups, and each cgroup has different configs, but all of the processes will send data via this socket with the same network priority.
Wouldn't that be addressed by 48a87cc26c13b68f6cce4e9d769fcb17a6b3e4b8
net: netprio: fd passed in SCM_RIGHTS datagram not set correctly

Quoted 9 lines

A socket fd passed in a SCM_RIGHTS datagram was not getting updated with the new tasks cgrp prioidx. This leaves IO on the socket tagged with the old tasks priority.
To fix this add a check in the scm recvmsg path to update the sock cgrp prioidx with the new tasks value.
As I read this this should work for net_prio.

But after process A passed the socket fd to B, both A and B can use the same socket to send data, right? Then if A and B were placed in different cgroups with differnt configs, A's config won't take effect anymore.

Am I missing something?

John Fastabend

2013-01-21 09:21 UTC #3 lore raw

Quoted 27 lines · Li Zefan

Quoted 26 lines · Daniel Wagner
Hi Li,
Quoted 11 lines · Li Zefan
I'm not a network developer, so correct me if I'm wrong.
Since commit 7955490f732c2b8
("net: netprio_cgroup: rework update socket logic"), sock->sk->sk_cgrp_prioidx is set when the socket is created, and won't be updated unless the task is moved to another cgroup.
Now the problem is, a socket can be _shared_ by multiple processes (fork, SCM_RIGHT). If we place those processes in different cgroups, and each cgroup has different configs, but all of the processes will send data via this socket with the same network priority.
Wouldn't that be addressed by 48a87cc26c13b68f6cce4e9d769fcb17a6b3e4b8
net: netprio: fd passed in SCM_RIGHTS datagram not set correctly
A socket fd passed in a SCM_RIGHTS datagram was not getting updated with the new tasks cgrp prioidx. This leaves IO on the socket tagged with the old tasks priority.
To fix this add a check in the scm recvmsg path to update the sock cgrp prioidx with the new tasks value.

Quoted 10 lines

Quoted 2 lines
As I read this this should work for net_prio.
But after process A passed the socket fd to B, both A and B can use the same socket to send data, right? Then if A and B were placed in different cgroups with differnt configs, A's config won't take effect anymore.
Am I missing something?

Hi Zefan,

Neil and I discusses this here, http://patchwork.ozlabs.org/patch/172343/
look towards the bottom of the thread. Quoted here.

Quoted 15 lines

Quoted 8 lines
I like the idea, but IIRC last time we tried this I think it caused problems with processes that shared sockets. That is to say, if you have a parent and child process that dup an socket descriptior, and put them in separate cgroups, you get unpredictable results, as the socket gets assigned a priority based on the last processed that moved cgroups.
Neil
Shared sockets creates strange behavior as it exists today. If a dup of the socket fd is created the private data is still shared right. So in this case the sk_cgrp_prioidx value is going to get updated by both threads and then it is a race to see what it happens to be set to in the xmit path.

Quoted 4 lines

With this patch at least the behavior is deterministic. Without it I can create the above scenario but have no way to determine what the skb priority will actually be set to.

Its unfortunate but I'm not sure how to fix it off hand with the shared value
in the socket.

.John

Signature · 1 line

John Fastabend         Intel Corporation

Li Zefan

author

2013-01-21 09:52 UTC #4 lore raw

Quoted 35 lines · John Fastabend

Quoted 34 lines · Li Zefan
Quoted 19 lines · Daniel Wagner
Hi Li,
Quoted 11 lines · Li Zefan
I'm not a network developer, so correct me if I'm wrong.
Since commit 7955490f732c2b8
("net: netprio_cgroup: rework update socket logic"), sock->sk->sk_cgrp_prioidx is set when the socket is created, and won't be updated unless the task is moved to another cgroup.
Now the problem is, a socket can be _shared_ by multiple processes (fork, SCM_RIGHT). If we place those processes in different cgroups, and each cgroup has different configs, but all of the processes will send data via this socket with the same network priority.
Wouldn't that be addressed by 48a87cc26c13b68f6cce4e9d769fcb17a6b3e4b8
net: netprio: fd passed in SCM_RIGHTS datagram not set correctly
Quoted 9 lines
A socket fd passed in a SCM_RIGHTS datagram was not getting updated with the new tasks cgrp prioidx. This leaves IO on the socket tagged with the old tasks priority.
To fix this add a check in the scm recvmsg path to update the sock cgrp prioidx with the new tasks value.
As I read this this should work for net_prio.
But after process A passed the socket fd to B, both A and B can use the same socket to send data, right? Then if A and B were placed in different cgroups with differnt configs, A's config won't take effect anymore.

Quoted 9 lines

Quoted 3 lines
Am I missing something?
Hi Zefan,
Neil and I discusses this here, http://patchwork.ozlabs.org/patch/172343/
look towards the bottom of the thread. Quoted here.

So this is a known issue. Why not document this behavior in Documentation/cgroups/netprio.txt?

Daniel Wagner

2013-01-21 09:27 UTC #5 lore raw

In reply to Li Zefan

Quoted 27 lines · Li Zefan

Quoted 26 lines · Daniel Wagner
Hi Li,
Quoted 11 lines · Li Zefan
I'm not a network developer, so correct me if I'm wrong.
Since commit 7955490f732c2b8
("net: netprio_cgroup: rework update socket logic"), sock->sk->sk_cgrp_prioidx is set when the socket is created, and won't be updated unless the task is moved to another cgroup.
Now the problem is, a socket can be _shared_ by multiple processes (fork, SCM_RIGHT). If we place those processes in different cgroups, and each cgroup has different configs, but all of the processes will send data via this socket with the same network priority.
Wouldn't that be addressed by 48a87cc26c13b68f6cce4e9d769fcb17a6b3e4b8
net: netprio: fd passed in SCM_RIGHTS datagram not set correctly
A socket fd passed in a SCM_RIGHTS datagram was not getting updated with the new tasks cgrp prioidx. This leaves IO on the socket tagged with the old tasks priority.
To fix this add a check in the scm recvmsg path to update the sock cgrp prioidx with the new tasks value.

Quoted 8 lines

Quoted 2 lines
As I read this this should work for net_prio.
But after process A passed the socket fd to B, both A and B can use the same socket to send data, right? Then if A and B were placed in different cgroups with differnt configs, A's config won't take effect anymore.
Am I missing something?

I don't know. I guess at one point the socket resources are shared and then one configuration is taking preference. As you can see I am far away of being
an expert in this field. Hopefully someone who understands this bits can chip in.

BTW, isn't this a similar to what should happen with the block io cgroup? What is the behavior with a fd writing to a file in the scenario you describe above?

Li Zefan

author

2013-01-21 09:57 UTC #6 lore raw

Quoted 43 lines · Daniel Wagner

Quoted 27 lines · Li Zefan
Quoted 26 lines · Daniel Wagner
Hi Li,
Quoted 11 lines · Li Zefan
I'm not a network developer, so correct me if I'm wrong.
Since commit 7955490f732c2b8
("net: netprio_cgroup: rework update socket logic"), sock->sk->sk_cgrp_prioidx is set when the socket is created, and won't be updated unless the task is moved to another cgroup.
Now the problem is, a socket can be _shared_ by multiple processes (fork, SCM_RIGHT). If we place those processes in different cgroups, and each cgroup has different configs, but all of the processes will send data via this socket with the same network priority.
Wouldn't that be addressed by 48a87cc26c13b68f6cce4e9d769fcb17a6b3e4b8
net: netprio: fd passed in SCM_RIGHTS datagram not set correctly
A socket fd passed in a SCM_RIGHTS datagram was not getting updated with the new tasks cgrp prioidx. This leaves IO on the socket tagged with the old tasks priority.
To fix this add a check in the scm recvmsg path to update the sock cgrp prioidx with the new tasks value.
Quoted 8 lines
Quoted 2 lines
As I read this this should work for net_prio.
But after process A passed the socket fd to B, both A and B can use the same socket to send data, right? Then if A and B were placed in different cgroups with differnt configs, A's config won't take effect anymore.
Am I missing something?
I don't know. I guess at one point the socket resources are shared and then one configuration is taking preference. As you can see I am far away of being
an expert in this field. Hopefully someone who understands this bits can chip in.

Quoted 4 lines

BTW, isn't this a similar to what should happen with the block io cgroup? What is the behavior with a fd writing to a file in the scenario you describe above?

It forbids task moving in this case:

/*
* We cannot support shared io contexts, as we have no mean to support
* two tasks with the same ioc in two different groups without major rework
* of the main cic data structures. For now we allow a task to change
* its cgroup only if it's the only owner of its ioc.
*/
static int blkcg_can_attach(struct cgroup *cgrp, struct cgroup_taskset *tset)

John Fastabend

2013-01-21 17:18 UTC #7 lore raw

Quoted 51 lines · Li Zefan

Quoted 43 lines · Daniel Wagner
Quoted 27 lines · Li Zefan
Quoted 26 lines · Daniel Wagner
Hi Li,
Quoted 11 lines · Li Zefan
I'm not a network developer, so correct me if I'm wrong.
Since commit 7955490f732c2b8
("net: netprio_cgroup: rework update socket logic"), sock->sk->sk_cgrp_prioidx is set when the socket is created, and won't be updated unless the task is moved to another cgroup.
Now the problem is, a socket can be _shared_ by multiple processes (fork, SCM_RIGHT). If we place those processes in different cgroups, and each cgroup has different configs, but all of the processes will send data via this socket with the same network priority.
Wouldn't that be addressed by 48a87cc26c13b68f6cce4e9d769fcb17a6b3e4b8
net: netprio: fd passed in SCM_RIGHTS datagram not set correctly
A socket fd passed in a SCM_RIGHTS datagram was not getting updated with the new tasks cgrp prioidx. This leaves IO on the socket tagged with the old tasks priority.
To fix this add a check in the scm recvmsg path to update the sock cgrp prioidx with the new tasks value.
Quoted 8 lines
Quoted 2 lines
As I read this this should work for net_prio.
But after process A passed the socket fd to B, both A and B can use the same socket to send data, right? Then if A and B were placed in different cgroups with differnt configs, A's config won't take effect anymore.
Am I missing something?
I don't know. I guess at one point the socket resources are shared and then one configuration is taking preference. As you can see I am far away of being
an expert in this field. Hopefully someone who understands this bits can chip in.
Quoted 4 lines
BTW, isn't this a similar to what should happen with the block io cgroup? What is the behavior with a fd writing to a file in the scenario you describe above?
It forbids task moving in this case:

Quoted 8 lines

/*
* We cannot support shared io contexts, as we have no mean to support
* two tasks with the same ioc in two different groups without major rework
* of the main cic data structures. For now we allow a task to change
* its cgroup only if it's the only owner of its ioc.
*/
static int blkcg_can_attach(struct cgroup *cgrp, struct cgroup_taskset *tset)

OK, I guess we should do something similar in the netprio, netcls cgroups and
yes document it as you noted in your last comment.

Thanks,
John

Signature · 1 line

John Fastabend         Intel Corporation

Daniel Wagner

2013-01-22 10:09 UTC #8 lore raw

Quoted 61 lines · John Fastabend

Quoted 51 lines · Li Zefan
Quoted 43 lines · Daniel Wagner
Quoted 27 lines · Li Zefan
Quoted 26 lines · Daniel Wagner
Hi Li,
Quoted 11 lines · Li Zefan
I'm not a network developer, so correct me if I'm wrong.
Since commit 7955490f732c2b8
("net: netprio_cgroup: rework update socket logic"), sock->sk->sk_cgrp_prioidx is set when the socket is created, and won't be updated unless the task is moved to another cgroup.
Now the problem is, a socket can be _shared_ by multiple processes (fork, SCM_RIGHT). If we place those processes in different cgroups, and each cgroup has different configs, but all of the processes will send data via this socket with the same network priority.
Wouldn't that be addressed by 48a87cc26c13b68f6cce4e9d769fcb17a6b3e4b8
net: netprio: fd passed in SCM_RIGHTS datagram not set correctly
A socket fd passed in a SCM_RIGHTS datagram was not getting updated with the new tasks cgrp prioidx. This leaves IO on the socket tagged with the old tasks priority.
To fix this add a check in the scm recvmsg path to update the sock cgrp prioidx with the new tasks value.
Quoted 8 lines
Quoted 2 lines
As I read this this should work for net_prio.
But after process A passed the socket fd to B, both A and B can use the same socket to send data, right? Then if A and B were placed in different cgroups with differnt configs, A's config won't take effect anymore.
Am I missing something?
I don't know. I guess at one point the socket resources are shared and then one configuration is taking preference. As you can see I am far away of being
an expert in this field. Hopefully someone who understands this bits can chip in.
Quoted 4 lines
BTW, isn't this a similar to what should happen with the block io cgroup? What is the behavior with a fd writing to a file in the scenario you describe above?
It forbids task moving in this case:
Quoted 8 lines
/*
* We cannot support shared io contexts, as we have no mean to support
* two tasks with the same ioc in two different groups without major rework
* of the main cic data structures. For now we allow a task to change
* its cgroup only if it's the only owner of its ioc.
*/
static int blkcg_can_attach(struct cgroup *cgrp, struct cgroup_taskset *tset)

Quoted 3 lines

OK, I guess we should do something similar in the netprio, netcls cgroups and
yes document it as you noted in your last comment.

Here is my attempt to add such a check. I really don't know if this is the correct way to do so. To test this I have written a test program, which seems to test the right thing. Please have a look and let me know if it is correct: http://www.monom.org/misc/scm_rights.c

And here a dirty first version of the patch:

From 49a78d907eaf31c16673025e7e3b4844e419e416 Mon Sep 17 00:00:00 2001
From: Daniel Wagner <[email protected]>
Date: Tue, 22 Jan 2013 11:08:22 +0100
Subject: [PATCH] net: net_prio: Block attach if a socket is shared

---
 net/core/netprio_cgroup.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

net/core/netprio_cgroup.c +30 -0

index 847c02b..de4e6c5 100644
@@ -274,9 +274,39 @@ static struct cftype ss_files[] = {
 	{ }	/* terminate */
 };
 
+static int check_cnt(const void *v, struct file *file, unsigned n)
+{
+	unsigned *flag = (unsigned *)v;
+	int err;
+
+	struct socket *sock = sock_from_file(file, &err);
+	if (sock && file_count(file) > 1)
+		*flag = 1;
+
+	return 0;
+}
+
+static int cgrp_can_attach(struct cgroup *cgrp, struct cgroup_taskset *tset)
+{
+	struct task_struct *p;
+	unsigned flag = 0;
+
+	cgroup_taskset_for_each(p, cgrp, tset) {
+		task_lock(p);
+		iterate_fd(p->files, 0, check_cnt, &flag);
+		task_unlock(p);
+	}
+
+	if (flag != 0)
+		return -EINVAL;
+
+	return 0;
+}
+
 struct cgroup_subsys net_prio_subsys = {
 	.name		= "net_prio",
 	.create		= cgrp_create,
+	.can_attach	= cgrp_can_attach,
 	.destroy	= cgrp_destroy,
 	.attach		= net_prio_attach,
 	.subsys_id	= net_prio_subsys_id,

Signature · 1 line

1.8.0.rc0

John Fastabend

2013-01-23 00:02 UTC #9 lore raw

[...]

Quoted 29 lines

Quoted 4 lines
OK, I guess we should do something similar in the netprio, netcls cgroups and
yes document it as you noted in your last comment.
Here is my attempt to add such a check. I really don't know if this is the correct way to do so. To test this I have written a test program, which seems to test the right thing. Please have a look and let me know if it is correct: http://www.monom.org/misc/scm_rights.c
And here a dirty first version of the patch:
From 49a78d907eaf31c16673025e7e3b4844e419e416 Mon Sep 17 00:00:00 2001
From: Daniel Wagner <[email protected]>
Date: Tue, 22 Jan 2013 11:08:22 +0100
Subject: [PATCH] net: net_prio: Block attach if a socket is shared
---
  net/core/netprio_cgroup.c | 30 ++++++++++++++++++++++++++++++
  1 file changed, 30 insertions(+)
net/core/netprio_cgroup.c +0 -0
index 847c02b..de4e6c5 100644
@@ -274,9 +274,39 @@ static struct cftype ss_files[] = {
  	{ }	/* terminate */
  };

net/core/netprio_cgroup.c static struct cftype ss_files[] = { · line 285

Quoted 10 lines

net/core/netprio_cgroup.c +9 -0

 
+static int check_cnt(const void *v, struct file *file, unsigned n)
+{
+	unsigned *flag = (unsigned *)v;
+	int err;
+
+	struct socket *sock = sock_from_file(file, &err);
+	if (sock && file_count(file) > 1)
+		*flag = 1;
+

I think this check will catch a lot of cases that are not necessarily sharing a socket across tasks though. For example iscsid passes a file descriptor to the kernel which does a sockfd_lookup() incrementing f_count. Similarly look at dup/clone/etc.

In many of these cases I believe it should be OK to move the task around when the sockets are not shared between multiple tasks.

.John

Signature · 1 line

John Fastabend         Intel Corporation

Daniel Wagner

2013-01-23 09:24 UTC #10 lore raw

Quoted 33 lines · John Fastabend

[...]
Quoted 31 lines
Quoted 4 lines
OK, I guess we should do something similar in the netprio, netcls cgroups and
yes document it as you noted in your last comment.
Here is my attempt to add such a check. I really don't know if this is the
correct way to do so. To test this I have written a test program, which seems to test the right thing. Please have a look and let me know if it is correct: http://www.monom.org/misc/scm_rights.c
And here a dirty first version of the patch:
From 49a78d907eaf31c16673025e7e3b4844e419e416 Mon Sep 17 00:00:00 2001
From: Daniel Wagner <[email protected]>
Date: Tue, 22 Jan 2013 11:08:22 +0100
Subject: [PATCH] net: net_prio: Block attach if a socket is shared
---
  net/core/netprio_cgroup.c | 30 ++++++++++++++++++++++++++++++
  1 file changed, 30 insertions(+)
net/core/netprio_cgroup.c +0 -0
index 847c02b..de4e6c5 100644
@@ -274,9 +274,39 @@ static struct cftype ss_files[] = {
      { }    /* terminate */
  };
 

Quoted 14 lines

net/core/netprio_cgroup.c static struct cftype ss_files[] = { · line 285
Quoted 9 lines
net/core/netprio_cgroup.c +9 -0
+static int check_cnt(const void *v, struct file *file, unsigned n)
+{
+    unsigned *flag = (unsigned *)v;
+    int err;
+
+    struct socket *sock = sock_from_file(file, &err);
+    if (sock && file_count(file) > 1)
+        *flag = 1;
+
I think this check will catch a lot of cases that are not necessarily sharing a socket across tasks though. For example iscsid passes a file descriptor to the kernel which does a sockfd_lookup() incrementing f_count. Similarly look at dup/clone/etc.

Yep, I expected that this patch was too simple.

Quoted 2 lines

In many of these cases I believe it should be OK to move the task around when the sockets are not shared between multiple tasks.

Do you know of a different way to identify the shared sockets?

thanks,
daniel

Li Zefan

author

2013-01-25 08:39 UTC #11 lore raw

In reply to Daniel Wagner

Quoted 20 lines

Quoted 16 lines
Quoted 7 lines
Quoted 4 lines
BTW, isn't this a similar to what should happen with the block io cgroup? What is the behavior with a fd writing to a file in the scenario you describe above?
It forbids task moving in this case:
Quoted 8 lines
/*
* We cannot support shared io contexts, as we have no mean to support
* two tasks with the same ioc in two different groups without major rework
* of the main cic data structures. For now we allow a task to change
* its cgroup only if it's the only owner of its ioc.
*/
static int blkcg_can_attach(struct cgroup *cgrp, struct cgroup_taskset *tset)
Quoted 3 lines
OK, I guess we should do something similar in the netprio, netcls cgroups and
yes document it as you noted in your last comment.

Quoted 7 lines

Here is my attempt to add such a check. I really don't know if this is the correct way to do so. To test this I have written a test program, which seems to test the right thing. Please have a look and let me know if it is correct: http://www.monom.org/misc/scm_rights.c
And here a dirty first version of the patch:

Adding new can_attach() is discouraged. It's considered bad, because if cgroup is mounted with multi subsystems binded, and if one of them can forbid task moving for some reason, this will add burden on users to use cgroup properly.