[PATCH net-next] net: socket: add the case sock_no_xxx support

Subsystems: networking [general], networking [sockets], the rest

STALE1784d

6 messages, 3 authors, 2021-09-20 · open the first message on its own page

[PATCH net-next] net: socket: add the case sock_no_xxx support

From: Yajun Deng <hidden>
Date: 2021-09-16 12:30:09

Those sock_no_{mmap, socketpair, listen, accept, connect, shutdown,
sendpage} functions are used many times in struct proto_ops, but they are
meaningless. So we can add them support in socket and delete them in struct
proto_ops.

Signed-off-by: Yajun Deng <redacted>
---
 net/socket.c | 71 ++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 58 insertions(+), 13 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index 7f64a6eccf63..4d0e1a2970fb 100644
--- a/net/socket.c
+++ b/net/socket.c

@@ -1306,6 +1306,9 @@ static int sock_mmap(struct file *file, struct vm_area_struct *vma)
 {
 	struct socket *sock = file->private_data;
 
+	if (likely(!sock->ops->mmap))
+		return -ENODEV;
+
 	return sock->ops->mmap(file, sock, vma);
 }

@@ -1629,11 +1632,19 @@ int __sys_socketpair(int family, int type, int protocol, int __user *usockvec)
 		goto out;
 	}
 
-	err = sock1->ops->socketpair(sock1, sock2);
-	if (unlikely(err < 0)) {
+	if (likely(!sock1->ops->socketpair)) {
+		err = -EOPNOTSUPP;
 		sock_release(sock2);
 		sock_release(sock1);
 		goto out;
+
+	} else {
+		err = sock1->ops->socketpair(sock1, sock2);
+		if (unlikely(err < 0)) {
+			sock_release(sock2);
+			sock_release(sock1);
+			goto out;
+		}
 	}
 
 	newfile1 = sock_alloc_file(sock1, flags, NULL);

@@ -1704,6 +1715,14 @@ SYSCALL_DEFINE3(bind, int, fd, struct sockaddr __user *, umyaddr, int, addrlen)
 	return __sys_bind(fd, umyaddr, addrlen);
 }
 
+static int __sock_listen(struct socket *sock, int backlog)
+{
+	if (likely(!sock->ops->listen))
+		return -EOPNOTSUPP;
+
+	return sock->ops->listen(sock, backlog);
+}
+
 /*
  *	Perform a listen. Basically, we allow the protocol to do anything
  *	necessary for a listen, and if that works, we mark the socket as

@@ -1724,7 +1743,7 @@ int __sys_listen(int fd, int backlog)
 
 		err = security_socket_listen(sock, backlog);
 		if (!err)
-			err = sock->ops->listen(sock, backlog);
+			err = __sock_listen(sock, backlog);
 
 		fput_light(sock->file, fput_needed);
 	}

@@ -1736,6 +1755,15 @@ SYSCALL_DEFINE2(listen, int, fd, int, backlog)
 	return __sys_listen(fd, backlog);
 }
 
+static int __sock_accept(struct socket *sock, struct socket *newsock,
+			 int flags, bool kern)
+{
+	if (likely(!sock->ops->accept))
+		return -EOPNOTSUPP;
+
+	return sock->ops->accept(sock, newsock, flags, kern);
+}
+
 struct file *do_accept(struct file *file, unsigned file_flags,
 		       struct sockaddr __user *upeer_sockaddr,
 		       int __user *upeer_addrlen, int flags)

@@ -1770,8 +1798,8 @@ struct file *do_accept(struct file *file, unsigned file_flags,
 	if (err)
 		goto out_fd;
 
-	err = sock->ops->accept(sock, newsock, sock->file->f_flags | file_flags,
-					false);
+	err = __sock_accept(sock, newsock, sock->file->f_flags | file_flags,
+			    false);
 	if (err < 0)
 		goto out_fd;

@@ -1864,6 +1892,15 @@ SYSCALL_DEFINE3(accept, int, fd, struct sockaddr __user *, upeer_sockaddr,
 	return __sys_accept4(fd, upeer_sockaddr, upeer_addrlen, 0);
 }
 
+static int __sock_connect(struct socket *sock, struct sockaddr *saddr,
+			  int len, int flags)
+{
+	if (likely(!sock->ops->connect))
+		return -EOPNOTSUPP;
+
+	return sock->ops->connect(sock, saddr, len, flags);
+}
+
 /*
  *	Attempt to connect to a socket with the server address.  The address
  *	is in user space so we verify it is OK and move it to kernel space.

@@ -1893,8 +1930,8 @@ int __sys_connect_file(struct file *file, struct sockaddr_storage *address,
 	if (err)
 		goto out;
 
-	err = sock->ops->connect(sock, (struct sockaddr *)address, addrlen,
-				 sock->file->f_flags | file_flags);
+	err = __sock_connect(sock, (struct sockaddr *)address, addrlen,
+			     sock->file->f_flags | file_flags);
 out:
 	return err;
 }

@@ -2235,6 +2272,14 @@ SYSCALL_DEFINE5(getsockopt, int, fd, int, level, int, optname,
 	return __sys_getsockopt(fd, level, optname, optval, optlen);
 }
 
+static int __sock_shutdown(struct socket *sock, int how)
+{
+	if (likely(!sock->ops->shutdown))
+		return -EOPNOTSUPP;
+
+	return sock->ops->shutdown(sock, how);
+}
+
 /*
  *	Shutdown a socket.
  */

