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

2012-08-14 Thread Neil Horman 
On Mon, Aug 13, 2012 at 07:43:21PM -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 
> Reported-by: Al Viro 
> Signed-off-by: John Fastabend 
> ---
> 
>  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(>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(>f_path, tmp, PAGE_SIZE);
> - rv = sscanf(path, "socket:[%lu]", );
> - if (rv <= 0)
> - continue;
> -
>   sock = sock_from_file(file, );
> - if (!err)
> + if (sock)
>   sock_update_netprioidx(sock->sk, p);
>   }
> - rcu_read_unlock();
> + spin_unlock(>file_lock);
>   task_unlock(p);
>   }
> - kfree(tmp);
>  }
>  
>  static struct cftype ss_files[] = {
> 
This looks ok to me, but I've already shown my inability to review code that
interfaces with VFS.  Al, what do you think?

Neil

> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-08-14 Thread Neil Horman 
On Mon, Aug 13, 2012 at 07:43:21PM -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 nhor...@tuxdriver.com
 Reported-by: Al Viro v...@zeniv.linux.org.uk
 Signed-off-by: John Fastabend john.r.fastab...@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[] = {
 
This looks ok to me, but I've already shown my inability to review code that
interfaces with VFS.  Al, what do you think?

Neil

 --
 To unsubscribe from this list: send the line unsubscribe netdev in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-08-13 Thread 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 
Reported-by: Al Viro 
Signed-off-by: John Fastabend 
---

 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(>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(>f_path, tmp, PAGE_SIZE);
-   rv = sscanf(path, "socket:[%lu]", );
-   if (rv <= 0)
-   continue;
-
sock = sock_from_file(file, );
-   if (!err)
+   if (sock)
sock_update_netprioidx(sock->sk, p);
}
-   rcu_read_unlock();
+   spin_unlock(>file_lock);
task_unlock(p);
}
-   kfree(tmp);
 }
 
 static struct cftype ss_files[] = {

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2012-08-13 Thread 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 nhor...@tuxdriver.com
Reported-by: Al Viro v...@zeniv.linux.org.uk
Signed-off-by: John Fastabend john.r.fastab...@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[] = {

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/