[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 
Signed-off-by: Al Viro 
---
 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, );
-   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(, FMODE_READ | FMODE_WRITE,
  _file_ops);
if (IS_ERR(file)) {
-   /* drop dentry, keep inode */
+   /* drop dentry, keep inode for a bit */
ihold(d_inode(path.dentry));
path_put();
+   /* ... 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, );
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;
+   

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

[Eric Dumazet]

On Fri, 2017-12-01 at 00:23 +, 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 
> ---

Nice cleanup !

Reviewed-by: Eric Dumazet 

[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 
---
 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, );
-   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(, FMODE_READ | FMODE_WRITE,
  _file_ops);
if (IS_ERR(file)) {
-   /* drop dentry, keep inode */
+   /* drop dentry, keep inode for a bit */
ihold(d_inode(path.dentry));
path_put();
+   /* ... 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, );
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 |