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 <[email protected]>
Reported-by: Al Viro <[email protected]>
Signed-off-by: John Fastabend <[email protected]>
---
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[] = {
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 <[email protected]>
Reported-by: Al Viro <[email protected]>
Signed-off-by: John Fastabend <[email protected]>
---
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]);
}
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 <[email protected]>
Reported-by: Al Viro <[email protected]>
Signed-off-by: John Fastabend <[email protected]>
---
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:
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 <[email protected]>
> Reported-by: Al Viro <[email protected]>
> Signed-off-by: John Fastabend <[email protected]>
> ---
>
> 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 <[email protected]>
It looks good to me. Al, could you please lend your review here too?
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 <[email protected]>
> Reported-by: Al Viro <[email protected]>
> Signed-off-by: John Fastabend <[email protected]>
> ---
>
> 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 <[email protected]>
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 <[email protected]>
> Reported-by: Al Viro <[email protected]>
> Signed-off-by: John Fastabend <[email protected]>
> ---
>
> 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 <[email protected]>
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 <[email protected]>
> > Reported-by: Al Viro <[email protected]>
> > Signed-off-by: John Fastabend <[email protected]>
> > ---
> >
> > 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 <[email protected]>
>
> 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.
From: John Fastabend <[email protected]>
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 <[email protected]>
> Reported-by: Al Viro <[email protected]>
> Signed-off-by: John Fastabend <[email protected]>
Applied.
From: John Fastabend <[email protected]>
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 <[email protected]>
> Reported-by: Al Viro <[email protected]>
> Signed-off-by: John Fastabend <[email protected]>
Applied.
From: John Fastabend <[email protected]>
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 <[email protected]>
> Reported-by: Al Viro <[email protected]>
> Signed-off-by: John Fastabend <[email protected]>
Applied.