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

From: Jakub Kicinski
Date: Thu Sep 23 2021 - 11:25:05 EST


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