[PATCH net-next 0/3] Report socket shutdown state via sock-diag

STALE5100d

8 messages, 3 authors, 2012-10-23 · open the first message on its own page

[PATCH net-next 0/3] Report socket shutdown state via sock-diag

From: Pavel Emelyanov <hidden>
Date: 2012-10-23 16:19:03

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

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

From: Pavel Emelyanov <hidden>
Date: 2012-10-23 16:22:54

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 <redacted>
---
 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(-)

diff --git a/include/net/sock.h b/include/net/sock.h
index c945fba..c42c115 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h

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

diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 766c596..3b3940c 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c

@@ -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);

diff --git a/net/iucv/af_iucv.c b/net/iucv/af_iucv.c
index cd6f7a9..308024c 100644
--- a/net/iucv/af_iucv.c
+++ b/net/iucv/af_iucv.c

@@ -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);

diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 5b5c876..14543f2 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c

@@ -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;

-- 
1.7.6.5

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

From: Pavel Emelyanov <hidden>
Date: 2012-10-23 16:26:08

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 <redacted>
---
 include/uapi/linux/inet_diag.h |    3 ++-
 net/ipv4/inet_diag.c           |    3 +++
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/include/uapi/linux/inet_diag.h b/include/uapi/linux/inet_diag.h
index 8c469af..c5e14f6 100644
--- a/include/uapi/linux/inet_diag.h
+++ b/include/uapi/linux/inet_diag.h

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

diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
index 535584c..52db768 100644
--- a/net/ipv4/inet_diag.c
+++ b/net/ipv4/inet_diag.c

@@ -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);

-- 
1.7.6.5

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

From: Pavel Emelyanov <hidden>
Date: 2012-10-23 16:28:25

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

---
 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(-)

diff --git a/include/linux/sock_diag.h b/include/linux/sock_diag.h
index e8d702e..df4b20f 100644
--- a/include/linux/sock_diag.h
+++ b/include/linux/sock_diag.h

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

diff --git a/include/net/sock.h b/include/net/sock.h
index c42c115..8b64048 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h

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

diff --git a/include/uapi/linux/inet_diag.h b/include/uapi/linux/inet_diag.h
index c5e14f6..ace76b5 100644
--- a/include/uapi/linux/inet_diag.h
+++ b/include/uapi/linux/inet_diag.h

@@ -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 */

diff --git a/include/uapi/linux/unix_diag.h b/include/uapi/linux/unix_diag.h
index b1d2bf1..2113d90 100644
--- a/include/uapi/linux/unix_diag.h
+++ b/include/uapi/linux/unix_diag.h

@@ -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,
 };

diff --git a/net/core/sock_diag.c b/net/core/sock_diag.c
index 602cd63..152db24 100644
--- a/net/core/sock_diag.c
+++ b/net/core/sock_diag.c

@@ -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);

diff --git a/net/ipv4/inet_diag.c b/net/ipv4/inet_diag.c
index 52db768..3b91f86 100644
--- a/net/ipv4/inet_diag.c
+++ b/net/ipv4/inet_diag.c

@@ -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);

diff --git a/net/unix/diag.c b/net/unix/diag.c
index 06748f1..e41308c 100644
--- a/net/unix/diag.c
+++ b/net/unix/diag.c

@@ -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:

-- 
1.7.6.5

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

From: Eric Dumazet <hidden>
Date: 2012-10-23 16:53:19

On Tue, 2012-10-23 at 20:28 +0400, Pavel Emelyanov wrote:

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

quoted hunk

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

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

From: Pavel Emelyanov <hidden>
Date: 2012-10-23 17:26:29

On 10/23/2012 08:53 PM, Eric Dumazet wrote:

On Tue, 2012-10-23 at 20:28 +0400, Pavel Emelyanov wrote:

quoted

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

quoted

 
+	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

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

From: David Miller <davem@davemloft.net>
Date: 2012-10-23 17:42:37

From: Pavel Emelyanov <redacted>
Date: Tue, 23 Oct 2012 20:28:22 +0400

quoted hunk

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

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

From: Pavel Emelyanov <hidden>
Date: 2012-10-23 17:53:09

On 10/23/2012 09:42 PM, David Miller wrote:

From: Pavel Emelyanov <redacted>
Date: Tue, 23 Oct 2012 20:28:22 +0400

quoted

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

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

`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