[net PATCH v3 1/3] net: netprio: fix files lock and remove useless d_path bits

[net PATCH v3 1/3] net: netprio: fix files lock and remove useless d_path bits

[John Fastabend]

Add lock to prevent a race with a file closing and also remove
useless and ugly sscanf code. The extra code was never needed
and the case it supposedly protected against is in fact handled
correctly by sock_from_file as pointed out by Al Viro.

CC: Neil Horman <nhorman@tuxdriver.com>
Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
---

 net/core/netprio_cgroup.c |   22 ++++------------------
 1 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c
index ed0c043..f65dba3 100644
--- a/net/core/netprio_cgroup.c
+++ b/net/core/netprio_cgroup.c
@@ -277,12 +277,6 @@ out_free_devname:
 void net_prio_attach(struct cgroup *cgrp, struct cgroup_taskset *tset)
 {
 	struct task_struct *p;
-	char *tmp = kzalloc(sizeof(char) * PATH_MAX, GFP_KERNEL);
-
-	if (!tmp) {
-		pr_warn("Unable to attach cgrp due to alloc failure!\n");
-		return;
-	}
 
 	cgroup_taskset_for_each(p, cgrp, tset) {
 		unsigned int fd;
@@ -296,32 +290,24 @@ void net_prio_attach(struct cgroup *cgrp, struct cgroup_taskset *tset)
 			continue;
 		}
 
-		rcu_read_lock();
+		spin_lock(&files->file_lock);
 		fdt = files_fdtable(files);
 		for (fd = 0; fd < fdt->max_fds; fd++) {
-			char *path;
 			struct file *file;
 			struct socket *sock;
-			unsigned long s;
-			int rv, err = 0;
+			int err;
 
 			file = fcheck_files(files, fd);
 			if (!file)
 				continue;
 
-			path = d_path(&file->f_path, tmp, PAGE_SIZE);
-			rv = sscanf(path, "socket:[%lu]", &s);
-			if (rv <= 0)
-				continue;
-
 			sock = sock_from_file(file, &err);
-			if (!err)
+			if (sock)
 				sock_update_netprioidx(sock->sk, p);
 		}
-		rcu_read_unlock();
+		spin_unlock(&files->file_lock);
 		task_unlock(p);
 	}
-	kfree(tmp);
 }
 
 static struct cftype ss_files[] = {

[net PATCH v3 2/3] net: netprio: fd passed in SCM_RIGHTS datagram not set correctly

[John Fastabend]

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.

Thanks to Al Viro for catching this.

CC: Neil Horman <nhorman@tuxdriver.com>
Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
---

 net/core/scm.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/net/core/scm.c b/net/core/scm.c
index 8f6ccfd..040cebe 100644
--- a/net/core/scm.c
+++ b/net/core/scm.c
@@ -265,6 +265,7 @@ void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
 	for (i=0, cmfptr=(__force int __user *)CMSG_DATA(cm); i<fdmax;
 	     i++, cmfptr++)
 	{
+		struct socket *sock;
 		int new_fd;
 		err = security_file_receive(fp[i]);
 		if (err)
@@ -281,6 +282,9 @@ void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
 		}
 		/* Bump the usage count and install the file. */
 		get_file(fp[i]);
+		sock = sock_from_file(fp[i], &err);
+		if (sock)
+			sock_update_netprioidx(sock->sk, current);
 		fd_install(new_fd, fp[i]);
 	}
 

Re: [net PATCH v3 2/3] net: netprio: fd passed in SCM_RIGHTS datagram not set correctly

[Neil Horman]

