[PATCH net v3 1/4] net: mptcp: take token lookup references with acquire ordering

[PATCH net v3 1/4] net: mptcp: take token lookup references with acquire ordering

[Hang Nan]

mptcp_token_get_sock() and mptcp_token_iter_next() take a reference on a
socket found on an RCU-protected hash chain and then re-read state to
decide whether that socket is the one they were looking for.  The
re-validation is only meaningful if it is ordered after the reference
acquisition.

The sockets come from mptcp_prot.slab, which is SLAB_TYPESAFE_BY_RCU, so
the memory a reader still points at can be freed and reallocated as a new
mptcp socket while the reader is between the chain walk and the
increment.  The new owner publishes itself in sock_init_data_uid() with

	smp_wmb();
	refcount_set(&sk->sk_refcnt, 1);

and initialises the rest of the socket only afterwards.  A reader whose
increment does not carry acquire ordering can have the re-read of
msk->token and sock_net(sk) satisfied before the increment, that is
against the old object's contents, while the increment itself succeeds on
the new one.  The lookup would then hand back a socket that belongs to a
different connection, or one that is not initialised yet.

refcount_inc_not_zero() has not provided that ordering since
commit fb041bb7c0a9 ("locking/refcount: Consolidate implementations of refcount_t"),
which removed the ordering the previous atomic_add_unless() based
implementation gave; refcount.h names this exact case as the reason the
acquire variants exist.

commit 5b5d8f351dd9 ("net: restore refcount ordering for lockless socket access")
restored the ordering for the IPv4/IPv6 established lookups,
inet_twsk_purge() and bpf_sk_assign().  These two mptcp sites have the
same shape and are converted as well.  There is no behaviour change on
strongly ordered CPUs.

Fixes: 2c5ebd001d4f ("mptcp: refactor token container")
Fixes: 96d890daad05 ("mptcp: add msk interations helper")
Cc: stable@vger.kernel.org
Signed-off-by: Hang Nan <2122295973@qq.com>
---
 net/mptcp/token.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/token.c b/net/mptcp/token.c
index e0f2823f92ef..3429b43a9f7c 100644
--- a/net/mptcp/token.c
+++ b/net/mptcp/token.c
@@ -309,7 +309,7 @@ struct mptcp_sock *mptcp_token_get_sock(struct net *net, u32 token)
 		    !net_eq(sock_net(sk), net))
 			continue;
 
