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

[Eric Dumazet <eric.dumazet@gmail.com>]

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: