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

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

[Yajun Deng]

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 <yajun.deng@linux.dev>
---
 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

[Jakub Kicinski]

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

[yajun.deng]

September 18, 2021 9:33 AM, "Jakub Kicinski" <kuba@kernel.org> wrote:

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

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

[Cong Wang]

On Sat, Sep 18, 2021 at 5:11 AM <yajun.deng@linux.dev> wrote:
>
> September 18, 2021 9:33 AM, "Jakub Kicinski" <kuba@kernel.org> wrote:
>
> > 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.
>
> 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

[yajun.deng]

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 <yajun.deng@linux.dev> wrote:
>
> September 18, 2021 9:33 AM, "Jakub Kicinski" <kuba@kernel.org> wrote:
>
> > 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.
>
> 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

[Cong Wang]

On Mon, Sep 20, 2021 at 5:28 AM yajun.deng@linux.dev
<yajun.deng@linux.dev> 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 <yajun.deng@linux.dev> wrote:
> >
> > September 18, 2021 9:33 AM, "Jakub Kicinski" <kuba@kernel.org> wrote:
> >
> > > 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.
> >
> > 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.