[PATCH 0/4] net: Improve socket sharing between multiple cgroups

[PATCH 0/4] net: Improve socket sharing between multiple cgroups

[Neil Horman]

Currently, the network centric cgroups for priority and classification don't 
share resources properly.  Since socket priority and classification are
assigned based on the socket through which the data was sent, an anomaly arises
when multiple processes share a given socket.  As the priority index and class-id
are updated when a skb is sent at the top of the stack, but interrogated and
used at the bottom of the stack, multiple process living in separate cgroups but
sharing a socket can result in skbs being sent at multiple separate priorities
and classifications.  Aside from the user confusion this may cause, additional
problems can arise if the socket is tcp, and the fluctuation in class or
priority causes network re-ordering that results in serious performance
degradation.

The problem is further compounded by processes that use network resources
unknowingly.  For instance,the CIFS file system creates a network socket to a
cifs server in the context of whatever process is writing to that mount point,
and the same socket will subsequently be used by any other processes writing to
that mount.  This socket sharing will almost certainly lead to classification
and priority changes on a single data stream that will degrade file system
throughput.

This patch series is meant to solve the first of these two problems.  It changes
the way in which the priority and net_cls cgroup implementations migrate the
priority and class-id of the sockets each task owns.  specifically this series:

1) adds an cgroup owner pid field to the sock structure
2) assigns the pid of the creating process at the time of sock allocation
3) zeros the owning pid at the time of sk_common_release (to prevent future pid
aliasing
4) adds a cgroup_attach method to each cgroup subsystem which identifies sockets
owned by that task (based on matching the task pid and the pid provided in (2)
to update the priority and class-id appropriately.

This does not solve the anonymous socket use problem (I plan to address that in
a future patch series), but it does allow for a sockets priority and
classification to be controlled by a single pid for the purposes of cgroup
assignment.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: Thomas Graf <tgraf@infradead.org>
CC: "David S. Miller" <davem@davemloft.net>

[PATCH 1/4] net_prio/classid: add cgroup process ownership filter

[Neil Horman]

To prevent multiple processes that share a socket from fighting over the cgroup
instance said socket belongs to, add a sk_cgrp_owner flag so that calls to
sk_update_[classid/prioidx] only do something in the event that its the owning
process updating them.  This way only one pid is responsible for updating the
sockets cgroup information.  When that process releases the socket, set the
owner pid to zero so as to prevent a future task that reuses the same pid from
inheriting the socket erroneously

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: Thomas Graf <tgraf@infradead.org>
CC: "David S. Miller" <davem@davemloft.net>
---
 include/net/sock.h |    4 ++++
 net/core/sock.c    |   15 ++++++++++++---
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index 18ecc99..cdb03c2 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -327,6 +327,7 @@ struct sock {
 	unsigned short		sk_max_ack_backlog;
 	__u32			sk_priority;
 #ifdef CONFIG_CGROUPS
+	pid_t			sk_cgrp_owner;
 	__u32			sk_cgrp_prioidx;
 #endif
 	struct pid		*sk_peer_pid;
@@ -1537,6 +1538,9 @@ static inline void sock_orphan(struct sock *sk)
 	sock_set_flag(sk, SOCK_DEAD);
 	sk_set_socket(sk, NULL);
 	sk->sk_wq  = NULL;
+#ifdef CONFIG_CGROUPS
+	sk->sk_cgrp_owner = 0;
+#endif
 	write_unlock_bh(&sk->sk_callback_lock);
 }
 
diff --git a/net/core/sock.c b/net/core/sock.c
index 5a6a906..b922fb5 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1162,24 +1162,31 @@ static void sk_prot_free(struct proto *prot, struct sock *sk)
 #ifdef CONFIG_CGROUPS
 void sock_update_classid(struct sock *sk)
 {
+	pid_t tpid;
 	u32 classid;
 
 	rcu_read_lock();  /* doing current task, which cannot vanish. */
 	classid = task_cls_classid(current);
+	tpid = task_pid_nr(current);
 	rcu_read_unlock();
-	if (classid && classid != sk->sk_classid)
+
+	if ((tpid == sk->sk_cgrp_owner) &&
+	    (classid && classid != sk->sk_classid))
 		sk->sk_classid = classid;
 }
 EXPORT_SYMBOL(sock_update_classid);
 
 void sock_update_netprioidx(struct sock *sk)
 {
+	pid_t tpid;
 	struct cgroup_netprio_state *state;
 	if (in_interrupt())
 		return;
 	rcu_read_lock();
+	tpid = task_pid_nr(current);
 	state = task_netprio_state(current);
-	sk->sk_cgrp_prioidx = state ? state->prioidx : 0;
+	if (tpid == sk->sk_cgrp_owner)
+		sk->sk_cgrp_prioidx = state ? state->prioidx : 0;
 	rcu_read_unlock();
 }
 EXPORT_SYMBOL_GPL(sock_update_netprioidx);
@@ -1208,7 +1215,9 @@ struct sock *sk_alloc(struct net *net, int family, gfp_t priority,
 		sock_lock_init(sk);
 		sock_net_set(sk, get_net(net));
 		atomic_set(&sk->sk_wmem_alloc, 1);
-
+#ifdef CONFIG_CGROUPS
+		sk->sk_cgrp_owner = task_pid_nr(current);
+#endif
 		sock_update_classid(sk);
 		sock_update_netprioidx(sk);
 	}
-- 
1.7.6.4

[PATCH 2/4] vfs: Export some file manipulation functions

[Neil Horman]

the networking cgroups can use the fd table of a task to make adjustments to the
sockets that they own, helping us set owners for various resources. Export
get_files_struct, put_files_struct and sock_from_file, so we can walk a tasks
fdarray easily.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: Thomas Graf <tgraf@infradead.org>
CC: "David S. Miller" <davem@davemloft.net>
---
 include/linux/fdtable.h |    4 ++--
 include/linux/net.h     |    1 +
 kernel/exit.c           |    2 ++
 net/socket.c            |    3 ++-
 4 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/include/linux/fdtable.h b/include/linux/fdtable.h
index 82163c4..1d06352 100644
--- a/include/linux/fdtable.h
+++ b/include/linux/fdtable.h
@@ -97,8 +97,8 @@ static inline struct file * fcheck_files(struct files_struct *files, unsigned in
 
 struct task_struct;
 
-struct files_struct *get_files_struct(struct task_struct *);
-void put_files_struct(struct files_struct *fs);
+extern struct files_struct *get_files_struct(struct task_struct *);
+extern void put_files_struct(struct files_struct *fs);
 void reset_files_struct(struct files_struct *);
 int unshare_files(struct files_struct **);
 struct files_struct *dup_fd(struct files_struct *, int *);
diff --git a/include/linux/net.h b/include/linux/net.h
index b299230..eea9912 100644
--- a/include/linux/net.h
+++ b/include/linux/net.h
@@ -245,6 +245,7 @@ extern int   	     sock_sendmsg(struct socket *sock, struct msghdr *msg,
 extern int	     sock_recvmsg(struct socket *sock, struct msghdr *msg,
 				  size_t size, int flags);
 extern int 	     sock_map_fd(struct socket *sock, int flags);
+extern struct socket *sock_from_file(struct file *file, int *err);
 extern struct socket *sockfd_lookup(int fd, int *err);
 #define		     sockfd_put(sock) fput(sock->file)
 extern int	     net_ratelimit(void);
diff --git a/kernel/exit.c b/kernel/exit.c
index d0b7d98..2f48b95 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -499,6 +499,7 @@ struct files_struct *get_files_struct(struct task_struct *task)
 
 	return files;
 }
+EXPORT_SYMBOL(get_files_struct);
 
 void put_files_struct(struct files_struct *files)
 {
@@ -520,6 +521,7 @@ void put_files_struct(struct files_struct *files)
 		rcu_read_unlock();
 	}
 }
+EXPORT_SYMBOL(put_files_struct);
 
 void reset_files_struct(struct files_struct *files)
 {
diff --git a/net/socket.c b/net/socket.c
index e62b4f0..eab8db1 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -398,7 +398,7 @@ int sock_map_fd(struct socket *sock, int flags)
 }
 EXPORT_SYMBOL(sock_map_fd);
 
-static struct socket *sock_from_file(struct file *file, int *err)
+struct socket *sock_from_file(struct file *file, int *err)
 {
 	if (file->f_op == &socket_file_ops)
 		return file->private_data;	/* set in sock_map_fd */
@@ -406,6 +406,7 @@ static struct socket *sock_from_file(struct file *file, int *err)
 	*err = -ENOTSOCK;
 	return NULL;
 }
+EXPORT_SYMBOL(sock_from_file);
 
 /**
  *	sockfd_lookup - Go from a file number to its socket slot
-- 
1.7.6.4

Re: [PATCH 2/4] vfs: Export some file manipulation functions

[Christoph Hellwig]

On Wed, Dec 21, 2011 at 09:39:48AM -0500, Neil Horman wrote:
> the networking cgroups can use the fd table of a task to make adjustments to the
> sockets that they own, helping us set owners for various resources. Export
> get_files_struct, put_files_struct and sock_from_file, so we can walk a tasks
> fdarray easily.

No, no one has any business using these lowlevel routines form modules.

Please find a way to do this in core code, or find more highlevel
primitives to export.

And not even Ccing linux-fsdevel on a change like this is one of the few
things I'd consider extremely offensive.

Re: [PATCH 2/4] vfs: Export some file manipulation functions

[Neil Horman]

On Wed, Dec 21, 2011 at 10:30:54AM -0500, Christoph Hellwig wrote:
> On Wed, Dec 21, 2011 at 09:39:48AM -0500, Neil Horman wrote:
> > the networking cgroups can use the fd table of a task to make adjustments to the
> > sockets that they own, helping us set owners for various resources. Export
> > get_files_struct, put_files_struct and sock_from_file, so we can walk a tasks
> > fdarray easily.
> 
> No, no one has any business using these lowlevel routines form modules.
> 
> Please find a way to do this in core code, or find more highlevel
> primitives to export.
> 
> And not even Ccing linux-fsdevel on a change like this is one of the few
> things I'd consider extremely offensive.
> 
> 


Ok, after discussing with you and Al on irc, I'm going to rewrite this to not
use the file table.
Neil

[PATCH 3/4] net: use cgroup_attach method to migrate socket priotiy and classid

[Neil Horman]

Now that we have an owner identified for each socket, we can use the
cgroup_attach method to migrate sockets owned by migrating tasks to their new
cgroup instances.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: Thomas Graf <tgraf@infradead.org>
CC: "David S. Miller" <davem@davemloft.net>
---
 net/core/netprio_cgroup.c |   40 +++++++++++++++++++++++++++++++++++++++-
 net/sched/cls_cgroup.c    |   39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+), 1 deletions(-)

diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c
index 3a9fd48..32eef0f 100644
--- a/net/core/netprio_cgroup.c
+++ b/net/core/netprio_cgroup.c
@@ -18,6 +18,7 @@
 #include <linux/cgroup.h>
 #include <linux/rcupdate.h>
 #include <linux/atomic.h>
+#include <linux/fdtable.h>
 #include <net/rtnetlink.h>
 #include <net/pkt_cls.h>
 #include <net/sock.h>
@@ -27,12 +28,14 @@ static struct cgroup_subsys_state *cgrp_create(struct cgroup_subsys *ss,
 					       struct cgroup *cgrp);
 static void cgrp_destroy(struct cgroup_subsys *ss, struct cgroup *cgrp);
 static int cgrp_populate(struct cgroup_subsys *ss, struct cgroup *cgrp);
-
+static void cgrp_attach(struct cgroup_subsys *ss, struct cgroup *cgrp,
+			struct cgroup *old_cgrp, struct task_struct *tsk);
 struct cgroup_subsys net_prio_subsys = {
 	.name		= "net_prio",
 	.create		= cgrp_create,
 	.destroy	= cgrp_destroy,
 	.populate	= cgrp_populate,
+	.attach		= cgrp_attach,
 #ifdef CONFIG_NETPRIO_CGROUP
 	.subsys_id	= net_prio_subsys_id,
 #endif
@@ -265,6 +268,41 @@ static int cgrp_populate(struct cgroup_subsys *ss, struct cgroup *cgrp)
 	return cgroup_add_files(cgrp, ss, ss_files, ARRAY_SIZE(ss_files));
 }
 
+static void cgrp_attach(struct cgroup_subsys *ss, struct cgroup *cgrp,
+			struct cgroup *old_cgrp, struct task_struct *tsk)
+{
+	int fd, err;
+	struct files_struct *files;
+	struct file *file;
+	struct socket *sock;
+	struct cgroup_netprio_state *old, *new;
+	pid_t task_pid;
+ 
+	files = get_files_struct(tsk);
+	task_pid = task_pid_nr(tsk);
+ 
+	old = cgrp_netprio_state(old_cgrp);
+	new = cgrp_netprio_state(cgrp);
+ 
+	spin_lock(&files->file_lock);
+	for (fd=0; fd < files_fdtable(files)->max_fds; fd++) {
+		file = fcheck_files(files, fd);
+		if (!file)
+			continue;
+		sock = sock_from_file(file, &err);
+		if (!sock)
+			continue;
+		if (!sock->sk)
+			continue;
+		if (sock->sk->sk_cgrp_owner == task_pid)
+			sock->sk->sk_cgrp_prioidx = new->prioidx;
+	}
+	spin_unlock(&files->file_lock);
+ 
+	put_files_struct(files);
+}
+ 
+
 static int netprio_device_event(struct notifier_block *unused,
 				unsigned long event, void *ptr)
 {
diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index f84fdc3..7020cda 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -17,6 +17,7 @@
 #include <linux/skbuff.h>
 #include <linux/cgroup.h>
 #include <linux/rcupdate.h>
+#include <linux/fdtable.h>
 #include <net/rtnetlink.h>
 #include <net/pkt_cls.h>
 #include <net/sock.h>
@@ -26,12 +27,15 @@ static struct cgroup_subsys_state *cgrp_create(struct cgroup_subsys *ss,
 					       struct cgroup *cgrp);
 static void cgrp_destroy(struct cgroup_subsys *ss, struct cgroup *cgrp);
 static int cgrp_populate(struct cgroup_subsys *ss, struct cgroup *cgrp);
+static void cgrp_attach(struct cgroup_subsys *ss, struct cgroup *cgrp,
+			struct cgroup *old_cgrp, struct task_struct *tsk);
 
 struct cgroup_subsys net_cls_subsys = {
 	.name		= "net_cls",
 	.create		= cgrp_create,
 	.destroy	= cgrp_destroy,
 	.populate	= cgrp_populate,
+	.attach		= cgrp_attach,
 #ifdef CONFIG_NET_CLS_CGROUP
 	.subsys_id	= net_cls_subsys_id,
 #endif
@@ -95,6 +99,41 @@ static int cgrp_populate(struct cgroup_subsys *ss, struct cgroup *cgrp)
 	return cgroup_add_files(cgrp, ss, ss_files, ARRAY_SIZE(ss_files));
 }
 
+static void cgrp_attach(struct cgroup_subsys *ss, struct cgroup *cgrp,
+			struct cgroup *old_cgrp, struct task_struct *tsk)
+{
+	int fd, err;
+	struct files_struct *files;
+	struct file *file;
+	struct socket *sock;
+	struct cgroup_cls_state *old, *new;
+	pid_t task_pid;
+ 
+	files = get_files_struct(tsk);
+	task_pid = task_pid_nr(tsk);
+ 
+	old = cgrp_cls_state(old_cgrp);
+	new = cgrp_cls_state(cgrp);
+ 
+	spin_lock(&files->file_lock);
+	for (fd=0; fd < files_fdtable(files)->max_fds; fd++) {
+		file = fcheck_files(files, fd);
+		if (!file)
+			continue;
+		sock = sock_from_file(file, &err);
+		if (!sock)
+			continue;
+		if (!sock->sk)
+			continue;
+		if (sock->sk->sk_cgrp_owner == task_pid)
+			sock->sk->sk_classid = new->classid;
+	}
+	spin_unlock(&files->file_lock);
+ 
+	put_files_struct(files);
+}
+ 
+
 struct cls_cgroup_head {
 	u32			handle;
 	struct tcf_exts		exts;
-- 
1.7.6.4

Re: [PATCH 3/4] net: use cgroup_attach method to migrate socket priotiy and classid

[Al Viro]

On Wed, Dec 21, 2011 at 09:39:49AM -0500, Neil Horman wrote:
> +static void cgrp_attach(struct cgroup_subsys *ss, struct cgroup *cgrp,
> +			struct cgroup *old_cgrp, struct task_struct *tsk)
> +{
> +	int fd, err;
> +	struct files_struct *files;
> +	struct file *file;
> +	struct socket *sock;
> +	struct cgroup_netprio_state *old, *new;
> +	pid_t task_pid;
> + 
> +	files = get_files_struct(tsk);
> +	task_pid = task_pid_nr(tsk);
> + 
> +	old = cgrp_netprio_state(old_cgrp);
> +	new = cgrp_netprio_state(cgrp);
> + 
> +	spin_lock(&files->file_lock);
> +	for (fd=0; fd < files_fdtable(files)->max_fds; fd++) {
> +		file = fcheck_files(files, fd);
> +		if (!file)
> +			continue;
> +		sock = sock_from_file(file, &err);
> +		if (!sock)
> +			continue;
> +		if (!sock->sk)
> +			continue;
> +		if (sock->sk->sk_cgrp_owner == task_pid)
> +			sock->sk->sk_cgrp_prioidx = new->prioidx;

This looks bogus *and* racy; what about e.g. files currently in SCM_RIGHTS
datagrams?

[PATCH 4/4] net: remove no-longer needed calls to sock_update_[classid|netprioix]

[Neil Horman]

Now that we can use cgroups to re-write individual sockets classid and priroity,
we no longer have to update them on each send/recieve

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: Thomas Graf <tgraf@infradead.org>
CC: "David S. Miller" <davem@davemloft.net>
---
 drivers/net/tun.c            |    2 --
 include/net/netprio_cgroup.h |    6 ------
 include/net/sock.h           |    8 --------
 net/core/sock.c              |    9 +++++----
 net/socket.c                 |   10 ----------
 5 files changed, 5 insertions(+), 30 deletions(-)

diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 93c5d72..cf70336 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -582,8 +582,6 @@ static struct sk_buff *tun_alloc_skb(struct tun_struct *tun,
 	struct sk_buff *skb;
 	int err;
 
-	sock_update_classid(sk);
-
 	/* Under a page?  Don't bother with paged skb. */
 	if (prepad + len < PAGE_SIZE || !linear)
 		linear = len;
diff --git a/include/net/netprio_cgroup.h b/include/net/netprio_cgroup.h
index e503b87..9c794ab 100644
--- a/include/net/netprio_cgroup.h
+++ b/include/net/netprio_cgroup.h
@@ -36,8 +36,6 @@ struct cgroup_netprio_state {
 extern int net_prio_subsys_id;
 #endif
 
-extern void sock_update_netprioidx(struct sock *sk);
-
 static inline struct cgroup_netprio_state
 		*task_netprio_state(struct task_struct *p)
 {
@@ -48,10 +46,6 @@ static inline struct cgroup_netprio_state
 	return NULL;
 #endif
 }
-
-#else
-
-#define sock_update_netprioidx(sk)
 #endif
 
 #endif  /* _NET_CLS_CGROUP_H */
diff --git a/include/net/sock.h b/include/net/sock.h
index cdb03c2..df30db0 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1367,14 +1367,6 @@ extern void *sock_kmalloc(struct sock *sk, int size,
 extern void sock_kfree_s(struct sock *sk, void *mem, int size);
 extern void sk_send_sigurg(struct sock *sk);
 
-#ifdef CONFIG_CGROUPS
-extern void sock_update_classid(struct sock *sk);
-#else
-static inline void sock_update_classid(struct sock *sk)
-{
-}
-#endif
-
 /*
  * Functions to fill in entries in struct proto_ops when a protocol
  * does not implement a particular function.
diff --git a/net/core/sock.c b/net/core/sock.c
index b922fb5..cfc2d10 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1160,7 +1160,7 @@ static void sk_prot_free(struct proto *prot, struct sock *sk)
 }
 
 #ifdef CONFIG_CGROUPS
-void sock_update_classid(struct sock *sk)
+static void sock_update_classid(struct sock *sk)
 {
 	pid_t tpid;
 	u32 classid;
@@ -1174,9 +1174,8 @@ void sock_update_classid(struct sock *sk)
 	    (classid && classid != sk->sk_classid))
 		sk->sk_classid = classid;
 }
-EXPORT_SYMBOL(sock_update_classid);
 
-void sock_update_netprioidx(struct sock *sk)
+static void sock_update_netprioidx(struct sock *sk)
 {
 	pid_t tpid;
 	struct cgroup_netprio_state *state;
@@ -1189,7 +1188,9 @@ void sock_update_netprioidx(struct sock *sk)
 		sk->sk_cgrp_prioidx = state ? state->prioidx : 0;
 	rcu_read_unlock();
 }
-EXPORT_SYMBOL_GPL(sock_update_netprioidx);
+#else
+#define sock_update_classid(sk)
+#define sock_update_netprioidx(sk)
 #endif
 
 /**
diff --git a/net/socket.c b/net/socket.c
index eab8db1..3c6693a 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -550,10 +550,6 @@ static inline int __sock_sendmsg_nosec(struct kiocb *iocb, struct socket *sock,
 {
 	struct sock_iocb *si = kiocb_to_siocb(iocb);
 
-	sock_update_classid(sock->sk);
-
-	sock_update_netprioidx(sock->sk);
-
 	si->sock = sock;
 	si->scm = NULL;
 	si->msg = msg;
@@ -716,8 +712,6 @@ static inline int __sock_recvmsg_nosec(struct kiocb *iocb, struct socket *sock,
 {
 	struct sock_iocb *si = kiocb_to_siocb(iocb);
 
-	sock_update_classid(sock->sk);
-
 	si->sock = sock;
 	si->scm = NULL;
 	si->msg = msg;
@@ -828,8 +822,6 @@ static ssize_t sock_splice_read(struct file *file, loff_t *ppos,
 	if (unlikely(!sock->ops->splice_read))
 		return -EINVAL;
 
-	sock_update_classid(sock->sk);
-
 	return sock->ops->splice_read(sock, ppos, pipe, len, flags);
 }
 
@@ -3376,8 +3368,6 @@ EXPORT_SYMBOL(kernel_setsockopt);
 int kernel_sendpage(struct socket *sock, struct page *page, int offset,
 		    size_t size, int flags)
 {
-	sock_update_classid(sock->sk);
-
 	if (sock->ops->sendpage)
 		return sock->ops->sendpage(sock, page, offset, size, flags);
 
-- 
1.7.6.4