[RFC][PATCHES] sock_alloc_file() cleanups and fixes

[RFC][PATCHES] sock_alloc_file() cleanups and fixes

[Al Viro]

	Almost all sock_alloc_file() callers want sock_release()
in case of failure.  Currently it consumes socket on success
(it will be destroyed on final fput() of resulting file) and
leaves it alone on failure.  Making it consume socket in all
cases makes for simpler life in callers.

	There are 3 exceptions:

* sock_map_fd() calls sock_alloc_file(), but doesn't do sock_release()
  in case of failure.  Its caller (sys_socket()) does, though, and it
  does get considerably simpler with sock_alloc_file() doing the cleanup
  in case of failure.

* sys_socketpair().  Handling of sock_alloc_file() failures is complicated
  by attempts to share bits and pieces of failure exits between various
  points of failure in there.  Reordering things a bit (reserving descriptors
  and copying them to userland before doing anything else) makes for simpler
  handling of failure exits and after such massage we get the situation
  when failure of sock_alloc_file() is immediately followed by sock_release().

* kcm_clone().  Badly broken in several respects - sk_alloc() failure ends
  up with double-free of struct socket (we do fput(), then sock_release())
  and copy_to_user() failure uses sys_close() to undo fd_install(), which
  is something we should never do.  Descriptor table might be shared and
  fd_install() should only be done past the last possible failure point.
  Fixing all of that is simple - we just need to move allocation of
  descriptor and fd_install() into the caller (before and after the call of
  kcm_clone(), resp.) and untangle the failure exits.  Makes for much simpler
  calling conventions for kcm_clone(), while we are at it, and as a side
  effect we get "sock_release() in case of sock_alloc_file() failure" for
  that one as well.

	The patch series (in followups to this mail and in
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git#work.net) does
the following:
	1) massage sys_socketpair() (should be a pure cleanup)
	2) fix and clean up kcm_clone() (-stable fodder)
	3) switch sock_alloc_file() to new calling conventions.

	It got some local testing, but it certainly needs more review.
Diffstat for the entire thing is
 drivers/staging/lustre/lnet/lnet/lib-socket.c |   8 ++---
 net/9p/trans_fd.c                             |   1 -
 net/kcm/kcmsock.c                             |  68 ++++++++++++++---------------------------
 net/sctp/socket.c                             |   1 -
 net/socket.c                                  | 110 +++++++++++++++++++++++++++----------------------------------------
 5 files changed, 69 insertions(+), 119 deletions(-)

	Please, review and comment.

[PATCH 1/3] socketpair(): allocate descriptors first

[Al Viro]

