Skip to contentSkip to conversation

Report socket shutdown state via sock-diag

PATCH In review Pavel Emelyanov 3 participants 8 messages Oct 23, 2012

Subsystems (5)
Networking Network drivers Sockets Networking unix sockets +1 more subsystems
lore
Share & tools
lore

Pavel Emelyanov

author
#0 lore raw

cover Report socket shutdown state via sock-diag

The sk->sk_shutdown mask (set by sys_shutdown) cannot be read from the user space. I suppose that the best API to report it is the recently added sock-diag one (as we can use it not only in the checkpoint-restore project, but also enhance the ss output with the shutdown info).

However, there's one issue with the inet-diag part of it -- we've run out of bits on the extension flags of dump request. This one is addressed in the patch #2.

Thanks,
Pavel

author
#1 lore raw

PATCH 1/3 sk: Introduce the shutdown user-to-mask routine

The sys_shutdown "how" argument is converted into kernel-side mask with a trick -- the "how++" does proper bits conversion. And since this is a trick, it's commented in the respective places.

When there will be sk_shutdown reporting mechanism it will be natural to report not the kernel-side mask, but the values known by userspace (this "how" thing), i.e. we'll have to do the reciprocal trick.

I propose to encapsulate both tricks in helpers, here's the user to kernel one.

Signed-off-by Pavel Emelyanov <[email protected]>
---
 include/net/sock.h |   18 ++++++++++++++++++
 net/ipv4/af_inet.c |    7 +++----
 net/iucv/af_iucv.c |    5 ++---
 net/unix/af_unix.c |    9 ++-------
 4 files changed, 25 insertions(+), 14 deletions(-)
include/net/sock.h +18 -0
index c945fba..c42c115 100644
@@ -1265,6 +1265,24 @@ void sk_prot_clear_portaddr_nulls(struct sock *sk, int size);
 #define RCV_SHUTDOWN	1
 #define SEND_SHUTDOWN	2
 
+static inline int shutdown_u2mask(int uhow)
+{
+	/*
+	 * map
+	 * 	SHUT_RD -> RCV_SHUTDOWN
+	 * 	SHUT_WR -> SEND_SHUTDOWN
+	 * 	SHUT_RDWR -> SHUTDOWN_MASK
+	 *
+	 * or report 0 on error
+	 */
+
+	uhow++;
+	if (uhow & ~SHUTDOWN_MASK)
+		uhow = 0;
+
+	return uhow;
+}
+
 #define SOCK_SNDBUF_LOCK	1
 #define SOCK_RCVBUF_LOCK	2
 #define SOCK_BINDADDR_LOCK	4
net/ipv4/af_inet.c +3 -4
index 766c596..3b3940c 100644
@@ -825,10 +825,9 @@ int inet_shutdown(struct socket *sock, int how)
 	/* This should really check to make sure
 	 * the socket is a TCP socket. (WHY AC...)
 	 */
-	how++; /* maps 0->1 has the advantage of making bit 1 rcvs and
-		       1->2 bit 2 snds.
-		       2->3 */
-	if ((how & ~SHUTDOWN_MASK) || !how)	/* MAXINT->0 */
+
+	how = shutdown_u2mask(how);
+	if (!how)
 		return -EINVAL;
 
 	lock_sock(sk);
net/iucv/af_iucv.c +2 -3
index cd6f7a9..308024c 100644
@@ -1497,9 +1497,8 @@ static int iucv_sock_shutdown(struct socket *sock, int how)
 	struct iucv_message txmsg;
 	int err = 0;
 
-	how++;
-
-	if ((how & ~SHUTDOWN_MASK) || !how)
+	how = shutdown_u2mask(how);
+	if (!how)
 		return -EINVAL;
 
 	lock_sock(sk);
net/unix/af_unix.c +2 -7
index 5b5c876..14543f2 100644
@@ -2057,14 +2057,9 @@ static int unix_shutdown(struct socket *sock, int mode)
 	struct sock *sk = sock->sk;
 	struct sock *other;
 
-	if (mode < SHUT_RD || mode > SHUT_RDWR)
+	mode = shutdown_u2mask(mode);
+	if (!mode)
 		return -EINVAL;
