| Message ID | 20171201002244.GK21978@ZenIV.linux.org.uk |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [1/3] socketpair(): allocate descriptors first | expand |
Sorry for top posting. Lets CC Tom on this patch ? On Fri, 2017-12-01 at 00:22 +0000, Al Viro wrote: > 1) it's fput() or sock_release(), not both > 2) don't do fd_install() until the last failure exit. > 3) not a bug per se, but... don't attach socket to struct file > until it's set up. > > Take reserving descriptor into the caller, move fd_install() to the > caller, sanitize failure exits and calling conventions. > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- > net/kcm/kcmsock.c | 71 +++++++++++++++++++++---------------------- > ------------ > 1 file changed, 27 insertions(+), 44 deletions(-) > > diff --git a/net/kcm/kcmsock.c b/net/kcm/kcmsock.c > index 0b750a22c4b9..c5fa634e63ca 100644 > --- a/net/kcm/kcmsock.c > +++ b/net/kcm/kcmsock.c > @@ -1625,60 +1625,35 @@ static struct proto kcm_proto = { > }; > > /* Clone a kcm socket. */ > -static int kcm_clone(struct socket *osock, struct kcm_clone *info, > - struct socket **newsockp) > +static struct file *kcm_clone(struct socket *osock) > { > struct socket *newsock; > struct sock *newsk; > - struct file *newfile; > - int err, newfd; > + struct file *file; > > - err = -ENFILE; > newsock = sock_alloc(); > if (!newsock) > - goto out; > + return ERR_PTR(-ENFILE); > > newsock->type = osock->type; > newsock->ops = osock->ops; > > __module_get(newsock->ops->owner); > > - newfd = get_unused_fd_flags(0); > - if (unlikely(newfd < 0)) { > - err = newfd; > - goto out_fd_fail; > - } > - > - newfile = sock_alloc_file(newsock, 0, osock->sk- > >sk_prot_creator->name); > - if (IS_ERR(newfile)) { > - err = PTR_ERR(newfile); > - goto out_sock_alloc_fail; > - } > - > newsk = sk_alloc(sock_net(osock->sk), PF_KCM, GFP_KERNEL, > &kcm_proto, true); > if (!newsk) { > - err = -ENOMEM; > - goto out_sk_alloc_fail; > + sock_release(newsock); > + return ERR_PTR(-ENOMEM); > } > - > sock_init_data(newsock, newsk); > init_kcm_sock(kcm_sk(newsk), kcm_sk(osock->sk)->mux); > > - fd_install(newfd, newfile); > - *newsockp = newsock; > - info->fd = newfd; > - > - return 0; > + file = sock_alloc_file(newsock, 0, osock->sk- > >sk_prot_creator->name); > + if (IS_ERR(file)) > + sock_release(newsock); > > -out_sk_alloc_fail: > - fput(newfile); > -out_sock_alloc_fail: > - put_unused_fd(newfd); > -out_fd_fail: > - sock_release(newsock); > -out: > - return err; > + return file; > } > > static int kcm_ioctl(struct socket *sock, unsigned int cmd, unsigned > long arg) > @@ -1708,17 +1683,25 @@ static int kcm_ioctl(struct socket *sock, > unsigned int cmd, unsigned long arg) > } > case SIOCKCMCLONE: { > struct kcm_clone info; > - struct socket *newsock = NULL; > - > - err = kcm_clone(sock, &info, &newsock); > - if (!err) { > - if (copy_to_user((void __user *)arg, &info, > - sizeof(info))) { > - err = -EFAULT; > - sys_close(info.fd); > - } > - } > + struct file *file; > + > + info.fd = get_unused_fd_flags(0); > + if (unlikely(info.fd < 0)) > + return info.fd; > > + file = kcm_clone(sock); > + if (IS_ERR(file)) { > + put_unused_fd(info.fd); > + return PTR_ERR(file); > + } > + if (copy_to_user((void __user *)arg, &info, > + sizeof(info))) { > + put_unused_fd(info.fd); > + fput(file); > + return -EFAULT; > + } > + fd_install(info.fd, file); > + err = 0; > break; > } > default:
On Thu, Nov 30, 2017 at 4:22 PM, Al Viro <viro@zeniv.linux.org.uk> wrote: > 1) it's fput() or sock_release(), not both > 2) don't do fd_install() until the last failure exit. > 3) not a bug per se, but... don't attach socket to struct file > until it's set up. > > Take reserving descriptor into the caller, move fd_install() to the > caller, sanitize failure exits and calling conventions. > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> Acked-by: Tom Herbert <tom@herbertland.com> Thanks for the fix! I'll try to test some today also. > --- > net/kcm/kcmsock.c | 71 +++++++++++++++++++++---------------------------------- > 1 file changed, 27 insertions(+), 44 deletions(-) > > diff --git a/net/kcm/kcmsock.c b/net/kcm/kcmsock.c > index 0b750a22c4b9..c5fa634e63ca 100644 > --- a/net/kcm/kcmsock.c > +++ b/net/kcm/kcmsock.c > @@ -1625,60 +1625,35 @@ static struct proto kcm_proto = { > }; > > /* Clone a kcm socket. */ > -static int kcm_clone(struct socket *osock, struct kcm_clone *info, > - struct socket **newsockp) > +static struct file *kcm_clone(struct socket *osock) > { > struct socket *newsock; > struct sock *newsk; > - struct file *newfile; > - int err, newfd; > + struct file *file; > > - err = -ENFILE; > newsock = sock_alloc(); > if (!newsock) > - goto out; > + return ERR_PTR(-ENFILE); > > newsock->type = osock->type; > newsock->ops = osock->ops; > > __module_get(newsock->ops->owner); > > - newfd = get_unused_fd_flags(0); > - if (unlikely(newfd < 0)) { > - err = newfd; > - goto out_fd_fail; > - } > - > - newfile = sock_alloc_file(newsock, 0, osock->sk->sk_prot_creator->name); > - if (IS_ERR(newfile)) { > - err = PTR_ERR(newfile); > - goto out_sock_alloc_fail; > - } > - > newsk = sk_alloc(sock_net(osock->sk), PF_KCM, GFP_KERNEL, > &kcm_proto, true); > if (!newsk) { > - err = -ENOMEM; > - goto out_sk_alloc_fail; > + sock_release(newsock); > + return ERR_PTR(-ENOMEM); > } > - > sock_init_data(newsock, newsk); > init_kcm_sock(kcm_sk(newsk), kcm_sk(osock->sk)->mux); > > - fd_install(newfd, newfile); > - *newsockp = newsock; > - info->fd = newfd; > - > - return 0; > + file = sock_alloc_file(newsock, 0, osock->sk->sk_prot_creator->name); > + if (IS_ERR(file)) > + sock_release(newsock); > > -out_sk_alloc_fail: > - fput(newfile); > -out_sock_alloc_fail: > - put_unused_fd(newfd); > -out_fd_fail: > - sock_release(newsock); > -out: > - return err; > + return file; > } > > static int kcm_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg) > @@ -1708,17 +1683,25 @@ static int kcm_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg) > } > case SIOCKCMCLONE: { > struct kcm_clone info; > - struct socket *newsock = NULL; > - > - err = kcm_clone(sock, &info, &newsock); > - if (!err) { > - if (copy_to_user((void __user *)arg, &info, > - sizeof(info))) { > - err = -EFAULT; > - sys_close(info.fd); > - } > - } > + struct file *file; > + > + info.fd = get_unused_fd_flags(0); > + if (unlikely(info.fd < 0)) > + return info.fd; > > + file = kcm_clone(sock); > + if (IS_ERR(file)) { > + put_unused_fd(info.fd); > + return PTR_ERR(file); > + } > + if (copy_to_user((void __user *)arg, &info, > + sizeof(info))) { > + put_unused_fd(info.fd); > + fput(file); > + return -EFAULT; > + } > + fd_install(info.fd, file); > + err = 0; > break; > } > default: > -- > 2.11.0 >
diff --git a/net/kcm/kcmsock.c b/net/kcm/kcmsock.c index 0b750a22c4b9..c5fa634e63ca 100644 --- a/net/kcm/kcmsock.c +++ b/net/kcm/kcmsock.c @@ -1625,60 +1625,35 @@ static struct proto kcm_proto = { }; /* Clone a kcm socket. */ -static int kcm_clone(struct socket *osock, struct kcm_clone *info, - struct socket **newsockp) +static struct file *kcm_clone(struct socket *osock) { struct socket *newsock; struct sock *newsk; - struct file *newfile; - int err, newfd; + struct file *file; - err = -ENFILE; newsock = sock_alloc(); if (!newsock) - goto out; + return ERR_PTR(-ENFILE); newsock->type = osock->type; newsock->ops = osock->ops; __module_get(newsock->ops->owner); - newfd = get_unused_fd_flags(0); - if (unlikely(newfd < 0)) { - err = newfd; - goto out_fd_fail; - } - - newfile = sock_alloc_file(newsock, 0, osock->sk->sk_prot_creator->name); - if (IS_ERR(newfile)) { - err = PTR_ERR(newfile); - goto out_sock_alloc_fail; - } - newsk = sk_alloc(sock_net(osock->sk), PF_KCM, GFP_KERNEL, &kcm_proto, true); if (!newsk) { - err = -ENOMEM; - goto out_sk_alloc_fail; + sock_release(newsock); + return ERR_PTR(-ENOMEM); } - sock_init_data(newsock, newsk); init_kcm_sock(kcm_sk(newsk), kcm_sk(osock->sk)->mux); - fd_install(newfd, newfile); - *newsockp = newsock; - info->fd = newfd; - - return 0; + file = sock_alloc_file(newsock, 0, osock->sk->sk_prot_creator->name); + if (IS_ERR(file)) + sock_release(newsock); -out_sk_alloc_fail: - fput(newfile); -out_sock_alloc_fail: - put_unused_fd(newfd); -out_fd_fail: - sock_release(newsock); -out: - return err; + return file; } static int kcm_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg) @@ -1708,17 +1683,25 @@ static int kcm_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg) } case SIOCKCMCLONE: { struct kcm_clone info; - struct socket *newsock = NULL; - - err = kcm_clone(sock, &info, &newsock); - if (!err) { - if (copy_to_user((void __user *)arg, &info, - sizeof(info))) { - err = -EFAULT; - sys_close(info.fd); - } - } + struct file *file; + + info.fd = get_unused_fd_flags(0); + if (unlikely(info.fd < 0)) + return info.fd; + file = kcm_clone(sock); + if (IS_ERR(file)) { + put_unused_fd(info.fd); + return PTR_ERR(file); + } + if (copy_to_user((void __user *)arg, &info, + sizeof(info))) { + put_unused_fd(info.fd); + fput(file); + return -EFAULT; + } + fd_install(info.fd, file); + err = 0; break; } default:
1) it's fput() or sock_release(), not both 2) don't do fd_install() until the last failure exit. 3) not a bug per se, but... don't attach socket to struct file until it's set up. Take reserving descriptor into the caller, move fd_install() to the caller, sanitize failure exits and calling conventions. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- net/kcm/kcmsock.c | 71 +++++++++++++++++++++---------------------------------- 1 file changed, 27 insertions(+), 44 deletions(-)