sock_alloc_file() cleanups and fixes

simplifies failure exits considerably...

Signed-off-by Al Viro <[email protected]>

---

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 <[email protected]>

---
 net/kcm/kcmsock.c | 71 +++++++++++++++++++++----------------------
------------

 1 file changed, 27 insertions(+), 44 deletions(-)

net/kcm/kcmsock.c +0 -0

index 0b750a22c4b9..c5fa634e63ca 100644
@@ -1625,60 +1625,35 @@ static struct proto kcm_proto = {
  };

 /* Clone a kcm socket. */

net/kcm/kcmsock.c +1 -2

-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;

net/kcm/kcmsock.c +1 -2

-	struct file *newfile;
-	int err, newfd;
+	struct file *file;

net/kcm/kcmsock.c +0 -1

-	err = -ENFILE;

  newsock = sock_alloc();
  if (!newsock)

net/kcm/kcmsock.c +1 -1

-		goto out;
+		return ERR_PTR(-ENFILE);

  newsock->type = osock->type;
  newsock->ops = osock->ops;

  __module_get(newsock->ops->owner);

net/kcm/kcmsock.c +0 -7

-	newfd = get_unused_fd_flags(0);
-	if (unlikely(newfd < 0)) {
-		err = newfd;
-		goto out_fd_fail;
-	}
-
-	newfile = sock_alloc_file(newsock, 0, osock->sk-

Quoted 1 line

sk_prot_creator->name);

net/kcm/kcmsock.c +0 -5

-	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) {

net/kcm/kcmsock.c +2 -2

-		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);

net/kcm/kcmsock.c +1 -5

-	fd_install(newfd, newfile);
-	*newsockp = newsock;
-	info->fd = newfd;
-
-	return 0;
+	file = sock_alloc_file(newsock, 0, osock->sk-

Quoted 1 line

sk_prot_creator->name);

net/kcm/kcmsock.c +2 -0

+	if (IS_ERR(file))
+		sock_release(newsock);

net/kcm/kcmsock.c +1 -8

-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)

net/kcm/kcmsock.c +5 -10

@@ -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;

net/kcm/kcmsock.c +13 -0

+		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 <[email protected]>

---
 net/kcm/kcmsock.c | 71 +++++++++++++++++++++----------------------------------
 1 file changed, 27 insertions(+), 44 deletions(-)

net/kcm/kcmsock.c +27 -45

index 0b750a22c4b9..c5fa634e63ca 100644
@@ -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

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 <[email protected]>

---

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

From: Al Viro <[email protected]>
Date: Fri, 1 Dec 2017 00:20:27 +0000

Quoted 4 lines

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.

Quoted 2 lines

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 <[email protected]>

Quoted 7 lines

From: Al Viro <[email protected]>
Date: Fri, 1 Dec 2017 00:20:27 +0000

Quoted 4 lines

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.

Quoted 6 lines

Quoted 2 lines

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 <[email protected]>

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?

From: Al Viro <[email protected]>
Date: Mon, 4 Dec 2017 16:41:01 +0000

Quoted 15 lines · David Miller

Quoted 7 lines

From: Al Viro <[email protected]>
Date: Fri, 1 Dec 2017 00:20:27 +0000

Quoted 4 lines

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.

Quoted 6 lines

Quoted 2 lines

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 <[email protected]>

Quoted 9 lines

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.