simplifies failure exits considerably...

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 net/socket.c | 89 ++++++++++++++++++++++++++----------------------------------
 1 file changed, 38 insertions(+), 51 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index 42d8e9c9ccd5..2df83c0bfde9 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1366,87 +1366,74 @@ SYSCALL_DEFINE4(socketpair, int, family, int, type, int, protocol,
 		flags = (flags & ~SOCK_NONBLOCK) | O_NONBLOCK;
 
 	/*
+	 * reserve descriptors and make sure we won't fail
+	 * to return them to userland.
+	 */
+	fd1 = get_unused_fd_flags(flags);
+	if (unlikely(fd1 < 0))
+		return fd1;
+
+	fd2 = get_unused_fd_flags(flags);
+	if (unlikely(fd2 < 0)) {
+		put_unused_fd(fd1);
+		return fd2;
+	}
+
+	err = put_user(fd1, &usockvec[0]);
+	if (err)
+		goto out;
+
+	err = put_user(fd2, &usockvec[1]);
+	if (err)
+		goto out;
+
+	/*
 	 * Obtain the first socket and check if the underlying protocol
 	 * supports the socketpair call.
 	 */
 
 	err = sock_create(family, type, protocol, &sock1);
-	if (err < 0)
+	if (unlikely(err < 0))
 		goto out;
 
 	err = sock_create(family, type, protocol, &sock2);
-	if (err < 0)
-		goto out_release_1;
-
-	err = sock1->ops->socketpair(sock1, sock2);
-	if (err < 0)
-		goto out_release_both;
-
-	fd1 = get_unused_fd_flags(flags);
-	if (unlikely(fd1 < 0)) {
-		err = fd1;
-		goto out_release_both;
+	if (unlikely(err < 0)) {
+		sock_release(sock1);
+		goto out;
 	}
 
-	fd2 = get_unused_fd_flags(flags);
-	if (unlikely(fd2 < 0)) {
-		err = fd2;
-		goto out_put_unused_1;
+	err = sock1->ops->socketpair(sock1, sock2);
+	if (unlikely(err < 0)) {
+		sock_release(sock2);
+		sock_release(sock1);
+		goto out;
 	}
 
 	newfile1 = sock_alloc_file(sock1, flags, NULL);
 	if (IS_ERR(newfile1)) {
 		err = PTR_ERR(newfile1);
-		goto out_put_unused_both;
+		sock_release(sock1);
+		sock_release(sock2);
+		goto out;
 	}
 
 	newfile2 = sock_alloc_file(sock2, flags, NULL);
 	if (IS_ERR(newfile2)) {
 		err = PTR_ERR(newfile2);
-		goto out_fput_1;
+		sock_release(sock2);
+		fput(newfile1);
+		goto out;
 	}
 
-	err = put_user(fd1, &usockvec[0]);
-	if (err)
-		goto out_fput_both;
-
-	err = put_user(fd2, &usockvec[1]);
-	if (err)
-		goto out_fput_both;
-
 	audit_fd_pair(fd1, fd2);
 
 	fd_install(fd1, newfile1);
 	fd_install(fd2, newfile2);
-	/* fd1 and fd2 may be already another descriptors.
-	 * Not kernel problem.
-	 */
-
 	return 0;
 
-out_fput_both:
-	fput(newfile2);
-	fput(newfile1);
-	put_unused_fd(fd2);
-	put_unused_fd(fd1);
-	goto out;
-
-out_fput_1:
-	fput(newfile1);
-	put_unused_fd(fd2);
-	put_unused_fd(fd1);
-	sock_release(sock2);
-	goto out;
-
-out_put_unused_both:
+out:
 	put_unused_fd(fd2);
-out_put_unused_1:
 	put_unused_fd(fd1);
-out_release_both:
-	sock_release(sock2);
-out_release_1:
-	sock_release(sock1);
-out:
 	return err;
 }
 
-- 
2.11.0

[PATCH 2/3] fix kcm_clone()

[Al Viro]

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:
-- 
2.11.0

[PATCH 3/3] make sock_alloc_file() do sock_release() on failures

[Al Viro]

This changes calling conventions (and simplifies the hell out
the callers).  New rules: once struct socket had been passed
to sock_alloc_file(), it's been consumed either by struct file
or by sock_release() done by sock_alloc_file().  Either way
the caller should not do sock_release() after that point.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/staging/lustre/lnet/lnet/lib-socket.c |  8 ++------
 net/9p/trans_fd.c                             |  1 -
 net/kcm/kcmsock.c                             |  7 +------
 net/sctp/socket.c                             |  1 -
 net/socket.c                                  | 25 ++++++++-----------------
 5 files changed, 11 insertions(+), 31 deletions(-)

diff --git a/drivers/staging/lustre/lnet/lnet/lib-socket.c b/drivers/staging/lustre/lnet/lnet/lib-socket.c
index 539a26444f31..7d49d4865298 100644
--- a/drivers/staging/lustre/lnet/lnet/lib-socket.c
+++ b/drivers/staging/lustre/lnet/lnet/lib-socket.c
@@ -71,16 +71,12 @@ lnet_sock_ioctl(int cmd, unsigned long arg)
 	}
 
 	sock_filp = sock_alloc_file(sock, 0, NULL);
-	if (IS_ERR(sock_filp)) {
-		sock_release(sock);
-		rc = PTR_ERR(sock_filp);
-		goto out;
-	}
+	if (IS_ERR(sock_filp))
+		return PTR_ERR(sock_filp);
 
 	rc = kernel_sock_unlocked_ioctl(sock_filp, cmd, arg);
 
 	fput(sock_filp);
-out:
 	return rc;
 }
 
diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
index 985046ae4231..80f5c79053a4 100644
--- a/net/9p/trans_fd.c
+++ b/net/9p/trans_fd.c
@@ -839,7 +839,6 @@ static int p9_socket_open(struct p9_client *client, struct socket *csocket)
 	if (IS_ERR(file)) {
 		pr_err("%s (%d): failed to map fd\n",
 		       __func__, task_pid_nr(current));
-		sock_release(csocket);
 		kfree(p);
 		return PTR_ERR(file);
 	}
