[PATCH mptcp-next v3 0/4] mptcp: inet diag listen dump support

[PATCH mptcp-next v3 0/4] mptcp: inet diag listen dump support

[Florian Westphal]

Changes in v3: fix intendation, fix uninitialised var (Mat)
	       extend mptcp diag tests

Changes in v2: drop patch 4 and prefer msk->first (Paolo Abeni)

Done in a not-very-elegant way:
Iterate over tcp listen hash, then pick out the tcp sockets
with mptcp-ctx structure attached, then take the conn->sk for dumping.

First patch is preparation/cleanup.
Second patch gets rid of locking to avoid a lockdep splat.
If the socket lock is really needed (I don't see where) I can workaround
this by dropping locks temporarily when iterating the listen hash table,
but its a bit more awkward.

Sample output:
ss -Mil
State        Recv-Q Send-Q Local Address:Port  Peer Address:Port
LISTEN       0      20     127.0.0.1:12000     0.0.0.0:*
         subflows_max:2

Florian Westphal (4):
  mptcp: diag: switch to context structure
  mptcp: remove locking in mptcp_diag_fill_info
  mptcp: listen diag dump support
  selftests/mptcp: add diag listen tests

 net/mptcp/mptcp_diag.c                    | 105 +++++++++++++++++++++-
 net/mptcp/sockopt.c                       |   6 --
 tools/testing/selftests/net/mptcp/diag.sh |  38 ++++++++
 3 files changed, 140 insertions(+), 9 deletions(-)

-- 
2.35.1

[PATCH mptcp-next v3 1/4] mptcp: diag: switch to context structure

[Florian Westphal]

Raw access to cb->arg[] is deprecated, use a context structure.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 no change.
 net/mptcp/mptcp_diag.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/mptcp_diag.c b/net/mptcp/mptcp_diag.c
index f44125dd6697..c4992eeb67d8 100644
--- a/net/mptcp/mptcp_diag.c
+++ b/net/mptcp/mptcp_diag.c
@@ -66,20 +66,28 @@ static int mptcp_diag_dump_one(struct netlink_callback *cb,
 	return err;
 }
 
+struct mptcp_diag_ctx {
+	long s_slot;
+	long s_num;
+};
+
 static void mptcp_diag_dump(struct sk_buff *skb, struct netlink_callback *cb,
 			    const struct inet_diag_req_v2 *r)
 {
 	bool net_admin = netlink_net_capable(cb->skb, CAP_NET_ADMIN);
+	struct mptcp_diag_ctx *diag_ctx = (void *)cb->ctx;
 	struct net *net = sock_net(skb->sk);
 	struct inet_diag_dump_data *cb_data;
 	struct mptcp_sock *msk;
 	struct nlattr *bc;
 
+	BUILD_BUG_ON(sizeof(cb->ctx) < sizeof(*diag_ctx));
+
 	cb_data = cb->data;
 	bc = cb_data->inet_diag_nla_bc;
 
-	while ((msk = mptcp_token_iter_next(net, &cb->args[0], &cb->args[1])) !=
-	       NULL) {
+	while ((msk = mptcp_token_iter_next(net, &diag_ctx->s_slot,
+					    &diag_ctx->s_num)) != NULL) {
 		struct inet_sock *inet = (struct inet_sock *)msk;
 		struct sock *sk = (struct sock *)msk;
 		int ret = 0;
@@ -101,7 +109,7 @@ static void mptcp_diag_dump(struct sk_buff *skb, struct netlink_callback *cb,
 		sock_put(sk);
 		if (ret < 0) {
 			/* will retry on the same position */
-			cb->args[1]--;
+			diag_ctx->s_num--;
 			break;
 		}
 		cond_resched();
-- 
2.35.1

[PATCH mptcp-next v3 2/4] mptcp: remove locking in mptcp_diag_fill_info

[Florian Westphal]

Problem is that listener iteration would call this from atomic context
so this locking is not allowed.

One way is to drop locks before calling the helper, but afaics the lock
isn't really needed, all values are fetched via READ_ONCE().

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 no change.
 net/mptcp/sockopt.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index f949d22f52bd..826b0c1dae98 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -853,15 +853,11 @@ static int mptcp_getsockopt_first_sf_only(struct mptcp_sock *msk, int level, int
 
 void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
 {
-	struct sock *sk = &msk->sk.icsk_inet.sk;
 	u32 flags = 0;
-	bool slow;
 	u8 val;
 
 	memset(info, 0, sizeof(*info));
 
-	slow = lock_sock_fast(sk);
-
 	info->mptcpi_subflows = READ_ONCE(msk->pm.subflows);
 	info->mptcpi_add_addr_signal = READ_ONCE(msk->pm.add_addr_signaled);
 	info->mptcpi_add_addr_accepted = READ_ONCE(msk->pm.add_addr_accepted);
@@ -882,8 +878,6 @@ void mptcp_diag_fill_info(struct mptcp_sock *msk, struct mptcp_info *info)
 	info->mptcpi_snd_una = READ_ONCE(msk->snd_una);
 	info->mptcpi_rcv_nxt = READ_ONCE(msk->ack_seq);
 	info->mptcpi_csum_enabled = READ_ONCE(msk->csum_enabled);
-
-	unlock_sock_fast(sk, slow);
 }
 EXPORT_SYMBOL_GPL(mptcp_diag_fill_info);
 
-- 
2.35.1

[PATCH mptcp-next v3 3/4] mptcp: listen diag dump support

[Florian Westphal]

makes 'ss -Ml' show mptcp listen sockets.

Iterate over the tcp listen sockets and pick those that have mptcp ulp
info attached.

mptcp_diag_get_info() is modified to prefer msk->first for mptcp sockets
in listen state.  This reports accurate number for recv and send queue
(pending / max connection backlog counters).

Sample output:
ss -Mil
State        Recv-Q Send-Q Local Address:Port  Peer Address:Port
LISTEN       0      20     127.0.0.1:12000     0.0.0.0:*
         subflows_max:2

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 Changes:
  - init nlattr *bc properly
  - fix sock_put indentation



 net/mptcp/mptcp_diag.c | 91 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 91 insertions(+)

diff --git a/net/mptcp/mptcp_diag.c b/net/mptcp/mptcp_diag.c
index c4992eeb67d8..dbb6d876a203 100644
--- a/net/mptcp/mptcp_diag.c
+++ b/net/mptcp/mptcp_diag.c
@@ -69,8 +69,83 @@ static int mptcp_diag_dump_one(struct netlink_callback *cb,
 struct mptcp_diag_ctx {
 	long s_slot;
 	long s_num;
+	unsigned int l_slot;
+	unsigned int l_num;
 };
 
+static void mptcp_diag_dump_listeners(struct sk_buff *skb, struct netlink_callback *cb,
+				      const struct inet_diag_req_v2 *r,
+				      bool net_admin)
+{
+	struct inet_diag_dump_data *cb_data = cb->data;
+	struct mptcp_diag_ctx *diag_ctx = (void *)cb->ctx;
+	struct nlattr *bc = cb_data->inet_diag_nla_bc;
+	struct net *net = sock_net(skb->sk);
+	int i;
+
+	for (i = diag_ctx->l_slot; i < INET_LHTABLE_SIZE; i++) {
+		struct inet_listen_hashbucket *ilb;
+		struct hlist_nulls_node *node;
+		struct sock *sk;
+		int num = 0;
+
+		ilb = &tcp_hashinfo.listening_hash[i];
+
+		rcu_read_lock();
+		spin_lock(&ilb->lock);
+		sk_nulls_for_each(sk, node, &ilb->nulls_head) {
+			const struct mptcp_subflow_context *ctx = mptcp_subflow_ctx(sk);
+			struct inet_sock *inet = inet_sk(sk);
+			int ret;
+
+			if (num < diag_ctx->l_num)
+				goto next_listen;
+
+			if (!ctx || strcmp(inet_csk(sk)->icsk_ulp_ops->name, "mptcp"))
+				goto next_listen;
+
+			sk = ctx->conn;
+			if (!sk || !net_eq(sock_net(sk), net))
+				goto next_listen;
+
+			if (r->sdiag_family != AF_UNSPEC &&
+			    sk->sk_family != r->sdiag_family)
+				goto next_listen;
+
+			if (r->id.idiag_sport != inet->inet_sport &&
+			    r->id.idiag_sport)
+				goto next_listen;
+
+			if (!refcount_inc_not_zero(&sk->sk_refcnt))
+				goto next_listen;
+
+			ret = sk_diag_dump(sk, skb, cb, r, bc, net_admin);
+
+			sock_put(sk);
+
+			if (ret < 0) {
+				spin_unlock(&ilb->lock);
+				rcu_read_unlock();
+				diag_ctx->l_slot = i;
+				diag_ctx->l_num = num;
+				return;
+			}
+			diag_ctx->l_num = num + 1;
+			num = 0;
+next_listen:
+			++num;
+		}
+		spin_unlock(&ilb->lock);
+		rcu_read_unlock();
+
+		cond_resched();
+		diag_ctx->l_num = 0;
+	}
+
+	diag_ctx->l_num = 0;
+	diag_ctx->l_slot = i;
+}
+
 static void mptcp_diag_dump(struct sk_buff *skb, struct netlink_callback *cb,
 			    const struct inet_diag_req_v2 *r)
 {
@@ -114,6 +189,9 @@ static void mptcp_diag_dump(struct sk_buff *skb, struct netlink_callback *cb,
 		}
 		cond_resched();
 	}
+
+	if ((r->idiag_states & TCPF_LISTEN) && r->id.idiag_dport == 0)
+		mptcp_diag_dump_listeners(skb, cb, r, net_admin);
 }
 
 static void mptcp_diag_get_info(struct sock *sk, struct inet_diag_msg *r,
@@ -124,6 +202,19 @@ static void mptcp_diag_get_info(struct sock *sk, struct inet_diag_msg *r,
 
 	r->idiag_rqueue = sk_rmem_alloc_get(sk);
 	r->idiag_wqueue = sk_wmem_alloc_get(sk);
+
+	if (inet_sk_state_load(sk) == TCP_LISTEN) {
+		struct sock *lsk = READ_ONCE(msk->first);
+
+		if (lsk) {
+			/* override with settings from tcp listener,
+			 * so Send-Q will show accept queue.
+			 */
+			r->idiag_rqueue = READ_ONCE(lsk->sk_ack_backlog);
+			r->idiag_wqueue = READ_ONCE(lsk->sk_max_ack_backlog);
+		}
+	}
+
 	if (!info)
 		return;
 
-- 
2.35.1

[PATCH mptcp-next v3 4/4] selftests/mptcp: add diag listen tests

[Florian Westphal]

Check dumping of mptcp listener sockets:
1. filter by dport should not return any results
2. filter by sport should return listen sk
3. filter by saddr+sport should return listen sk
4. no filter should return listen sk

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 new in v3.

 tools/testing/selftests/net/mptcp/diag.sh | 38 +++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/tools/testing/selftests/net/mptcp/diag.sh b/tools/testing/selftests/net/mptcp/diag.sh
index ff821025d309..9dd43d7d957b 100755
--- a/tools/testing/selftests/net/mptcp/diag.sh
+++ b/tools/testing/selftests/net/mptcp/diag.sh
@@ -71,6 +71,43 @@ chk_msk_remote_key_nr()
 		__chk_nr "grep -c remote_key" $*
 }
 
+__chk_listen()
+{
+	local filter="$1"
+	local expected=$2
+
+	shift 2
+	msg=$*
+
+	nr=$(ss -N $ns -Ml "$filter" | grep -c LISTEN)
+	printf "%-50s" "$msg"
+
+	if [ $nr != $expected ]; then
+		echo "[ fail ] expected $expected found $nr"
+		ret=$test_cnt
+	else
+		echo "[  ok  ]"
+	fi
+}
+
+chk_msk_listen()
+{
+	lport=$1
+	local msg="check for listen socket"
+
+	# destination port search should always return empty list
+	__chk_listen "dport $lport" 0 "listen match for dport $lport"
+
+	# should return 'our' mptcp listen socket
+	__chk_listen "sport $lport" 1 "listen match for sport $lport"
+
+	__chk_listen "src inet:0.0.0.0:$lport" 1 "listen match for saddr and sport"
+
+	__chk_listen "" 1 "all listen sockets"
+
+	nr=$(ss -Ml $filter | wc -l)
+}
+
 # $1: ns, $2: port
 wait_local_port_listen()
 {
@@ -113,6 +150,7 @@ echo "a" | \
 				0.0.0.0 >/dev/null &
 wait_local_port_listen $ns 10000
 chk_msk_nr 0 "no msk on netns creation"
+chk_msk_listen 10000
 
 echo "b" | \
 	timeout ${timeout_test} \
-- 
2.35.1

Re: [PATCH mptcp-next v3 0/4] mptcp: inet diag listen dump support

[Mat Martineau]

On Tue, 29 Mar 2022, Florian Westphal wrote:

> Changes in v3: fix intendation, fix uninitialised var (Mat)
> 	       extend mptcp diag tests

Thanks for adding the tests. Series looks good to me:

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

>
> Changes in v2: drop patch 4 and prefer msk->first (Paolo Abeni)
>
> Done in a not-very-elegant way:
> Iterate over tcp listen hash, then pick out the tcp sockets
> with mptcp-ctx structure attached, then take the conn->sk for dumping.
>
> First patch is preparation/cleanup.
> Second patch gets rid of locking to avoid a lockdep splat.
> If the socket lock is really needed (I don't see where) I can workaround
> this by dropping locks temporarily when iterating the listen hash table,
> but its a bit more awkward.
>
> Sample output:
> ss -Mil
> State        Recv-Q Send-Q Local Address:Port  Peer Address:Port
> LISTEN       0      20     127.0.0.1:12000     0.0.0.0:*
>         subflows_max:2
>
> Florian Westphal (4):
>  mptcp: diag: switch to context structure
>  mptcp: remove locking in mptcp_diag_fill_info
>  mptcp: listen diag dump support
>  selftests/mptcp: add diag listen tests
>
> net/mptcp/mptcp_diag.c                    | 105 +++++++++++++++++++++-
> net/mptcp/sockopt.c                       |   6 --
> tools/testing/selftests/net/mptcp/diag.sh |  38 ++++++++
> 3 files changed, 140 insertions(+), 9 deletions(-)
>
> -- 
> 2.35.1
>
>
>

--
Mat Martineau
Intel

Re: [PATCH mptcp-next v3 0/4] mptcp: inet diag listen dump support

[Matthieu Baerts]

Hi Florian, Mat,

On 29/03/2022 13:35, Florian Westphal wrote:
> Changes in v3: fix intendation, fix uninitialised var (Mat)
> 	       extend mptcp diag tests
> 
> Changes in v2: drop patch 4 and prefer msk->first (Paolo Abeni)
> 
> Done in a not-very-elegant way:
> Iterate over tcp listen hash, then pick out the tcp sockets
> with mptcp-ctx structure attached, then take the conn->sk for dumping.
> 
> First patch is preparation/cleanup.
> Second patch gets rid of locking to avoid a lockdep splat.
> If the socket lock is really needed (I don't see where) I can workaround
> this by dropping locks temporarily when iterating the listen hash table,
> but its a bit more awkward.
> 
> Sample output:
> ss -Mil
> State        Recv-Q Send-Q Local Address:Port  Peer Address:Port
> LISTEN       0      20     127.0.0.1:12000     0.0.0.0:*
>          subflows_max:2

Thank you for these patches and reviews!

Now in our tree (feat. for net-next) with Mat's RvB tag:

New patches for t/upstream:
- 0432b1c61654: mptcp: diag: switch to context structure
- ba9e26b62530: mptcp: remove locking in mptcp_diag_fill_info
- b433728093dd: mptcp: listen diag dump support
- a9cc2c58bf79: selftests/mptcp: add diag listen tests
- Results: ccc95f7c14d9..3b82e3d7eed2 (export)

Builds and tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20220330T150002
https://github.com/multipath-tcp/mptcp_net-next/actions/workflows/build-validation.yml?query=branch:export

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net