-	/* This maps:
-	 * SHUT_RD   (0) -> RCV_SHUTDOWN  (1)
-	 * SHUT_WR   (1) -> SEND_SHUTDOWN (2)
-	 * SHUT_RDWR (2) -> SHUTDOWN_MASK (3)
-	 */
-	++mode;
 
 	unix_state_lock(sk);
 	sk->sk_shutdown |= mode;
Signature · 1 line
1.7.6.5
author
#2 lore raw

PATCH 2/3 inet-diag: Get more bits for extension flag

In reply to Pavel Emelyanov

The inet-diag dumping request carries 8-bit ext value which denotes what additional info we want to see. Unfortunately there's only one bit left on it and occupying one with the "I want shutdown" is unwanted.

However, there are 8 more bits in the request and this "pad" field is currently unused. I propose to make the 8th bit of the existing ext flag mean "this unused space contains more ext bits".

Signed-off-by Pavel Emelyanov <[email protected]>
---
 include/uapi/linux/inet_diag.h |    3 ++-
 net/ipv4/inet_diag.c           |    3 +++
 2 files changed, 5 insertions(+), 1 deletions(-)
include/uapi/linux/inet_diag.h +2 -1
index 8c469af..c5e14f6 100644
@@ -38,7 +38,7 @@ struct inet_diag_req_v2 {
 	__u8	sdiag_family;
 	__u8	sdiag_protocol;
 	__u8	idiag_ext;
-	__u8	pad;
+	__u8	idiag_ext2;
 	__u32	idiag_states;
 	struct inet_diag_sockid id;
 };

@@ -109,6 +109,7 @@ enum {
 	INET_DIAG_TOS,
 	INET_DIAG_TCLASS,
 	INET_DIAG_SKMEMINFO,
+	__INET_DIAG_EXT2,
 };
 
 #define INET_DIAG_MAX INET_DIAG_SKMEMINFO
net/ipv4/inet_diag.c +3 -0
index 535584c..52db768 100644
@@ -81,6 +81,9 @@ int inet_sk_diag_fill(struct sock *sk, struct inet_connection_sock *icsk,
 	const struct inet_diag_handler *handler;
 	int ext = req->idiag_ext;
 
+	if (ext & (1 << (__INET_DIAG_EXT2 - 1)))
+		ext |= req->idiag_ext2 << __INET_DIAG_EXT2;
+
 	handler = inet_diag_table[req->sdiag_protocol];
 	BUG_ON(handler == NULL);
Signature · 1 line
1.7.6.5
author
#3 lore raw

PATCH 3/3 sock-diag: Report shutdown for inet and unix sockets

In reply to Pavel Emelyanov

Add ext bits for inet-diag and unix-diag and report sk_shutdown state.

When it's zero (i.e. -- no shutdown on a socket) diag reports nothing for this request. Otherwise report user-space known values using the proper mask-to-user conversion trick.

Signed-off-by Pavel Emelyanov <[email protected]>
---
 include/linux/sock_diag.h      |    1 +
 include/net/sock.h             |   12 ++++++++++++
 include/uapi/linux/inet_diag.h |    3 ++-
 include/uapi/linux/unix_diag.h |    2 ++
 net/core/sock_diag.c           |    9 +++++++++
 net/ipv4/inet_diag.c           |    4 ++++
 net/unix/diag.c                |    4 ++++
 7 files changed, 34 insertions(+), 1 deletions(-)
include/linux/sock_diag.h +1 -0
index e8d702e..df4b20f 100644
@@ -22,5 +22,6 @@ int sock_diag_check_cookie(void *sk, __u32 *cookie);
 void sock_diag_save_cookie(void *sk, __u32 *cookie);
 
 int sock_diag_put_meminfo(struct sock *sk, struct sk_buff *skb, int attr);
+int sock_diag_put_shutdown(struct sock *sk, struct sk_buff *skb, int attr);
 
 #endif
include/net/sock.h +12 -0
index c42c115..8b64048 100644
@@ -1283,6 +1283,18 @@ static inline int shutdown_u2mask(int uhow)
 	return uhow;
 }
 