diff --git a/net/kcm/kcmsock.c b/net/kcm/kcmsock.c
index c5fa634e63ca..d4e98f20fc2a 100644
--- a/net/kcm/kcmsock.c
+++ b/net/kcm/kcmsock.c
@@ -1629,7 +1629,6 @@ static struct file *kcm_clone(struct socket *osock)
 {
 	struct socket *newsock;
 	struct sock *newsk;
-	struct file *file;
 
 	newsock = sock_alloc();
 	if (!newsock)
@@ -1649,11 +1648,7 @@ static struct file *kcm_clone(struct socket *osock)
 	sock_init_data(newsock, newsk);
 	init_kcm_sock(kcm_sk(newsk), kcm_sk(osock->sk)->mux);
 
-	file = sock_alloc_file(newsock, 0, osock->sk->sk_prot_creator->name);
-	if (IS_ERR(file))
-		sock_release(newsock);
-
-	return file;
+	return sock_alloc_file(newsock, 0, osock->sk->sk_prot_creator->name);
 }
 
 static int kcm_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 3204a9b29407..8bb5163d6331 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -5080,7 +5080,6 @@ static int sctp_getsockopt_peeloff_common(struct sock *sk, sctp_peeloff_arg_t *p
 	*newfile = sock_alloc_file(newsock, 0, NULL);
 	if (IS_ERR(*newfile)) {
 		put_unused_fd(retval);
-		sock_release(newsock);
 		retval = PTR_ERR(*newfile);
 		*newfile = NULL;
 		return retval;
diff --git a/net/socket.c b/net/socket.c
index 2df83c0bfde9..05f361faec45 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -406,8 +406,10 @@ struct file *sock_alloc_file(struct socket *sock, int flags, const char *dname)
 		name.len = strlen(name.name);
 	}
 	path.dentry = d_alloc_pseudo(sock_mnt->mnt_sb, &name);
-	if (unlikely(!path.dentry))
+	if (unlikely(!path.dentry)) {
+		sock_release(sock);
 		return ERR_PTR(-ENOMEM);
+	}
 	path.mnt = mntget(sock_mnt);
 
 	d_instantiate(path.dentry, SOCK_INODE(sock));
@@ -415,9 +417,11 @@ struct file *sock_alloc_file(struct socket *sock, int flags, const char *dname)
 	file = alloc_file(&path, FMODE_READ | FMODE_WRITE,
 		  &socket_file_ops);
 	if (IS_ERR(file)) {
-		/* drop dentry, keep inode */
+		/* drop dentry, keep inode for a bit */
 		ihold(d_inode(path.dentry));
 		path_put(&path);
+		/* ... and now kill it properly */
+		sock_release(sock);
 		return file;
 	}
 
@@ -1330,19 +1334,9 @@ SYSCALL_DEFINE3(socket, int, family, int, type, int, protocol)
 
 	retval = sock_create(family, type, protocol, &sock);
 	if (retval < 0)
-		goto out;
-
-	retval = sock_map_fd(sock, flags & (O_CLOEXEC | O_NONBLOCK));
-	if (retval < 0)
-		goto out_release;
-
-out:
-	/* It may be already another descriptor 8) Not kernel problem. */
-	return retval;
+		return retval;
 
-out_release:
-	sock_release(sock);
-	return retval;
+	return sock_map_fd(sock, flags & (O_CLOEXEC | O_NONBLOCK));
 }
 
 /*
@@ -1412,7 +1406,6 @@ SYSCALL_DEFINE4(socketpair, int, family, int, type, int, protocol,
 	newfile1 = sock_alloc_file(sock1, flags, NULL);
 	if (IS_ERR(newfile1)) {
 		err = PTR_ERR(newfile1);
-		sock_release(sock1);
 		sock_release(sock2);
 		goto out;
 	}
@@ -1420,7 +1413,6 @@ SYSCALL_DEFINE4(socketpair, int, family, int, type, int, protocol,
 	newfile2 = sock_alloc_file(sock2, flags, NULL);
 	if (IS_ERR(newfile2)) {
 		err = PTR_ERR(newfile2);
-		sock_release(sock2);
 		fput(newfile1);
 		goto out;
 	}
@@ -1549,7 +1541,6 @@ SYSCALL_DEFINE4(accept4, int, fd, struct sockaddr __user *, upeer_sockaddr,
 	if (IS_ERR(newfile)) {
 		err = PTR_ERR(newfile);
 		put_unused_fd(newfd);
-		sock_release(newsock);
 		goto out_put;
 	}
 
-- 
2.11.0

Re: [PATCH 1/3] socketpair(): allocate descriptors first

[Eric Dumazet]

On Fri, 2017-12-01 at 00:22 +0000, Al Viro wrote:
> simplifies failure exits considerably...
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH 2/3] fix kcm_clone()

[Eric Dumazet]

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:

Re: [PATCH 3/3] make sock_alloc_file() do sock_release() on failures

[Eric Dumazet]

On Fri, 2017-12-01 at 00:23 +0000, Al Viro wrote:
> This changes calling conventions (and simplifies the hell out
> the callers).  New rules: once struct socket had been passed
> to sock_alloc_file(), it's been consumed either by struct file
> or by sock_release() done by sock_alloc_file().  Either way
> the caller should not do sock_release() after that point.
> 
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---

Nice cleanup !

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH 2/3] fix kcm_clone()

[Tom Herbert]

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
>

Re: [RFC][PATCHES] sock_alloc_file() cleanups and fixes

[David Miller]

From: Al Viro <viro@ZenIV.linux.org.uk>
Date: Fri, 1 Dec 2017 00:20:27 +0000

> 	1) massage sys_socketpair() (should be a pure cleanup)
> 	2) fix and clean up kcm_clone() (-stable fodder)
> 	3) switch sock_alloc_file() to new calling conventions.
> 
> 	It got some local testing, but it certainly needs more review.
> Diffstat for the entire thing is

Series looks great to me:

Acked-by: David S. Miller <davem@davemloft.net>

Re: [RFC][PATCHES] sock_alloc_file() cleanups and fixes

[Al Viro]

On Mon, Dec 04, 2017 at 10:35:24AM -0500, David Miller wrote:
> From: Al Viro <viro@ZenIV.linux.org.uk>
> Date: Fri, 1 Dec 2017 00:20:27 +0000
> 
> > 	1) massage sys_socketpair() (should be a pure cleanup)
> > 	2) fix and clean up kcm_clone() (-stable fodder)
> > 	3) switch sock_alloc_file() to new calling conventions.
> > 
> > 	It got some local testing, but it certainly needs more review.
> > Diffstat for the entire thing is
> 
> Series looks great to me:
> 
> Acked-by: David S. Miller <davem@davemloft.net>

How do you prefer it to be handled?  KCM one should go into everything
since 4.6 (with trivial modifications in 4.11 and 4.12 - both had
massaged the place around the call of kcm_clone() a bit, but this fix
overwrites the entire area and that can be dropped into earlier
kernels without any problems).  I've put that into vfs.git#net-fixes
and have the other two in vfs.git#for-davem on top of that, with
you merging the latter into net-next.git and the former - into net.git.
Is that OK with you, or would you prefer some other way of handling
that kind of stuff?

Re: [RFC][PATCHES] sock_alloc_file() cleanups and fixes

[David Miller]

From: Al Viro <viro@ZenIV.linux.org.uk>
Date: Mon, 4 Dec 2017 16:41:01 +0000

> On Mon, Dec 04, 2017 at 10:35:24AM -0500, David Miller wrote:
>> From: Al Viro <viro@ZenIV.linux.org.uk>
>> Date: Fri, 1 Dec 2017 00:20:27 +0000
>> 
>> > 	1) massage sys_socketpair() (should be a pure cleanup)
>> > 	2) fix and clean up kcm_clone() (-stable fodder)
>> > 	3) switch sock_alloc_file() to new calling conventions.
>> > 
>> > 	It got some local testing, but it certainly needs more review.
>> > Diffstat for the entire thing is
>> 
>> Series looks great to me:
>> 
>> Acked-by: David S. Miller <davem@davemloft.net>
> 
> How do you prefer it to be handled?  KCM one should go into everything
> since 4.6 (with trivial modifications in 4.11 and 4.12 - both had
> massaged the place around the call of kcm_clone() a bit, but this fix
> overwrites the entire area and that can be dropped into earlier
> kernels without any problems).  I've put that into vfs.git#net-fixes
> and have the other two in vfs.git#for-davem on top of that, with
> you merging the latter into net-next.git and the former - into net.git.
> Is that OK with you, or would you prefer some other way of handling
> that kind of stuff?

Why don't you resubmit this to netdev as a non-RFC, I'll queue it up to
'net' and -stable as well.

Thanks Al.

Re: [RFC][PATCHES] sock_alloc_file() cleanups and fixes

[Al Viro]

On Tue, Dec 05, 2017 at 02:44:43PM -0500, David Miller wrote:
> From: Al Viro <viro@ZenIV.linux.org.uk>
> Date: Mon, 4 Dec 2017 16:41:01 +0000
> 
> > On Mon, Dec 04, 2017 at 10:35:24AM -0500, David Miller wrote:
> >> From: Al Viro <viro@ZenIV.linux.org.uk>
> >> Date: Fri, 1 Dec 2017 00:20:27 +0000
> >> 
> >> > 	1) massage sys_socketpair() (should be a pure cleanup)
> >> > 	2) fix and clean up kcm_clone() (-stable fodder)
> >> > 	3) switch sock_alloc_file() to new calling conventions.
> >> > 
> >> > 	It got some local testing, but it certainly needs more review.
> >> > Diffstat for the entire thing is
> >> 
> >> Series looks great to me:
> >> 
> >> Acked-by: David S. Miller <davem@davemloft.net>
> > 
> > How do you prefer it to be handled?  KCM one should go into everything
> > since 4.6 (with trivial modifications in 4.11 and 4.12 - both had
> > massaged the place around the call of kcm_clone() a bit, but this fix
> > overwrites the entire area and that can be dropped into earlier
> > kernels without any problems).  I've put that into vfs.git#net-fixes
> > and have the other two in vfs.git#for-davem on top of that, with
> > you merging the latter into net-next.git and the former - into net.git.
> > Is that OK with you, or would you prefer some other way of handling
> > that kind of stuff?
> 
> Why don't you resubmit this to netdev as a non-RFC, I'll queue it up to
> 'net' and -stable as well.

Sent...

Re: [PATCH 3/3] make sock_alloc_file() do sock_release() on failures

[David Miller]

From: Al Viro <viro@ZenIV.linux.org.uk>
Date: Tue, 5 Dec 2017 23:29:09 +0000

> This changes calling conventions (and simplifies the hell out
> the callers).  New rules: once struct socket had been passed
> to sock_alloc_file(), it's been consumed either by struct file
> or by sock_release() done by sock_alloc_file().  Either way
> the caller should not do sock_release() after that point.
> 
> Reviewed-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

Applied.

[PATCH 3/3] make sock_alloc_file() do sock_release() on failures

[Al Viro]

This changes calling conventions (and simplifies the hell out
the callers).  New rules: once struct socket had been passed
to sock_alloc_file(), it's been consumed either by struct file
or by sock_release() done by sock_alloc_file().  Either way
the caller should not do sock_release() after that point.

Reviewed-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
 drivers/staging/lustre/lnet/lnet/lib-socket.c |  8 ++------
 net/9p/trans_fd.c                             |  1 -
 net/kcm/kcmsock.c                             |  7 +------
 net/sctp/socket.c                             |  1 -
 net/socket.c                                  | 25 ++++++++-----------------
 5 files changed, 11 insertions(+), 31 deletions(-)

diff --git a/drivers/staging/lustre/lnet/lnet/lib-socket.c b/drivers/staging/lustre/lnet/lnet/lib-socket.c
index 539a26444f31..7d49d4865298 100644
--- a/drivers/staging/lustre/lnet/lnet/lib-socket.c
+++ b/drivers/staging/lustre/lnet/lnet/lib-socket.c
@@ -71,16 +71,12 @@ lnet_sock_ioctl(int cmd, unsigned long arg)
 	}
 
 	sock_filp = sock_alloc_file(sock, 0, NULL);
-	if (IS_ERR(sock_filp)) {
-		sock_release(sock);
-		rc = PTR_ERR(sock_filp);
-		goto out;
-	}
+	if (IS_ERR(sock_filp))
+		return PTR_ERR(sock_filp);
 
 	rc = kernel_sock_unlocked_ioctl(sock_filp, cmd, arg);
 
 	fput(sock_filp);
-out:
 	return rc;
 }
 
diff --git a/net/9p/trans_fd.c b/net/9p/trans_fd.c
index 985046ae4231..80f5c79053a4 100644
--- a/net/9p/trans_fd.c
+++ b/net/9p/trans_fd.c
@@ -839,7 +839,6 @@ static int p9_socket_open(struct p9_client *client, struct socket *csocket)
 	if (IS_ERR(file)) {
 		pr_err("%s (%d): failed to map fd\n",
 		       __func__, task_pid_nr(current));
-		sock_release(csocket);
 		kfree(p);
 		return PTR_ERR(file);
 	}
diff --git a/net/kcm/kcmsock.c b/net/kcm/kcmsock.c
index c5fa634e63ca..d4e98f20fc2a 100644
--- a/net/kcm/kcmsock.c
+++ b/net/kcm/kcmsock.c
@@ -1629,7 +1629,6 @@ static struct file *kcm_clone(struct socket *osock)
 {
 	struct socket *newsock;
 	struct sock *newsk;
-	struct file *file;
 
 	newsock = sock_alloc();
 	if (!newsock)
@@ -1649,11 +1648,7 @@ static struct file *kcm_clone(struct socket *osock)
 	sock_init_data(newsock, newsk);
 	init_kcm_sock(kcm_sk(newsk), kcm_sk(osock->sk)->mux);
 
-	file = sock_alloc_file(newsock, 0, osock->sk->sk_prot_creator->name);
-	if (IS_ERR(file))
-		sock_release(newsock);
-
-	return file;
+	return sock_alloc_file(newsock, 0, osock->sk->sk_prot_creator->name);
 }
 
 static int kcm_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 3204a9b29407..8bb5163d6331 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -5080,7 +5080,6 @@ static int sctp_getsockopt_peeloff_common(struct sock *sk, sctp_peeloff_arg_t *p
 	*newfile = sock_alloc_file(newsock, 0, NULL);
 	if (IS_ERR(*newfile)) {
 		put_unused_fd(retval);
-		sock_release(newsock);
 		retval = PTR_ERR(*newfile);
 		*newfile = NULL;
 		return retval;
diff --git a/net/socket.c b/net/socket.c
index 2df83c0bfde9..05f361faec45 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -406,8 +406,10 @@ struct file *sock_alloc_file(struct socket *sock, int flags, const char *dname)
 		name.len = strlen(name.name);
 	}
 	path.dentry = d_alloc_pseudo(sock_mnt->mnt_sb, &name);
-	if (unlikely(!path.dentry))
+	if (unlikely(!path.dentry)) {
+		sock_release(sock);
 		return ERR_PTR(-ENOMEM);
+	}
 	path.mnt = mntget(sock_mnt);
 
 	d_instantiate(path.dentry, SOCK_INODE(sock));
@@ -415,9 +417,11 @@ struct file *sock_alloc_file(struct socket *sock, int flags, const char *dname)
 	file = alloc_file(&path, FMODE_READ | FMODE_WRITE,
 		  &socket_file_ops);
 	if (IS_ERR(file)) {
-		/* drop dentry, keep inode */
+		/* drop dentry, keep inode for a bit */
 		ihold(d_inode(path.dentry));
 		path_put(&path);