-		if (!refcount_inc_not_zero(&sk->sk_refcnt))
+		if (!refcount_inc_not_zero_acquire(&sk->sk_refcnt))
 			goto not_found;
 
 		if (READ_ONCE(msk->token) != token ||
@@ -370,7 +370,7 @@ struct mptcp_sock *mptcp_token_iter_next(const struct net *net, long *s_slot,
 			if (num <= *s_num)
 				continue;
 
-			if (!refcount_inc_not_zero(&sk->sk_refcnt))
+			if (!refcount_inc_not_zero_acquire(&sk->sk_refcnt))
 				continue;
 
 			if (!net_eq(sock_net(sk), net)) {
-- 
2.47.3

[PATCH net v3 2/4] net: core: take the reuseport migration reference with acquire ordering

[Hang Nan]

reuseport_migrate_sock() returns a listener with a reference held, and
every caller immediately reads that listener: tcp_v4_rcv() and
tcp_v6_rcv() pass it to tcp_filter() and tcp_check_req(), and the
request-socket paths clone its state.  A listener removed from the
reuseport group concurrently with that lookup must not be observed in a
half-torn-down state after the reference was successfully taken.

refcount_inc_not_zero() lost its acquire ordering in commit fb041bb7c0a9
("locking/refcount: Consolidate implementations of refcount_t"), so these
subsequent reads are not ordered against the reference acquisition.  Use
refcount_inc_not_zero_acquire(), as was done for the established-socket
lookups, inet_twsk_purge() and bpf_sk_assign() in commit 5b5d8f351dd9
("net: restore refcount ordering for lockless socket access").

No behaviour change on strongly ordered architectures.

Fixes: 1cd62c21572c ("tcp: Add reuseport_migrate_sock() to select a new listener.")
Cc: stable@vger.kernel.org
Signed-off-by: Hang Nan <2122295973@qq.com>
---
 net/core/sock_reuseport.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/core/sock_reuseport.c b/net/core/sock_reuseport.c
index b9e5648fa4c5..56230aa55a3e 100644
--- a/net/core/sock_reuseport.c
+++ b/net/core/sock_reuseport.c
@@ -667,7 +667,8 @@ struct sock *reuseport_migrate_sock(struct sock *sk,
 	if (!nsk)
 		nsk = reuseport_select_sock_by_hash(reuse, hash, socks);
 
-	if (IS_ERR_OR_NULL(nsk) || unlikely(!refcount_inc_not_zero(&nsk->sk_refcnt))) {
+	if (IS_ERR_OR_NULL(nsk) ||
+	    unlikely(!refcount_inc_not_zero_acquire(&nsk->sk_refcnt))) {
 		nsk = NULL;
 		goto failure;
 	}
-- 
2.47.3

[PATCH net v3 3/4] net: mptcp: reject token 0 in mptcp_token_get_sock()

[Hang Nan]

Token 0 is never assigned to a connection: __token_bucket_busy() treats
it as busy.  It is, however, the value mptcp_token_destroy() stores into a
socket that is being torn down, and the value a freshly allocated msk
holds until it is hashed.

Both validations in mptcp_token_get_sock() compare the socket's token
with the requested one, so for a request for token 0 they cannot tell a
match apart from a socket whose token has just been cleared.  With
SLAB_TYPESAFE_BY_RCU the same is true for a socket whose memory has
already been reused by a new msk that has not been hashed yet.

The token comes from the wire: mptcp_parse_option() copies the MP_JOIN
SYN token without validating it.  A lookup for token 0 can therefore
return a dying or half-initialised msk, which subflow_token_join_request()
then hands to mptcp_pm_get_local_id() and, when the subflow uses a
different source port, to mptcp_pm_announced_has_ssk(), which walks
msk->pm.anno_list.  That list is only initialised later, from
mptcp_pm_data_init().

Reject token 0 before walking the chain.

Fixes: 2c5ebd001d4f ("mptcp: refactor token container")
Cc: stable@vger.kernel.org
Signed-off-by: Hang Nan <2122295973@qq.com>
---
 net/mptcp/token.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/mptcp/token.c b/net/mptcp/token.c
index 3429b43a9f7c..8a603fc8dc05 100644
--- a/net/mptcp/token.c
+++ b/net/mptcp/token.c
@@ -299,6 +299,10 @@ struct mptcp_sock *mptcp_token_get_sock(struct net *net, u32 token)
 	struct mptcp_sock *msk;
 	struct sock *sk;
 
+	/* Token 0 is never assigned, but is what a torn down socket holds. */
+	if (!token)
+		return NULL;
+
 	rcu_read_lock();
 	bucket = token_bucket(token);
 
-- 
2.47.3

[PATCH net v3 4/4] net: mptcp: verify the token slot in mptcp_token_iter_next()

[Hang Nan]

mptcp_token_iter_next() takes a reference on a socket found on the chain
of the slot it is walking and then re-checks only net_eq().  A socket
that was freed and whose memory has been reused by a new msk in the same
network namespace passes that check while it is still zeroed, and the
callers - mptcp_diag and the userspace path manager - then use mptcp_pm
state that has not been initialised yet.

The token of a socket on a chain always hashes to the slot being walked,
and token 0 is never assigned, so re-read the token after the acquisition
and drop the socket unless it is non-zero and still maps to this slot.

Fixes: 96d890daad05 ("mptcp: add msk interations helper")
Cc: stable@vger.kernel.org
Signed-off-by: Hang Nan <2122295973@qq.com>
---
 net/mptcp/token.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/token.c b/net/mptcp/token.c
index 8a603fc8dc05..48470d6e751d 100644
--- a/net/mptcp/token.c
+++ b/net/mptcp/token.c
@@ -355,6 +355,7 @@ struct mptcp_sock *mptcp_token_iter_next(const struct net *net, long *s_slot,
 	struct mptcp_sock *ret = NULL;
 	struct hlist_nulls_node *pos;
 	int slot, num = 0;
+	u32 token;
 
 	for (slot = *s_slot; slot <= token_mask; *s_num = 0, slot++) {
 		struct token_bucket *bucket = &token_hash[slot];
@@ -377,7 +378,9 @@ struct mptcp_sock *mptcp_token_iter_next(const struct net *net, long *s_slot,
 			if (!refcount_inc_not_zero_acquire(&sk->sk_refcnt))
 				continue;
 
-			if (!net_eq(sock_net(sk), net)) {
+			token = READ_ONCE(mptcp_sk(sk)->token);
+			if (!token || (token & token_mask) != slot ||
+			    !net_eq(sock_net(sk), net)) {
 				sock_put(sk);
 				continue;
 			}
-- 
2.47.3

Re: [PATCH net v3 2/4] net: core: take the reuseport migration reference with acquire ordering

[Hang Nan]

Thanks, agreed - the patch is dropped in v4, and the series is now three
patches: the two mptcp token patches and the iterator one, unchanged.

One correction to the reasoning: TCP sockets are allocated from a
SLAB_TYPESAFE_BY_RCU slab too (tcp_prot.slab_flags in net/ipv4/tcp_ipv4.c),
so "not a SLAB_TYPESAFE_BY_RCU lookup" is not what separates this site
from the ones in 5b5d8f351dd9 ("net: restore refcount ordering for
lockless socket access").  What separates it is the second half of your
argument, which I agree with: the acquire only orders a validation
performed after the increment, and reuseport_migrate_sock() performs
none.  It takes the reference, unlocks and returns the listener; the
callers go straight to tcp_filter() and tcp_check_req(), and the state
they actually depend on is re-checked under queue->rskq_lock in
inet_csk_reqsk_queue_add() (net/ipv4/inet_connection_sock.c:1403), which
drops the child when sk->sk_state is not TCP_LISTEN.  Ordering the
increment changes nothing about what any of those paths observe, so the
change was cosmetic and I should not have included it.

I also considered adding the missing post-acquire validation instead
(re-checking sk_state or membership in the reuseport group), but that
would duplicate the check that already exists in inet_csk_reqsk_queue_add
under the queue lock, so dropping the patch seemed the honest fix.

pw-bot: cr