+static inline int shutdown_mask2u(int mask)
+{
+	/*
+	 * map
+	 * 	RCV_SHUTDOWN -> SHUT_RD
+	 * 	SEND_SHUTDOWN -> SHUT_WR
+	 * 	SHUTDOWN_MASK -> SHUT_RDWR
+	 */
+
+	return mask - 1;
+}
+
 #define SOCK_SNDBUF_LOCK	1
 #define SOCK_RCVBUF_LOCK	2
 #define SOCK_BINDADDR_LOCK	4
include/uapi/linux/inet_diag.h +2 -1
index c5e14f6..ace76b5 100644
@@ -110,9 +110,10 @@ enum {
 	INET_DIAG_TCLASS,
 	INET_DIAG_SKMEMINFO,
 	__INET_DIAG_EXT2,
+	INET_DIAG_SHUTDOWN,
 };
 
-#define INET_DIAG_MAX INET_DIAG_SKMEMINFO
+#define INET_DIAG_MAX INET_DIAG_SHUTDOWN
 
 
 /* INET_DIAG_MEM */
include/uapi/linux/unix_diag.h +2 -0
index b1d2bf1..2113d90 100644
@@ -19,6 +19,7 @@ struct unix_diag_req {
 #define UDIAG_SHOW_ICONS	0x00000008	/* show pending connections */
 #define UDIAG_SHOW_RQLEN	0x00000010	/* show skb receive queue len */
 #define UDIAG_SHOW_MEMINFO	0x00000020	/* show memory info of a socket */
+#define UDIAG_SHOW_SHUTDOWN	0x00000040	/* show shutdown state */
 
 struct unix_diag_msg {
 	__u8	udiag_family;

@@ -37,6 +38,7 @@ enum {
 	UNIX_DIAG_ICONS,
 	UNIX_DIAG_RQLEN,
 	UNIX_DIAG_MEMINFO,
+	UNIX_DIAG_SHUTDOWN,
 
 	UNIX_DIAG_MAX,
 };
net/core/sock_diag.c +9 -0
index 602cd63..152db24 100644
@@ -49,6 +49,15 @@ int sock_diag_put_meminfo(struct sock *sk, struct sk_buff *skb, int attrtype)
 }
 EXPORT_SYMBOL_GPL(sock_diag_put_meminfo);
 
+int sock_diag_put_shutdown(struct sock *sk, struct sk_buff *skb, int attrtype)
+{
+	if (!sk->sk_shutdown)
+		return 0;
+
+	return nla_put_u32(skb, attrtype, shutdown_mask2u(sk->sk_shutdown));
+}
+EXPORT_SYMBOL_GPL(sock_diag_put_shutdown);
+
 void sock_diag_register_inet_compat(int (*fn)(struct sk_buff *skb, struct nlmsghdr *nlh))
 {
 	mutex_lock(&sock_diag_table_mutex);
net/ipv4/inet_diag.c +4 -0
index 52db768..3b91f86 100644
@@ -115,6 +115,10 @@ int inet_sk_diag_fill(struct sock *sk, struct inet_connection_sock *icsk,
 		if (nla_put_u8(skb, INET_DIAG_TOS, inet->tos) < 0)
 			goto errout;
 
+	if (ext & (1 << (INET_DIAG_SHUTDOWN - 1)))
+		if (sock_diag_put_shutdown(sk, skb, INET_DIAG_SHUTDOWN))
+			goto errout;
+
 #if IS_ENABLED(CONFIG_IPV6)
 	if (r->idiag_family == AF_INET6) {
 		const struct ipv6_pinfo *np = inet6_sk(sk);
net/unix/diag.c +4 -0
index 06748f1..e41308c 100644
@@ -151,6 +151,10 @@ static int sk_diag_fill(struct sock *sk, struct sk_buff *skb, struct unix_diag_r
 	    sock_diag_put_meminfo(sk, skb, UNIX_DIAG_MEMINFO))
 		goto out_nlmsg_trim;
 
+	if ((req->udiag_show & UDIAG_SHOW_SHUTDOWN) &&
+	    sock_diag_put_shutdown(sk, skb, UNIX_DIAG_SHUTDOWN))
+		goto out_nlmsg_trim;
+
 	return nlmsg_end(skb, nlh);
 
 out_nlmsg_trim:
Signature · 1 line
1.7.6.5
#4 lore raw

Re: [PATCH 3/3] sock-diag: Report shutdown for inet and unix sockets

net/ipv4/inet_diag.c int inet_sk_diag_fill(struct sock *sk, struct inet_connection_sock *icsk, · line 121

Quoted 7 lines · Pavel Emelyanov

Add ext bits for inet-diag and unix-diag and report sk_shutdown state.

net/ipv4/inet_diag.c +4 -0
+	if (ext & (1 << (INET_DIAG_SHUTDOWN - 1)))
+		if (sock_diag_put_shutdown(sk, skb, INET_DIAG_SHUTDOWN))
+			goto errout;
+

I dont feel the need to make this conditional and consume one bit.

author
#5 lore raw

Re: [PATCH 3/3] sock-diag: Report shutdown for inet and unix sockets

Quoted 10 lines · Eric Dumazet

net/ipv4/inet_diag.c int inet_sk_diag_fill(struct sock *sk, struct inet_connection_sock *icsk, · line 121

Quoted 7 lines · Pavel Emelyanov

Add ext bits for inet-diag and unix-diag and report sk_shutdown state.

net/ipv4/inet_diag.c +4 -0
+	if (ext & (1 << (INET_DIAG_SHUTDOWN - 1)))
+		if (sock_diag_put_shutdown(sk, skb, INET_DIAG_SHUTDOWN))
+			goto errout;
+

I dont feel the need to make this conditional and consume one bit.

OK, but I have one concern about it.

I'll have to add the attrtype for it anyway. And when later we'll add yet another one, say INET_DIAG_FOO, we will not be able to request this in such

   req->ext |= (1 << (INET_DIAG_FOO - 1));

manner as it's currently done. If that's OK, I will rework the patch.

Thanks,
Pavel

maintainer
#6 lore raw

Re: [PATCH 3/3] sock-diag: Report shutdown for inet and unix sockets

In reply to Pavel Emelyanov

From: Pavel Emelyanov <[email protected]>
Date: Tue, 23 Oct 2012 20:28:22 +0400

include/net/sock.h static inline int shutdown_u2mask(int uhow) · line 1296

Quoted 11 lines
include/net/sock.h +11 -0
+static inline int shutdown_mask2u(int mask)
+{
+	/*
+	 * map
+	 * 	RCV_SHUTDOWN -> SHUT_RD
+	 * 	SEND_SHUTDOWN -> SHUT_WR
+	 * 	SHUTDOWN_MASK -> SHUT_RDWR
+	 */
+
+	return mask - 1;
+}

This is horrible.

You're returning "-1" when the socket hasn't been shutdown in any way.

Do this:

1) Use a '1' based encoding like the kernel codes so that '0' means no shutdown, as any sane interface would. That's why we use that representation internally.

2) Get rid of all of this extension crap, and just report this value in the pad byte. In older kernels it will just be zero, which is fine.

author
#7 lore raw

Re: [PATCH 3/3] sock-diag: Report shutdown for inet and unix sockets

Quoted 21 lines · David Miller

From: Pavel Emelyanov <[email protected]>
Date: Tue, 23 Oct 2012 20:28:22 +0400

include/net/sock.h static inline int shutdown_u2mask(int uhow) · line 1296

Quoted 11 lines
include/net/sock.h +11 -0
+static inline int shutdown_mask2u(int mask)
+{
+	/*
+	 * map
+	 * 	RCV_SHUTDOWN -> SHUT_RD
+	 * 	SEND_SHUTDOWN -> SHUT_WR
+	 * 	SHUTDOWN_MASK -> SHUT_RDWR
+	 */
+
+	return mask - 1;
+}

This is horrible.

You're returning "-1" when the socket hasn't been shutdown in any way.

Do this:

Quoted 7 lines

1) Use a '1' based encoding like the kernel codes so that '0' means no shutdown, as any sane interface would. That's why we use that representation internally.

2) Get rid of all of this extension crap, and just report this value in the pad byte. In older kernels it will just be zero, which is fine.

The response message of inet-diag never had any pad bytes, I'll have to add the new attrtype (the unix-diag response has one, but I plan to add attrtype there too for uniformity).

Thanks,
Pavel