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

[Jakub Kicinski]

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