From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Dumazet Subject: Re: [PATCH 2/3] fix kcm_clone() Date: Fri, 01 Dec 2017 01:31:51 -0800 Message-ID: <1512120711.19682.32.camel@gmail.com> References: <20171201002027.GI21978@ZenIV.linux.org.uk> <20171201002244.GK21978@ZenIV.linux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit Cc: Tom Herbert To: Al Viro , netdev@vger.kernel.org Return-path: Received: from mail-io0-f194.google.com ([209.85.223.194]:34528 "EHLO mail-io0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750872AbdLAJby (ORCPT ); Fri, 1 Dec 2017 04:31:54 -0500 Received: by mail-io0-f194.google.com with SMTP id s19so10621811ioa.1 for ; Fri, 01 Dec 2017 01:31:54 -0800 (PST) In-Reply-To: <20171201002244.GK21978@ZenIV.linux.org.uk> Sender: netdev-owner@vger.kernel.org List-ID: 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 > --- >  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: