Skip to contentSkip to conversation

net: Allow protocols to provide an unlocked_recvmsg sk_prot method

RFC In review Arnaldo Carvalho de Melo 3 participants 4 messages May 20, 2009 – May 22, 2009

Subsystems (4)
Networking Network drivers Sockets UDP
lore
Share & tools
lore

Arnaldo Carvalho de Melo

author
#0 lore raw

RFC 2/2 net: Allow protocols to provide an unlocked_recvmsg sk_prot method

So that the socket layer kwows which protocol uses locking and can ask for an unlocked recvmsg call inside recvmmsg, that takes the lock for a batch of packets.

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 include/linux/socket.h |    3 ++
 include/net/sock.h     |    5 ++++
 net/core/sock.c        |   11 +++++++--
 net/ipv4/udp.c         |   52 ++++++++++++++++++++++++++++++++++++++++-------
 net/socket.c           |   39 ++++++++++++++++++++++++-----------
 5 files changed, 87 insertions(+), 23 deletions(-)
include/linux/socket.h +3 -0
index 50c6c44..7ef30a3 100644
@@ -265,6 +265,9 @@ struct ucred {
 #define MSG_ERRQUEUE	0x2000	/* Fetch message from error queue */
 #define MSG_NOSIGNAL	0x4000	/* Do not generate SIGPIPE */
 #define MSG_MORE	0x8000	/* Sender will send more */
+#ifdef __KERNEL__
+#define MSG_UNLOCKED	0x10000	/* Don't lock the sock */
+#endif
 
 #define MSG_EOF         MSG_FIN
include/net/sock.h +5 -0
index da2ea5f..43c231c 100644
@@ -637,6 +637,11 @@ struct proto {
 					   struct msghdr *msg,
 					size_t len, int noblock, int flags, 
 					int *addr_len);
+	int			(*unlocked_recvmsg)(struct kiocb *iocb,
+						    struct sock *sk,
+						    struct msghdr *msg,
+						    size_t len, int noblock,
+						    int flags, int *addr_len);
 	int			(*sendpage)(struct sock *sk, struct page *page,
 					int offset, size_t size, int flags);
 	int			(*bind)(struct sock *sk,
net/core/sock.c +8 -3
index 9730820..2640ab7 100644
@@ -1918,11 +1918,16 @@ int sock_common_recvmsg(struct kiocb *iocb, struct socket *sock,
 			struct msghdr *msg, size_t size, int flags)
 {
 	struct sock *sk = sock->sk;
-	int addr_len = 0;
 	int err;
+	int addr_len = 0;
+
+	BUG_ON((flags & MSG_UNLOCKED) &&
+	       sk->sk_prot->unlocked_recvmsg == NULL);
 
-	err = sk->sk_prot->recvmsg(iocb, sk, msg, size, flags & MSG_DONTWAIT,
-				   flags & ~MSG_DONTWAIT, &addr_len);
+	err = ((flags & MSG_UNLOCKED) ?
+	       sk->sk_prot->unlocked_recvmsg :
+	       sk->sk_prot->recvmsg)(iocb, sk, msg, size, flags & MSG_DONTWAIT,
+				     flags & ~MSG_DONTWAIT, &addr_len);
 	if (err >= 0)
 		msg->msg_namelen = addr_len;
 	return err;
net/ipv4/udp.c +44 -8

86 lines not on this page · raw message

net/socket.c +27 -12
index f0249cb..3ab1520 100644
@@ -2084,29 +2084,23 @@ out:
  *	Linux recvmmsg interface
  */
 
-SYSCALL_DEFINE4(recvmmsg, int, fd, struct mmsghdr __user *, mmsg,
-		unsigned int, vlen, unsigned int, flags)
+static int __sys_recvmmsg(struct socket *sock, struct mmsghdr __user *mmsg,
+			  unsigned vlen, unsigned flags)
 {
-	int fput_needed, err, datagrams = 0;
-	struct socket *sock = sockfd_lookup_light(fd, &err, &fput_needed);
+	int err, datagrams = 0;
 	struct mmsghdr __user *entry = mmsg;
 
-	if (!sock)
-		goto out;
-
 	while (datagrams < vlen) {
 		err = __sys_recvmsg(sock, (struct msghdr __user *)entry, flags);
 		if (err < 0)
-			goto out_put;
+			break;
 		err = __put_user(err, &entry->msg_len);
 		if (err)
-			goto out_put;
+			break;
 		++entry;
 		++datagrams;
 	}
-out_put:
-	fput_light(sock->file, fput_needed);
-out:
+
 	/*
 	 * We may return less entries than requested (vlen) if the
 	 * sock is non block and there aren't enough datagrams.

@@ -2116,6 +2110,27 @@ out:
 	return err;
 }
 
+SYSCALL_DEFINE4(recvmmsg, int, fd, struct mmsghdr __user *, mmsg,
+		unsigned int, vlen, unsigned int, flags)
+{
+	int fput_needed, err;
+	struct socket *sock = sockfd_lookup_light(fd, &err, &fput_needed);
+
+	if (!sock)
+		goto out;
+
+	if (sock->sk->sk_prot->unlocked_recvmsg) {
+		lock_sock(sock->sk);
+		err = __sys_recvmmsg(sock, mmsg, vlen, flags | MSG_UNLOCKED);
+		release_sock(sock->sk);
+	} else
+		err = __sys_recvmmsg(sock, mmsg, vlen, flags);
+	
+	fput_light(sock->file, fput_needed);
+out:
+	return err;
+}
+
 #ifdef __ARCH_WANT_SYS_SOCKETCALL
 
 /* Argument list sizes for sys_socketcall */
Signature · 1 line
1.6.0.6
#1 lore raw
Quoted 19 lines · On Thursday 21 May 2009 02:06:59 ext Arnaldo Carvalho de Melo wrote:

So that the socket layer kwows which protocol uses locking and can ask for an unlocked recvmsg call inside recvmmsg, that takes the lock for a batch of packets.

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 include/linux/socket.h |    3 ++
 include/net/sock.h     |    5 ++++
 net/core/sock.c        |   11 +++++++--
 net/ipv4/udp.c         |   52

++++++++++++++++++++++++++++++++++++++++------- net/socket.c |
39 ++++++++++++++++++++++++----------- 5 files changed, 87 insertions(+), 23 deletions(-)

include/linux/socket.h +0 -0
index 50c6c44..7ef30a3 100644

include/linux/socket.h struct ucred { · line 270

Quoted 6 lines
include/linux/socket.h +3 -0
@@ -265,6 +265,9 @@ struct ucred {
 #define MSG_ERRQUEUE	0x2000	/* Fetch message from error queue */
 #define MSG_NOSIGNAL	0x4000	/* Do not generate SIGPIPE */
 #define MSG_MORE	0x8000	/* Sender will send more */
+#ifdef __KERNEL__
+#define MSG_UNLOCKED	0x10000	/* Don't lock the sock */
+#endif

I might be missing something but... What prevents an evil userland from setting the flag anyway and hitting the BUG case?

Signature · 2 lines
Rémi Denis-Courmont
Nokia Devices R&D, Maemo Software, Helsinki
maintainer
#2 lore raw

From: "Rémi Denis-Courmont" <[email protected]>
Date: Fri, 22 May 2009 10:26:51 +0300

Quoted 11 lines · On Thursday 21 May 2009 02:06:59 ext Arnaldo Carvalho de Melo wrote:

include/linux/socket.h struct ucred { · line 270

Quoted 7 lines
include/linux/socket.h +3 -0
@@ -265,6 +265,9 @@ struct ucred {
 #define MSG_ERRQUEUE	0x2000	/* Fetch message from error queue */
 #define MSG_NOSIGNAL	0x4000	/* Do not generate SIGPIPE */
 #define MSG_MORE	0x8000	/* Sender will send more */
+#ifdef __KERNEL__
+#define MSG_UNLOCKED	0x10000	/* Don't lock the sock */
+#endif

I might be missing something but... What prevents an evil userland from setting the flag anyway and hitting the BUG case?

Yes, we'll need to clear this on all paths where we get msg flags from the user.

There's a lot of such places.

So maybe we need to pass this state around in a different, internal, way.

author
#3 lore raw

Em Fri, May 22, 2009 at 12:47:38AM -0700, David Miller escreveu:

Quoted 15 lines

From: "Rémi Denis-Courmont" <[email protected]>
Date: Fri, 22 May 2009 10:26:51 +0300

Quoted 11 lines · On Thursday 21 May 2009 02:06:59 ext Arnaldo Carvalho de Melo wrote:

include/linux/socket.h struct ucred { · line 270

Quoted 7 lines
include/linux/socket.h +3 -0
@@ -265,6 +265,9 @@ struct ucred {
 #define MSG_ERRQUEUE	0x2000	/* Fetch message from error queue */
 #define MSG_NOSIGNAL	0x4000	/* Do not generate SIGPIPE */
 #define MSG_MORE	0x8000	/* Sender will send more */
+#ifdef __KERNEL__
+#define MSG_UNLOCKED	0x10000	/* Don't lock the sock */
+#endif

I might be missing something but... What prevents an evil userland from setting the flag anyway and hitting the BUG case?

Quoted 7 lines

Yes, we'll need to clear this on all paths where we get msg flags from the user.

There's a lot of such places.

So maybe we need to pass this state around in a different, internal, way.

Yeah, I'll think about it, that was the easiest way to implement it for the proof of concept we have now. Filtering it out at syscall entry and at sock_common_recvmsg would fix it, but I'm not sure if its the best option.

The comments about the interface provided to userspace (struct mmsghdr), how to return errors after some datagrams were put in the array (sk->sk_err being stored then returned in the next call), timeouts, etc are great, thanks, after some more comments I'll respin these patches.

- Arnaldo