All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next] net: socket: integrate sockfd_lookup() and sockfd_lookup_light()
@ 2021-09-22  6:31 Yajun Deng
  2021-09-23 15:24 ` Jakub Kicinski
                   ` (5 more replies)
  0 siblings, 6 replies; 7+ messages in thread
From: Yajun Deng @ 2021-09-22  6:31 UTC (permalink / raw)
  To: davem, kuba; +Cc: netdev, linux-kernel, Yajun Deng

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

Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
---
 include/linux/net.h |   8 +++-
 net/socket.c        | 101 +++++++++++++++++---------------------------
 2 files changed, 46 insertions(+), 63 deletions(-)

diff --git a/include/linux/net.h b/include/linux/net.h
index ba736b457a06..63a179d4f760 100644
--- a/include/linux/net.h
+++ b/include/linux/net.h
@@ -238,8 +238,14 @@ int sock_recvmsg(struct socket *sock, struct msghdr *msg, int flags);
 struct file *sock_alloc_file(struct socket *sock, int flags, const char *dname);
 struct socket *sockfd_lookup(int fd, int *err);
 struct socket *sock_from_file(struct file *file);
-#define		     sockfd_put(sock) fput(sock->file)
 int net_ratelimit(void);
+#define		     sockfd_put(sock)             \
+do {                                              \
+	struct fd *fd = (struct fd *)&sock->file; \
+						  \
+	if (fd->flags & FDPUT_FPUT)               \
+		fput(sock->file);                 \
+} while (0)
 
 #define net_ratelimited_function(function, ...)			\
 do {								\
diff --git a/net/socket.c b/net/socket.c
index 7f64a6eccf63..ca8a05aee982 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -521,28 +521,7 @@ EXPORT_SYMBOL(sock_from_file);
  *
  *	On a success the socket object pointer is returned.
  */
-
 struct socket *sockfd_lookup(int fd, int *err)
-{
-	struct file *file;
-	struct socket *sock;
-
-	file = fget(fd);
-	if (!file) {
-		*err = -EBADF;
-		return NULL;
-	}
-
-	sock = sock_from_file(file);
-	if (!sock) {
-		*err = -ENOTSOCK;
-		fput(file);
-	}
-	return sock;
-}
-EXPORT_SYMBOL(sockfd_lookup);
-
-static struct socket *sockfd_lookup_light(int fd, int *err, int *fput_needed)
 {
 	struct fd f = fdget(fd);
 	struct socket *sock;
@@ -551,7 +530,6 @@ static struct socket *sockfd_lookup_light(int fd, int *err, int *fput_needed)
 	if (f.file) {
 		sock = sock_from_file(f.file);
 		if (likely(sock)) {
-			*fput_needed = f.flags & FDPUT_FPUT;
 			return sock;
 		}
 		*err = -ENOTSOCK;
@@ -559,6 +537,7 @@ static struct socket *sockfd_lookup_light(int fd, int *err, int *fput_needed)
 	}
 	return NULL;
 }
+EXPORT_SYMBOL(sockfd_lookup);
 
 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);
 	}
 	return err;
 }
@@ -1713,10 +1692,10 @@ SYSCALL_DEFINE3(bind, int, fd, struct sockaddr __user *, umyaddr, int, addrlen)
 int __sys_listen(int fd, int backlog)
 {
 	struct socket *sock;
-	int err, fput_needed;
+	int err;
 	int somaxconn;
 
-	sock = sockfd_lookup_light(fd, &err, &fput_needed);
+	sock = sockfd_lookup(fd, &err);
 	if (sock) {
 		somaxconn = sock_net(sock->sk)->core.sysctl_somaxconn;
 		if ((unsigned int)backlog > somaxconn)
@@ -1726,7 +1705,7 @@ int __sys_listen(int fd, int backlog)
 		if (!err)
 			err = sock->ops->listen(sock, backlog);
 
-		fput_light(sock->file, fput_needed);
+		sockfd_put(sock);
 	}
 	return err;
 }