+		/* ... and now kill it properly */
+		sock_release(sock);
 		return file;
 	}
 
@@ -1330,19 +1334,9 @@ SYSCALL_DEFINE3(socket, int, family, int, type, int, protocol)
 
 	retval = sock_create(family, type, protocol, &sock);
 	if (retval < 0)
-		goto out;
-
-	retval = sock_map_fd(sock, flags & (O_CLOEXEC | O_NONBLOCK));
-	if (retval < 0)
-		goto out_release;
-
-out:
-	/* It may be already another descriptor 8) Not kernel problem. */
-	return retval;
+		return retval;
 
-out_release:
-	sock_release(sock);
-	return retval;
+	return sock_map_fd(sock, flags & (O_CLOEXEC | O_NONBLOCK));
 }
 
 /*
@@ -1412,7 +1406,6 @@ SYSCALL_DEFINE4(socketpair, int, family, int, type, int, protocol,
 	newfile1 = sock_alloc_file(sock1, flags, NULL);
 	if (IS_ERR(newfile1)) {
 		err = PTR_ERR(newfile1);
-		sock_release(sock1);
 		sock_release(sock2);
 		goto out;
 	}
@@ -1420,7 +1413,6 @@ SYSCALL_DEFINE4(socketpair, int, family, int, type, int, protocol,
 	newfile2 = sock_alloc_file(sock2, flags, NULL);
 	if (IS_ERR(newfile2)) {
 		err = PTR_ERR(newfile2);
-		sock_release(sock2);
 		fput(newfile1);
 		goto out;
 	}
@@ -1549,7 +1541,6 @@ SYSCALL_DEFINE4(accept4, int, fd, struct sockaddr __user *, upeer_sockaddr,
 	if (IS_ERR(newfile)) {
 		err = PTR_ERR(newfile);
 		put_unused_fd(newfd);
-		sock_release(newsock);
 		goto out_put;
 	}
 
-- 
2.11.0