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 | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/net/core/scm.c b/net/core/scm.c
index 8f6ccfd..a14d9e2 100644
--- a/net/core/scm.c
+++ b/net/core/scm.c
@@ -249,6 +249,7 @@ void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
struct file **fp = scm->fp->fp;
int __user *cmfptr;
int err = 0, i;
+ __u32 prioidx = task_netprioidx(current);
if (MSG_CMSG_COMPAT & msg->msg_flags) {
scm_detach_fds_compat(msg, scm);
@@ -265,6 +266,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 +283,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->sk->sk_cgrp_prioidx = prioidx;
fd_install(new_fd, fp[i]);
}
On Mon, Aug 13, 2012 at 07:43:27PM -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 | 5 +++++
> 1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/net/core/scm.c b/net/core/scm.c
> index 8f6ccfd..a14d9e2 100644
> --- a/net/core/scm.c
> +++ b/net/core/scm.c
> @@ -249,6 +249,7 @@ void scm_detach_fds(struct msghdr *msg, struct scm_cookie *scm)
> struct file **fp = scm->fp->fp;
> int __user *cmfptr;
> int err = 0, i;
> + __u32 prioidx = task_netprioidx(current);
>
> if (MSG_CMSG_COMPAT & msg->msg_flags) {
> scm_detach_fds_compat(msg, scm);
> @@ -265,6 +266,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 +283,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->sk->sk_cgrp_prioidx = prioidx;
nit: You can replace the prioidx variable above and this set with a call to
sock_update_netprioidx
Neil
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 <[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[] = {
>
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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On 8/14/2012 5:55 AM, Neil Horman wrote:
> On Mon, Aug 13, 2012 at 07:43:27PM -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]>
>> ---
>>
[...]
>> @@ -281,6 +283,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->sk->sk_cgrp_prioidx = prioidx;
> nit: You can replace the prioidx variable above and this set with a call to
> sock_update_netprioidx
>
> Neil
>
OK but then I should also make sock_update_netprioidx inline and drop
the in_interrupt() call. I'll send a v3 with this change and also a
third patch to fix a race between write_priomap and cgrp_create (also
spotted by Al Viro).