@@ -1933,9 +1912,9 @@ int __sys_getsockname(int fd, struct sockaddr __user *usockaddr,
 {
 	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)
 		goto out;
 
@@ -1950,7 +1929,7 @@ int __sys_getsockname(int fd, struct sockaddr __user *usockaddr,
 	err = move_addr_to_user(&address, err, usockaddr, usockaddr_len);
 
 out_put:
-	fput_light(sock->file, fput_needed);
+	sockfd_put(sock);
 out:
 	return err;
 }
@@ -1971,13 +1950,13 @@ int __sys_getpeername(int fd, struct sockaddr __user *usockaddr,
 {
 	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 != NULL) {
 		err = security_socket_getpeername(sock);
 		if (err) {
-			fput_light(sock->file, fput_needed);
+			sockfd_put(sock);
 			return err;
 		}
 
@@ -1986,7 +1965,7 @@ int __sys_getpeername(int fd, struct sockaddr __user *usockaddr,
 			/* "err" is actually length in this case */
 			err = move_addr_to_user(&address, err, usockaddr,
 						usockaddr_len);
-		fput_light(sock->file, fput_needed);
+		sockfd_put(sock);
 	}
 	return err;
 }
@@ -2010,12 +1989,11 @@ int __sys_sendto(int fd, void __user *buff, size_t len, unsigned int flags,
 	int err;
 	struct msghdr msg;
 	struct iovec iov;
-	int fput_needed;
 
 	err = import_single_range(WRITE, buff, len, &iov, &msg.msg_iter);
 	if (unlikely(err))
 		return err;
-	sock = sockfd_lookup_light(fd, &err, &fput_needed);
+	sock = sockfd_lookup(fd, &err);
 	if (!sock)
 		goto out;
 
@@ -2036,7 +2014,7 @@ int __sys_sendto(int fd, void __user *buff, size_t len, unsigned int flags,
 	err = sock_sendmsg(sock, &msg);
 
 out_put:
-	fput_light(sock->file, fput_needed);
+	sockfd_put(sock);
 out:
 	return err;
 }
@@ -2071,12 +2049,11 @@ int __sys_recvfrom(int fd, void __user *ubuf, size_t size, unsigned int flags,
 	struct msghdr msg;
 	struct sockaddr_storage address;
 	int err, err2;
-	int fput_needed;
 
 	err = import_single_range(READ, ubuf, size, &iov, &msg.msg_iter);
 	if (unlikely(err))
 		return err;
-	sock = sockfd_lookup_light(fd, &err, &fput_needed);
+	sock = sockfd_lookup(fd, &err);
 	if (!sock)
 		goto out;
 
@@ -2099,7 +2076,7 @@ int __sys_recvfrom(int fd, void __user *ubuf, size_t size, unsigned int flags,
 			err = err2;
 	}
 
-	fput_light(sock->file, fput_needed);
+	sockfd_put(sock);
 out:
 	return err;
 }
@@ -2141,13 +2118,13 @@ int __sys_setsockopt(int fd, int level, int optname, char __user *user_optval,
 {
 	sockptr_t optval = USER_SOCKPTR(user_optval);
 	char *kernel_optval = NULL;
-	int err, fput_needed;
+	int err;
 	struct socket *sock;
 
 	if (optlen < 0)
 		return -EINVAL;
 
-	sock = sockfd_lookup_light(fd, &err, &fput_needed);
+	sock = sockfd_lookup(fd, &err);
 	if (!sock)
 		return err;
 
@@ -2177,7 +2154,7 @@ int __sys_setsockopt(int fd, int level, int optname, char __user *user_optval,
 					    optlen);
 	kfree(kernel_optval);
 out_put:
-	fput_light(sock->file, fput_needed);
+	sockfd_put(sock);
 	return err;
 }
 
@@ -2197,11 +2174,11 @@ INDIRECT_CALLABLE_DECLARE(bool tcp_bpf_bypass_getsockopt(int level,
 int __sys_getsockopt(int fd, int level, int optname, char __user *optval,
 		int __user *optlen)
 {
-	int err, fput_needed;
+	int err;
 	struct socket *sock;
 	int max_optlen;
 
-	sock = sockfd_lookup_light(fd, &err, &fput_needed);
+	sock = sockfd_lookup(fd, &err);
 	if (!sock)
 		return err;
 
@@ -2225,7 +2202,7 @@ int __sys_getsockopt(int fd, int level, int optname, char __user *optval,
 						     optval, optlen, max_optlen,
 						     err);
 out_put:
-	fput_light(sock->file, fput_needed);
+	sockfd_put(sock);
 	return err;
 }
 
@@ -2252,13 +2229,13 @@ int __sys_shutdown_sock(struct socket *sock, int how)
 
 int __sys_shutdown(int fd, int how)
 {
-	int err, fput_needed;
+	int err;
 	struct socket *sock;
 
-	sock = sockfd_lookup_light(fd, &err, &fput_needed);
+	sock = sockfd_lookup(fd, &err);
 	if (sock != NULL) {
 		err = __sys_shutdown_sock(sock, how);
-		fput_light(sock->file, fput_needed);
+		sockfd_put(sock);
 	}
 	return err;
 }
@@ -2478,20 +2455,20 @@ long __sys_sendmsg_sock(struct socket *sock, struct msghdr *msg,
 long __sys_sendmsg(int fd, struct user_msghdr __user *msg, unsigned int flags,
 		   bool forbid_cmsg_compat)
 {
-	int fput_needed, err;
+	int err;
 	struct msghdr msg_sys;
 	struct socket *sock;
 
 	if (forbid_cmsg_compat && (flags & MSG_CMSG_COMPAT))
 		return -EINVAL;
 
-	sock = sockfd_lookup_light(fd, &err, &fput_needed);
+	sock = sockfd_lookup(fd, &err);
 	if (!sock)
 		goto out;
 
 	err = ___sys_sendmsg(sock, msg, &msg_sys, flags, NULL, 0);
 
-	fput_light(sock->file, fput_needed);
+	sockfd_put(sock);
 out:
 	return err;
 }
@@ -2508,7 +2485,7 @@ SYSCALL_DEFINE3(sendmsg, int, fd, struct user_msghdr __user *, msg, unsigned int
 int __sys_sendmmsg(int fd, struct mmsghdr __user *mmsg, unsigned int vlen,
 		   unsigned int flags, bool forbid_cmsg_compat)
 {
-	int fput_needed, err, datagrams;
+	int err, datagrams;
 	struct socket *sock;
 	struct mmsghdr __user *entry;
 	struct compat_mmsghdr __user *compat_entry;
@@ -2524,7 +2501,7 @@ int __sys_sendmmsg(int fd, struct mmsghdr __user *mmsg, unsigned int vlen,
 
 	datagrams = 0;
 
-	sock = sockfd_lookup_light(fd, &err, &fput_needed);
+	sock = sockfd_lookup(fd, &err);
 	if (!sock)
 		return err;
 
@@ -2563,7 +2540,7 @@ int __sys_sendmmsg(int fd, struct mmsghdr __user *mmsg, unsigned int vlen,
 		cond_resched();
 	}
 
-	fput_light(sock->file, fput_needed);
+	sockfd_put(sock);
 
 	/* We only return an error if no datagrams were able to be sent */
 	if (datagrams != 0)
@@ -2686,20 +2663,20 @@ long __sys_recvmsg_sock(struct socket *sock, struct msghdr *msg,
 long __sys_recvmsg(int fd, struct user_msghdr __user *msg, unsigned int flags,
 		   bool forbid_cmsg_compat)
 {
-	int fput_needed, err;
+	int err;
 	struct msghdr msg_sys;
 	struct socket *sock;
 
 	if (forbid_cmsg_compat && (flags & MSG_CMSG_COMPAT))
 		return -EINVAL;
 
-	sock = sockfd_lookup_light(fd, &err, &fput_needed);
+	sock = sockfd_lookup(fd, &err);
 	if (!sock)
 		goto out;
 
 	err = ___sys_recvmsg(sock, msg, &msg_sys, flags, 0);
 
-	fput_light(sock->file, fput_needed);
+	sockfd_put(sock);
 out:
 	return err;
 }
@@ -2718,7 +2695,7 @@ static int do_recvmmsg(int fd, struct mmsghdr __user *mmsg,
 			  unsigned int vlen, unsigned int flags,
 			  struct timespec64 *timeout)
 {
-	int fput_needed, err, datagrams;
+	int err, datagrams;
 	struct socket *sock;
 	struct mmsghdr __user *entry;
 	struct compat_mmsghdr __user *compat_entry;
@@ -2733,7 +2710,7 @@ static int do_recvmmsg(int fd, struct mmsghdr __user *mmsg,
 
 	datagrams = 0;
 
-	sock = sockfd_lookup_light(fd, &err, &fput_needed);
+	sock = sockfd_lookup(fd, &err);
 	if (!sock)
 		return err;
 
@@ -2820,7 +2797,7 @@ static int do_recvmmsg(int fd, struct mmsghdr __user *mmsg,
 		sock->sk->sk_err = -err;
 	}
 out_put:
-	fput_light(sock->file, fput_needed);
+	sockfd_put(sock);
 
 	return datagrams;
 }
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next] net: socket: integrate sockfd_lookup() and sockfd_lookup_light()
  2021-09-22  6:31 [PATCH net-next] net: socket: integrate sockfd_lookup() and sockfd_lookup_light() Yajun Deng
@ 2021-09-23 15:24 ` Jakub Kicinski
  2021-09-23 16:49 ` Eric Dumazet
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Jakub Kicinski @ 2021-09-23 15:24 UTC (permalink / raw)
  To: Yajun Deng; +Cc: davem, netdev, linux-kernel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next] net: socket: integrate sockfd_lookup() and sockfd_lookup_light()
  2021-09-22  6:31 [PATCH net-next] net: socket: integrate sockfd_lookup() and sockfd_lookup_light() Yajun Deng
  2021-09-23 15:24 ` Jakub Kicinski
@ 2021-09-23 16:49 ` Eric Dumazet
  2021-09-24  2:39 ` yajun.deng
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: Eric Dumazet @ 2021-09-23 16:49 UTC (permalink / raw)
  To: Yajun Deng, davem, kuba; +Cc: netdev, linux-kernel



On 9/21/21 11:31 PM, 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().

???

> 
> Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
> ---
>  include/linux/net.h |   8 +++-
>  net/socket.c        | 101 +++++++++++++++++---------------------------
>  2 files changed, 46 insertions(+), 63 deletions(-)
> 
> diff --git a/include/linux/net.h b/include/linux/net.h
> index ba736b457a06..63a179d4f760 100644
> --- a/include/linux/net.h
> +++ b/include/linux/net.h
> @@ -238,8 +238,14 @@ int sock_recvmsg(struct socket *sock, struct msghdr *msg, int flags);
>  struct file *sock_alloc_file(struct socket *sock, int flags, const char *dname);
>  struct socket *sockfd_lookup(int fd, int *err);
>  struct socket *sock_from_file(struct file *file);
> -#define		     sockfd_put(sock) fput(sock->file)
>  int net_ratelimit(void);
> +#define		     sockfd_put(sock)             \
> +do {                                              \
> +	struct fd *fd = (struct fd *)&sock->file; \
> +						  \
> +	if (fd->flags & FDPUT_FPUT)               \
> +		fput(sock->file);                 \
> +} while (0)
>  

Really ?

I wonder how was this tested ?

We can not store FDPUT_FPUT in the sock itself, for obvious reasons.
Each thread needs to keep this information private.


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next] net: socket: integrate sockfd_lookup() and sockfd_lookup_light()
  2021-09-22  6:31 [PATCH net-next] net: socket: integrate sockfd_lookup() and sockfd_lookup_light() Yajun Deng
  2021-09-23 15:24 ` Jakub Kicinski
  2021-09-23 16:49 ` Eric Dumazet
@ 2021-09-24  2:39 ` yajun.deng
  2021-09-24  2:49 ` yajun.deng
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 7+ messages in thread
From: yajun.deng @ 2021-09-24  2:39 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: davem, netdev, linux-kernel

September 23, 2021 11:24 PM, "Jakub Kicinski" <kuba@kernel.org> wrote:

> 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?
> 
Yes, this patch means each caller of sockfd_lookup() can used sockfd_lookup_light() instead.
> Please extend the commit message clearly walking us thru why this is
> safe now (and perhaps why it wasn't in the past).
>
The sockfd_lookup() and  sockfd_lookup_light() are both safe. The fact that they have been around for so long is the best proof. sockfd_lookup_light() is just lower load than sockfd_lookup(). so we can used the lower load helper function.

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

In fact, the sockfd_put() already changed in this patch. Here is the modified:
#define                     sockfd_put(sock)             \
do {                                              \
       struct fd *fd = (struct fd *)&sock->file; \
                                                 \
       if (fd->flags & FDPUT_FPUT)               \
               fput(sock->file);                 \
}

> 
> All these changes do not immediately strike me as correct.
> 
This is the information of this patch:
 include/linux/net.h |   8 +++-
 net/socket.c        | 101 +++++++++++++++++---------------------------
 2 files changed, 46 insertions(+), 63 deletions(-)

>> }
>> return err;
>> }

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next] net: socket: integrate sockfd_lookup() and sockfd_lookup_light()
  2021-09-22  6:31 [PATCH net-next] net: socket: integrate sockfd_lookup() and sockfd_lookup_light() Yajun Deng
                   ` (2 preceding siblings ...)
  2021-09-24  2:39 ` yajun.deng
@ 2021-09-24  2:49 ` yajun.deng
  2021-09-24  2:56 ` Al Viro
  2021-09-24  3:39 ` yajun.deng
  5 siblings, 0 replies; 7+ messages in thread
From: yajun.deng @ 2021-09-24  2:49 UTC (permalink / raw)
  To: Eric Dumazet, davem, kuba; +Cc: netdev, linux-kernel

September 24, 2021 12:49 AM, "Eric Dumazet" <eric.dumazet@gmail.com> wrote:

> On 9/21/21 11:31 PM, 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().
> 
> ???
> 
>> Signed-off-by: Yajun Deng <yajun.deng@linux.dev>
>> ---
>> include/linux/net.h | 8 +++-
>> net/socket.c | 101 +++++++++++++++++---------------------------
>> 2 files changed, 46 insertions(+), 63 deletions(-)
>> 
>> diff --git a/include/linux/net.h b/include/linux/net.h
>> index ba736b457a06..63a179d4f760 100644
>> --- a/include/linux/net.h
>> +++ b/include/linux/net.h
>> @@ -238,8 +238,14 @@ int sock_recvmsg(struct socket *sock, struct msghdr *msg, int flags);
>> struct file *sock_alloc_file(struct socket *sock, int flags, const char *dname);
>> struct socket *sockfd_lookup(int fd, int *err);
>> struct socket *sock_from_file(struct file *file);
>> -#define sockfd_put(sock) fput(sock->file)
>> int net_ratelimit(void);
>> +#define sockfd_put(sock) \
>> +do { \
>> + struct fd *fd = (struct fd *)&sock->file; \
>> + \
>> + if (fd->flags & FDPUT_FPUT) \
>> + fput(sock->file); \
>> +} while (0)
> 
> Really ?
> 
> I wonder how was this tested ?
> 
> We can not store FDPUT_FPUT in the sock itself, for obvious reasons.
> Each thread needs to keep this information private.

The sockfd_lookup() already changed in this patch. FDPUT_FPUT stored in fdget(), precisely in __fget_light().

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next] net: socket: integrate sockfd_lookup() and sockfd_lookup_light()
  2021-09-22  6:31 [PATCH net-next] net: socket: integrate sockfd_lookup() and sockfd_lookup_light() Yajun Deng
                   ` (3 preceding siblings ...)
  2021-09-24  2:49 ` yajun.deng
@ 2021-09-24  2:56 ` Al Viro
  2021-09-24  3:39 ` yajun.deng
  5 siblings, 0 replies; 7+ messages in thread
From: Al Viro @ 2021-09-24  2:56 UTC (permalink / raw)
  To: Yajun Deng; +Cc: davem, kuba, netdev, linux-kernel

On Wed, Sep 22, 2021 at 02:31:06PM +0800, Yajun Deng wrote:

> -#define		     sockfd_put(sock) fput(sock->file)
>  int net_ratelimit(void);
> +#define		     sockfd_put(sock)             \
> +do {                                              \
> +	struct fd *fd = (struct fd *)&sock->file; \

Have you even bothered to take a look at struct fd declaration?
Or struct socket one, for that matter...  What we have there is
	...
        struct file             *file;
	struct sock             *sk;
	...

You are taking the address of 'file' field.  And treat it as
a pointer to a structure consisting of struct file * and
unsigned int.

> +						  \
> +	if (fd->flags & FDPUT_FPUT)               \

... so that would take first 4 bytes in the memory occupied
by 'sk' field of struct socket and check if bit 0 is set.

And what significance would that bit have, pray tell?  On
little-endian architectures it's going to be the least
significant bit in the first byte in 'sk' field.  I.e.
there you are testing if the contents of 'sk' (a pointer
to struct sock) happens to be odd.  It won't be.  The
same goes for 32bit big-endian - there you will be checking
the least significant bit of the 4th byte of 'sk', which
again is asking 'is the pointer stored there odd for some
reason?'

On 64bit big-endian you are checking if the bit 32 of
the address of object sock->sk points to is set.  And the
answer to that is "hell knows and how could that possibly
be relevant to anything?"

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next] net: socket: integrate sockfd_lookup() and sockfd_lookup_light()
  2021-09-22  6:31 [PATCH net-next] net: socket: integrate sockfd_lookup() and sockfd_lookup_light() Yajun Deng
                   ` (4 preceding siblings ...)
  2021-09-24  2:56 ` Al Viro
@ 2021-09-24  3:39 ` yajun.deng
  5 siblings, 0 replies; 7+ messages in thread
From: yajun.deng @ 2021-09-24  3:39 UTC (permalink / raw)
  To: Al Viro, kuba, Eric Dumazet; +Cc: davem, netdev, linux-kernel

September 24, 2021 10:56 AM, "Al Viro" <viro@zeniv.linux.org.uk> wrote:

> On Wed, Sep 22, 2021 at 02:31:06PM +0800, Yajun Deng wrote:
> 
>> -#define sockfd_put(sock) fput(sock->file)
>> int net_ratelimit(void);
>> +#define sockfd_put(sock) \
>> +do { \
>> + struct fd *fd = (struct fd *)&sock->file; \
> 
> Have you even bothered to take a look at struct fd declaration?
> Or struct socket one, for that matter... What we have there is
> ...
> struct file *file;
> struct sock *sk;
> ...
> 
> You are taking the address of 'file' field. And treat it as
> a pointer to a structure consisting of struct file * and
> unsigned int.
> 
>> + \
>> + if (fd->flags & FDPUT_FPUT) \
> 
> ... so that would take first 4 bytes in the memory occupied
> by 'sk' field of struct socket and check if bit 0 is set.
> 
> And what significance would that bit have, pray tell? On
> little-endian architectures it's going to be the least
> significant bit in the first byte in 'sk' field. I.e.
> there you are testing if the contents of 'sk' (a pointer
> to struct sock) happens to be odd. It won't be. The
> same goes for 32bit big-endian - there you will be checking
> the least significant bit of the 4th byte of 'sk', which
> again is asking 'is the pointer stored there odd for some
> reason?'
> 
> On 64bit big-endian you are checking if the bit 32 of
> the address of object sock->sk points to is set. And the
> answer to that is "hell knows and how could that possibly
> be relevant to anything?"

Well, the forced conversion is wrong. sorry for that.

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-09-24  3:39 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-22  6:31 [PATCH net-next] net: socket: integrate sockfd_lookup() and sockfd_lookup_light() Yajun Deng
2021-09-23 15:24 ` Jakub Kicinski
2021-09-23 16:49 ` Eric Dumazet
2021-09-24  2:39 ` yajun.deng
2021-09-24  2:49 ` yajun.deng
2021-09-24  2:56 ` Al Viro
2021-09-24  3:39 ` yajun.deng

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.