On Tue, Aug 14, 2012 at 03:34:30PM -0700, John Fastabend wrote:
> 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.
> 
> Thanks to Al Viro for catching this.
> 
> CC: Neil Horman <nhorman@tuxdriver.com>
> Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> ---
> 
>  net/core/scm.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/net/core/scm.c b/net/core/scm.c
> index 8f6ccfd..040cebe 100644
> --- a/net/core/scm.c
> +++ b/net/core/scm.c
> @@ -265,6 +265,7 @@ void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
>  	for (i=0, cmfptr=(__force int __user *)CMSG_DATA(cm); i<fdmax;
>  	     i++, cmfptr++)
>  	{
> +		struct socket *sock;
>  		int new_fd;
>  		err = security_file_receive(fp[i]);
>  		if (err)
> @@ -281,6 +282,9 @@ void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
>  		}
>  		/* Bump the usage count and install the file. */
>  		get_file(fp[i]);
> +		sock = sock_from_file(fp[i], &err);
> +		if (sock)
> +			sock_update_netprioidx(sock->sk, current);
>  		fd_install(new_fd, fp[i]);
>  	}
>  
> 
> 

Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [net PATCH v3 2/3] net: netprio: fd passed in SCM_RIGHTS datagram not set correctly

[David Miller]

From: John Fastabend <john.r.fastabend@intel.com>
Date: Tue, 14 Aug 2012 15:34:30 -0700

> 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.
> 
> Thanks to Al Viro for catching this.
> 
> CC: Neil Horman <nhorman@tuxdriver.com>
> Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>

Applied.

[net PATCH v3 3/3] net: netprio: fix cgrp create and write priomap race

[John Fastabend]

A race exists where creating cgroups and also updating the priomap
may result in losing a priomap update. This is because priomap
writers are not protected by rtnl_lock.

Move priority writer into rtnl_lock()/rtnl_unlock().

CC: Neil Horman <nhorman@tuxdriver.com>
Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
---

 net/core/netprio_cgroup.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c
index f65dba3..c75e3f9 100644
--- a/net/core/netprio_cgroup.c
+++ b/net/core/netprio_cgroup.c
@@ -101,12 +101,10 @@ static int write_update_netdev_table(struct net_device *dev)
 	u32 max_len;
 	struct netprio_map *map;
 
-	rtnl_lock();
 	max_len = atomic_read(&max_prioidx) + 1;
 	map = rtnl_dereference(dev->priomap);
 	if (!map || map->priomap_len < max_len)
 		ret = extend_netdev_table(dev, max_len);
-	rtnl_unlock();
 
 	return ret;
 }
