netdev - Re: [PATCH net-next] net: socket: integrate sockfd_lookup() and sockfd_lookup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 23 Sep 2021 08:24:53 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Yajun Deng <yajun.deng@...ux.dev>
Cc:     davem@...emloft.net, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] net: socket: integrate sockfd_lookup() and
 sockfd_lookup_light()

On Wed, 22 Sep 2021 14:31:06 +0800 Yajun Deng wrote:
> As commit 6cb153cab92a("[NET]: use fget_light() in net/socket.c") said,
> sockfd_lookup_light() is lower load than sockfd_lookup(). So we can
> remove sockfd_lookup() but keep the name. As the same time, move flags
> to sockfd_put().

You just assume that each caller of sockfd_lookup() already meets the
criteria under which sockfd_lookup_light() can be used? Am I reading
this right?

Please extend the commit message clearly walking us thru why this is
safe now (and perhaps why it wasn't in the past).

>  static ssize_t sockfs_listxattr(struct dentry *dentry, char *buffer,
>  				size_t size)
> @@ -1680,9 +1659,9 @@ int __sys_bind(int fd, struct sockaddr __user *umyaddr, int addrlen)
>  {
>  	struct socket *sock;
>  	struct sockaddr_storage address;
> -	int err, fput_needed;
> +	int err;
>  
> -	sock = sockfd_lookup_light(fd, &err, &fput_needed);
> +	sock = sockfd_lookup(fd, &err);
>  	if (sock) {
>  		err = move_addr_to_kernel(umyaddr, addrlen, &address);
>  		if (!err) {
> @@ -1694,7 +1673,7 @@ int __sys_bind(int fd, struct sockaddr __user *umyaddr, int addrlen)
>  						      (struct sockaddr *)
>  						      &address, addrlen);
>  		}
> -		fput_light(sock->file, fput_needed);
> +		sockfd_put(sock);

And we just replace fput_light() with fput() even tho the reference was
taken with fdget()? fdget() == __fget_light().

Maybe you missed fget() vs fdget()?

All these changes do not immediately strike me as correct.

>  	}
>  	return err;
>  }