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 <[email protected]>
---
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
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.
September 18, 2021 9:33 AM, "Jakub Kicinski" <[email protected]> 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,
On Sat, Sep 18, 2021 at 5:11 AM <[email protected]> wrote:
>
> September 18, 2021 9:33 AM, "Jakub Kicinski" <[email protected]> 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.
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 <[email protected]> wrote:
>
> September 18, 2021 9:33 AM, "Jakub Kicinski" <[email protected]> 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.
On Mon, Sep 20, 2021 at 5:28 AM [email protected]
<[email protected]> 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 <[email protected]> wrote:
> >
> > September 18, 2021 9:33 AM, "Jakub Kicinski" <[email protected]> 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.