@@ -256,17 +254,17 @@ static int write_priomap(struct cgroup *cgrp, struct cftype *cft,
 	if (!dev)
 		goto out_free_devname;
 
+	rtnl_lock();
 	ret = write_update_netdev_table(dev);
 	if (ret < 0)
 		goto out_put_dev;
 
-	rcu_read_lock();
-	map = rcu_dereference(dev->priomap);
+	map = rtnl_dereference(dev->priomap);
 	if (map)
 		map->priomap[prioidx] = priority;
-	rcu_read_unlock();
 
 out_put_dev:
+	rtnl_unlock();
 	dev_put(dev);
 
 out_free_devname:

Re: [net PATCH v3 3/3] net: netprio: fix cgrp create and write priomap race

[Neil Horman]

On Tue, Aug 14, 2012 at 03:34:35PM -0700, John Fastabend wrote:
> A race exists where creating cgroups and also updating the priomap
> may result in losing a priomap update. This is because priomap
> writers are not protected by rtnl_lock.
> 
> Move priority writer into rtnl_lock()/rtnl_unlock().
> 
> CC: Neil Horman <nhorman@tuxdriver.com>
> Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> ---
> 
>  net/core/netprio_cgroup.c |    8 +++-----
>  1 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c
> index f65dba3..c75e3f9 100644
> --- a/net/core/netprio_cgroup.c
> +++ b/net/core/netprio_cgroup.c
> @@ -101,12 +101,10 @@ static int write_update_netdev_table(struct net_device *dev)
>  	u32 max_len;
>  	struct netprio_map *map;
>  
> -	rtnl_lock();
>  	max_len = atomic_read(&max_prioidx) + 1;
>  	map = rtnl_dereference(dev->priomap);
>  	if (!map || map->priomap_len < max_len)
>  		ret = extend_netdev_table(dev, max_len);
> -	rtnl_unlock();
>  
>  	return ret;
>  }
> @@ -256,17 +254,17 @@ static int write_priomap(struct cgroup *cgrp, struct cftype *cft,
>  	if (!dev)
>  		goto out_free_devname;
>  
> +	rtnl_lock();
>  	ret = write_update_netdev_table(dev);
>  	if (ret < 0)
>  		goto out_put_dev;
>  
> -	rcu_read_lock();
> -	map = rcu_dereference(dev->priomap);
> +	map = rtnl_dereference(dev->priomap);
>  	if (map)
>  		map->priomap[prioidx] = priority;
> -	rcu_read_unlock();
>  
>  out_put_dev:
> +	rtnl_unlock();
>  	dev_put(dev);
>  
>  out_free_devname:
> 
> 

Acked-by: Neil Horman <nhorman@tuxdriver.com>

Re: [net PATCH v3 3/3] net: netprio: fix cgrp create and write priomap race

[David Miller]

From: John Fastabend <john.r.fastabend@intel.com>
Date: Tue, 14 Aug 2012 15:34:35 -0700

> A race exists where creating cgroups and also updating the priomap
> may result in losing a priomap update. This is because priomap
> writers are not protected by rtnl_lock.
> 
> Move priority writer into rtnl_lock()/rtnl_unlock().
> 
> CC: Neil Horman <nhorman@tuxdriver.com>
> Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>

Applied.

Re: [net PATCH v3 1/3] net: netprio: fix files lock and remove useless d_path bits

[Neil Horman]

On Tue, Aug 14, 2012 at 03:34:24PM -0700, John Fastabend wrote:
> Add lock to prevent a race with a file closing and also remove
> useless and ugly sscanf code. The extra code was never needed
> and the case it supposedly protected against is in fact handled
> correctly by sock_from_file as pointed out by Al Viro.
> 
> CC: Neil Horman <nhorman@tuxdriver.com>
> Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> ---
> 
>  net/core/netprio_cgroup.c |   22 ++++------------------
>  1 files changed, 4 insertions(+), 18 deletions(-)
> 
> diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c
> index ed0c043..f65dba3 100644
> --- a/net/core/netprio_cgroup.c
> +++ b/net/core/netprio_cgroup.c
> @@ -277,12 +277,6 @@ out_free_devname:
>  void net_prio_attach(struct cgroup *cgrp, struct cgroup_taskset *tset)
>  {
>  	struct task_struct *p;
> -	char *tmp = kzalloc(sizeof(char) * PATH_MAX, GFP_KERNEL);
> -
> -	if (!tmp) {
> -		pr_warn("Unable to attach cgrp due to alloc failure!\n");
> -		return;
> -	}
>  
>  	cgroup_taskset_for_each(p, cgrp, tset) {
>  		unsigned int fd;
> @@ -296,32 +290,24 @@ void net_prio_attach(struct cgroup *cgrp, struct cgroup_taskset *tset)
>  			continue;
>  		}
>  
> -		rcu_read_lock();
> +		spin_lock(&files->file_lock);
>  		fdt = files_fdtable(files);
>  		for (fd = 0; fd < fdt->max_fds; fd++) {
> -			char *path;
>  			struct file *file;
>  			struct socket *sock;
> -			unsigned long s;
> -			int rv, err = 0;
> +			int err;
>  
>  			file = fcheck_files(files, fd);
>  			if (!file)
>  				continue;
>  
> -			path = d_path(&file->f_path, tmp, PAGE_SIZE);
> -			rv = sscanf(path, "socket:[%lu]", &s);
> -			if (rv <= 0)
> -				continue;
> -
>  			sock = sock_from_file(file, &err);
> -			if (!err)
> +			if (sock)
>  				sock_update_netprioidx(sock->sk, p);
>  		}
> -		rcu_read_unlock();
> +		spin_unlock(&files->file_lock);
>  		task_unlock(p);
>  	}
> -	kfree(tmp);
>  }
>  
>  static struct cftype ss_files[] = {
> 
> 
Acked-by: Neil Horman <nhorman@tuxdriver.com>

It looks good to me.  Al, could you please lend your review here too?

Re: [net PATCH v3 1/3] net: netprio: fix files lock and remove useless d_path bits

[Al Viro]

On Wed, Aug 15, 2012 at 06:52:06AM -0400, Neil Horman wrote:
> On Tue, Aug 14, 2012 at 03:34:24PM -0700, John Fastabend wrote:
> > Add lock to prevent a race with a file closing and also remove
> > useless and ugly sscanf code. The extra code was never needed
> > and the case it supposedly protected against is in fact handled
> > correctly by sock_from_file as pointed out by Al Viro.
> > 
> > CC: Neil Horman <nhorman@tuxdriver.com>
> > Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
> > Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> > ---
> > 
> >  net/core/netprio_cgroup.c |   22 ++++------------------
> >  1 files changed, 4 insertions(+), 18 deletions(-)
> > 
> > diff --git a/net/core/netprio_cgroup.c b/net/core/netprio_cgroup.c
> > index ed0c043..f65dba3 100644
> > --- a/net/core/netprio_cgroup.c
> > +++ b/net/core/netprio_cgroup.c
> > @@ -277,12 +277,6 @@ out_free_devname:
> >  void net_prio_attach(struct cgroup *cgrp, struct cgroup_taskset *tset)
> >  {
> >  	struct task_struct *p;
> > -	char *tmp = kzalloc(sizeof(char) * PATH_MAX, GFP_KERNEL);
> > -
> > -	if (!tmp) {
> > -		pr_warn("Unable to attach cgrp due to alloc failure!\n");
> > -		return;
> > -	}
> >  
> >  	cgroup_taskset_for_each(p, cgrp, tset) {
> >  		unsigned int fd;
> > @@ -296,32 +290,24 @@ void net_prio_attach(struct cgroup *cgrp, struct cgroup_taskset *tset)
> >  			continue;
> >  		}
> >  
> > -		rcu_read_lock();
> > +		spin_lock(&files->file_lock);
> >  		fdt = files_fdtable(files);
> >  		for (fd = 0; fd < fdt->max_fds; fd++) {
> > -			char *path;
> >  			struct file *file;
> >  			struct socket *sock;
> > -			unsigned long s;
> > -			int rv, err = 0;
> > +			int err;
> >  
> >  			file = fcheck_files(files, fd);
> >  			if (!file)
> >  				continue;
> >  
> > -			path = d_path(&file->f_path, tmp, PAGE_SIZE);
> > -			rv = sscanf(path, "socket:[%lu]", &s);
> > -			if (rv <= 0)
> > -				continue;
> > -
> >  			sock = sock_from_file(file, &err);
> > -			if (!err)
> > +			if (sock)
> >  				sock_update_netprioidx(sock->sk, p);
> >  		}
> > -		rcu_read_unlock();
> > +		spin_unlock(&files->file_lock);
> >  		task_unlock(p);
> >  	}
> > -	kfree(tmp);
> >  }
> >  
> >  static struct cftype ss_files[] = {
> > 
> > 
> Acked-by: Neil Horman <nhorman@tuxdriver.com>
> 
> It looks good to me.  Al, could you please lend your review here too?

Tolerable...  I still don't like the idea of iterating through the
descriptor tables, but at least that variant is safe wrt locking and
lifetime rules.

Re: [net PATCH v3 1/3] net: netprio: fix files lock and remove useless d_path bits

[David Miller]

From: John Fastabend <john.r.fastabend@intel.com>
Date: Tue, 14 Aug 2012 15:34:24 -0700

> Add lock to prevent a race with a file closing and also remove
> useless and ugly sscanf code. The extra code was never needed
> and the case it supposedly protected against is in fact handled
> correctly by sock_from_file as pointed out by Al Viro.
> 
> CC: Neil Horman <nhorman@tuxdriver.com>
> Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>

Applied.