@@ -2245,7 +2290,7 @@ int __sys_shutdown_sock(struct socket *sock, int how)
 
 	err = security_socket_shutdown(sock, how);
 	if (!err)
-		err = sock->ops->shutdown(sock, how);
+		err = __sock_shutdown(sock, how);
 
 	return err;
 }

@@ -3394,7 +3439,7 @@ EXPORT_SYMBOL(kernel_bind);
 
 int kernel_listen(struct socket *sock, int backlog)
 {
-	return sock->ops->listen(sock, backlog);
+	return __sock_listen(sock, backlog);
 }
 EXPORT_SYMBOL(kernel_listen);

@@ -3419,7 +3464,7 @@ int kernel_accept(struct socket *sock, struct socket **newsock, int flags)
 	if (err < 0)
 		goto done;
 
-	err = sock->ops->accept(sock, *newsock, flags, true);
+	err = __sock_accept(sock, *newsock, flags, true);
 	if (err < 0) {
 		sock_release(*newsock);
 		*newsock = NULL;

@@ -3450,7 +3495,7 @@ EXPORT_SYMBOL(kernel_accept);
 int kernel_connect(struct socket *sock, struct sockaddr *addr, int addrlen,
 		   int flags)
 {
-	return sock->ops->connect(sock, addr, addrlen, flags);
+	return __sock_connect(sock, addr, addrlen, flags);
 }
 EXPORT_SYMBOL(kernel_connect);

@@ -3498,7 +3543,7 @@ EXPORT_SYMBOL(kernel_getpeername);
 int kernel_sendpage(struct socket *sock, struct page *page, int offset,
 		    size_t size, int flags)
 {
-	if (sock->ops->sendpage) {
+	if (unlikely(sock->ops->sendpage)) {
 		/* Warn in case the improper page to zero-copy send */
 		WARN_ONCE(!sendpage_ok(page), "improper page for zero-copy send");
 		return sock->ops->sendpage(sock, page, offset, size, flags);

@@ -3542,7 +3587,7 @@ EXPORT_SYMBOL(kernel_sendpage_locked);
 
 int kernel_sock_shutdown(struct socket *sock, enum sock_shutdown_cmd how)
 {
-	return sock->ops->shutdown(sock, how);
+	return __sock_shutdown(sock, how);
 }
 EXPORT_SYMBOL(kernel_sock_shutdown);

-- 
2.32.0

Re: [PATCH net-next] net: socket: add the case sock_no_xxx support

From: Jakub Kicinski <kuba@kernel.org>
Date: 2021-09-18 01:33:16

On Thu, 16 Sep 2021 20:29:43 +0800 Yajun Deng wrote:

Those sock_no_{mmap, socketpair, listen, accept, connect, shutdown,
sendpage} functions are used many times in struct proto_ops, but they are
meaningless. So we can add them support in socket and delete them in struct
proto_ops.

So the reason to do this is.. what exactly?

Removing a couple empty helpers (which is not even part of this patch)?

I'm not sold, sorry.

Re: [PATCH net-next] net: socket: add the case sock_no_xxx support

From: <hidden>
Date: 2021-09-18 02:54:23

September 18, 2021 9:33 AM, "Jakub Kicinski" [off-list ref] wrote:

On Thu, 16 Sep 2021 20:29:43 +0800 Yajun Deng wrote:

quoted

Those sock_no_{mmap, socketpair, listen, accept, connect, shutdown,
sendpage} functions are used many times in struct proto_ops, but they are
meaningless. So we can add them support in socket and delete them in struct
proto_ops.

So the reason to do this is.. what exactly?

Removing a couple empty helpers (which is not even part of this patch)?

I'm not sold, sorry.

When we define a struct proto_ops xxx, we only need to assign meaningful member variables that we need.
Those {mmap, socketpair, listen, accept, connect, shutdown, sendpage} members we don't need assign
it if we don't need. We just need do once in socket, not in every struct proto_ops.

These members are assigned meaningless values far more often than meaningful ones, so this patch I used likely(!!sock->ops->xxx) for this case. This is the reason why I send this patch.

I would send a set of patchs remove most of the meaningless members assigned rather than remove sock_no_{mmap, socketpair, listen, accept, connect, shutdown, sendpage} functions if this patch was accepted. 
e.g.

diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 1d816a5fd3eb..86422eb440cb 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c

@@ -1059,20 +1059,16 @@ const struct proto_ops inet_dgram_ops = {
        .release           = inet_release,
        .bind              = inet_bind,
        .connect           = inet_dgram_connect,
-       .socketpair        = sock_no_socketpair,
-       .accept            = sock_no_accept,

--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c

@@ -1059,20 +1059,16 @@ const struct proto_ops inet_dgram_ops = {
        .release           = inet_release,
        .bind              = inet_bind,
        .connect           = inet_dgram_connect,
-       .socketpair        = sock_no_socketpair,
-       .accept            = sock_no_accept,
        .getname           = inet_getname,


        .gettstamp         = sock_gettstamp,
-       .listen            = sock_no_listen,
        .shutdown          = inet_shutdown,
        .setsockopt        = sock_common_setsockopt,
        .getsockopt        = sock_common_getsockopt,
        .sendmsg           = inet_sendmsg,
        .read_sock         = udp_read_sock,
        .recvmsg           = inet_recvmsg,
-       .mmap              = sock_no_mmap,
        .sendpage          = inet_sendpage,
        .set_peek_off      = sk_set_peek_off,
 #ifdef CONFIG_COMPAT

@@ -1091,19 +1087,15 @@ static const struct proto_ops inet_sockraw_ops = {
        .release           = inet_release,
        .bind              = inet_bind,
        .connect           = inet_dgram_connect,
-       .socketpair        = sock_no_socketpair,
-       .accept            = sock_no_accept,
        .getname           = inet_getname,
        .poll              = datagram_poll,
        .ioctl             = inet_ioctl,
        .gettstamp         = sock_gettstamp,
-       .listen            = sock_no_listen,
        .shutdown          = inet_shutdown,
        .setsockopt        = sock_common_setsockopt,
        .getsockopt        = sock_common_getsockopt,
        .sendmsg           = inet_sendmsg,
        .recvmsg           = inet_recvmsg,
-       .mmap              = sock_no_mmap,
        .sendpage          = inet_sendpage,
 #ifdef CONFIG_COMPAT
        .compat_ioctl      = inet_compat_ioctl,

Re: [PATCH net-next] net: socket: add the case sock_no_xxx support

From: Cong Wang <hidden>
Date: 2021-09-19 23:52:52

On Sat, Sep 18, 2021 at 5:11 AM [off-list ref] wrote:

September 18, 2021 9:33 AM, "Jakub Kicinski" [off-list ref] wrote:

quoted

On Thu, 16 Sep 2021 20:29:43 +0800 Yajun Deng wrote:

quoted

Those sock_no_{mmap, socketpair, listen, accept, connect, shutdown,
sendpage} functions are used many times in struct proto_ops, but they are
meaningless. So we can add them support in socket and delete them in struct
proto_ops.

So the reason to do this is.. what exactly?

Removing a couple empty helpers (which is not even part of this patch)?

I'm not sold, sorry.

When we define a struct proto_ops xxx, we only need to assign meaningful member variables that we need.
Those {mmap, socketpair, listen, accept, connect, shutdown, sendpage} members we don't need assign
it if we don't need. We just need do once in socket, not in every struct proto_ops.

These members are assigned meaningless values far more often than meaningful ones, so this patch I used likely(!!sock->ops->xxx) for this case. This is the reason why I send this patch.

But you end up adding more code:

 1 file changed, 58 insertions(+), 13 deletions(-)

I don't see this as a gain from any perspective.

Thanks.

Re: Re: [PATCH net-next] net: socket: add the case sock_no_xxx support

From: yajun.deng@linux.dev <hidden>
Date: 2021-09-20 12:28:31

From: Cong Wang
Date: 2021-09-20 07:52
To: Yajun Deng
CC: Jakub Kicinski; David Miller; Linux Kernel Network Developers; LKML
Subject: Re: [PATCH net-next] net: socket: add the case sock_no_xxx support
On Sat, Sep 18, 2021 at 5:11 AM [off-list ref] wrote:

September 18, 2021 9:33 AM, "Jakub Kicinski" [off-list ref] wrote:

quoted

On Thu, 16 Sep 2021 20:29:43 +0800 Yajun Deng wrote:

quoted

Those sock_no_{mmap, socketpair, listen, accept, connect, shutdown,
sendpage} functions are used many times in struct proto_ops, but they are
meaningless. So we can add them support in socket and delete them in struct
proto_ops.

So the reason to do this is.. what exactly?

Removing a couple empty helpers (which is not even part of this patch)?

I'm not sold, sorry.

When we define a struct proto_ops xxx, we only need to assign meaningful member variables that we need.
Those {mmap, socketpair, listen, accept, connect, shutdown, sendpage} members we don't need assign
it if we don't need. We just need do once in socket, not in every struct proto_ops.

These members are assigned meaningless values far more often than meaningful ones, so this patch I used likely(!!sock->ops->xxx) for this case. This is the reason why I send this patch.

 
But you end up adding more code:
 
1 file changed, 58 insertions(+), 13 deletions(-) 

Yes，This would add more code, but this is at the cost of reducing other codes. At the same time, the code will only run  likely(!sock->ops->xxx) in most cases.  Don’t you think that this kind of meaningless thing shouldn’t be done by socket?
 
I don't see this as a gain from any perspective.
 
Thanks.

Re: Re: [PATCH net-next] net: socket: add the case sock_no_xxx support

From: Cong Wang <hidden>
Date: 2021-09-20 16:33:24

On Mon, Sep 20, 2021 at 5:28 AM yajun.deng@linux.dev
[off-list ref] wrote:

From: Cong Wang
Date: 2021-09-20 07:52
To: Yajun Deng
CC: Jakub Kicinski; David Miller; Linux Kernel Network Developers; LKML
Subject: Re: [PATCH net-next] net: socket: add the case sock_no_xxx support
On Sat, Sep 18, 2021 at 5:11 AM [off-list ref] wrote:

quoted

September 18, 2021 9:33 AM, "Jakub Kicinski" [off-list ref] wrote:

quoted

On Thu, 16 Sep 2021 20:29:43 +0800 Yajun Deng wrote:

quoted

Those sock_no_{mmap, socketpair, listen, accept, connect, shutdown,
sendpage} functions are used many times in struct proto_ops, but they are
meaningless. So we can add them support in socket and delete them in struct
proto_ops.

So the reason to do this is.. what exactly?

Removing a couple empty helpers (which is not even part of this patch)?

I'm not sold, sorry.

When we define a struct proto_ops xxx, we only need to assign meaningful member variables that we need.
Those {mmap, socketpair, listen, accept, connect, shutdown, sendpage} members we don't need assign
it if we don't need. We just need do once in socket, not in every struct proto_ops.

These members are assigned meaningless values far more often than meaningful ones, so this patch I used likely(!!sock->ops->xxx) for this case. This is the reason why I send this patch.

But you end up adding more code:

1 file changed, 58 insertions(+), 13 deletions(-)

Yes，This would add more code, but this is at the cost of reducing other codes. At the same time, the code will only run  likely(!sock->ops->xxx) in most cases.  Don’t you think that this kind of meaningless thing shouldn’t be done by socket?

I have no idea why you call it reducing code while adding 45 lines
of code. So this does not make sense to me.

Thanks.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help