Merge branch 'mptcp-misc-fixes-for-v7-3-rc1'

Matthieu Baerts says: ==================== mptcp: misc fixes for v7.3-rc1 Here are various unrelated fixes: - Patch 1: Do not reschedule the RTX timer for sockets that fell back to TCP. A fix for v5.7. - Patch 2: Avoid copying thmac which will not be used and could be uninitialised. A fix for v5.7. - Patch 3: Re-set the request backup flag when SYN cookies are used. A fix for v5.9. - Patch 4: Drop pending ADD_ADDR when removing ID0, and avoid a WARN. A fix for v5.13. - Patch 5: Handle invalid suboptions where the checksum is requested in the MP_CAPABLE 4th ACK with data, but not added in the option. A fix for v5.14. - Patch 6: Prevent a race between mptcp_disconnect() and the retransmit timer. A fix for v5.17. - Patch 7: Fix a use-after-free in the selftests that could lead to false positive. A fix for v5.17. - Patch 8: Limit new addresses with the userspace PM to avoid an address ID overflow. A fix for v5.19. - Patch 9: Reset the ADD_ADDR retransmission counter when the timer is reused. A fix for v5.19. - Patch 10: Remove unneeded and confusing READ_ONCE() annotations. A fix for v6.13. - Patches 11-12: Get nstat counters for the current test, not since the creation of the netns. A fix for v6.19. - Patch 13: Fix an uninit-value in mptcp_write_data_fin for a corner case now that only a part of the tcp_out_options struct is reset. A fix for v7.1. - Patches 14-15: Two follow-up patches addressing minor comments discovered after the human review. A fix for v7.3-rc1. ====================

Jakub Kicinski committed 2 months ago

Commit 9a1599e2 parents 5be081b f01b82710 files · +78 −36Browse files

Unified · Split

10 files

net/mptcp/options.c +5 −3
net/mptcp/pm.c +1 −1
net/mptcp/pm_kernel.c +8 −0
net/mptcp/pm_userspace.c +14 −4
net/mptcp/protocol.c +27 −14
net/mptcp/protocol.h +1 −0
net/mptcp/subflow.c +0 −1
net/mptcp/syncookies.c +4 −1
tools/testing/selftests/net/mptcp/mptcp_connect.c +3 −1
tools/testing/selftests/net/mptcp/mptcp_lib.sh +15 −11

net/mptcp/options.c+5−3

 		 * In other words, the only way for checksums not to be used
 		 * is if both hosts in their SYNs set A=0."
 		 */
-		if (flags & MPTCP_CAP_CHECKSUM_REQD)
+		if ((flags & MPTCP_CAP_CHECKSUM_REQD) &&
+		    opsize < TCPOLEN_MPTCP_MPC_ACK_DATA)
 			mp_opt->suboptions |= OPTION_MPTCP_CSUMREQD;
 		mp_opt->deny_join_id0 = !!(flags & MPTCP_CAP_DENY_JOIN_ID0);
 		return false;
 	/* MPC/MPJ needed only on 3rd ack packet, DATA_FIN and TCP shutdown take precedence */
-	if (READ_ONCE(subflow->fully_established) || snd_data_fin_enable ||
+	if (subflow->fully_established || snd_data_fin_enable ||
 	    subflow->snd_isn != TCP_SKB_CB(skb)->seq ||
 	    sk->sk_state != TCP_ESTABLISHED)
 		return false;
 		ext->data_seq = data_fin_tx_seq;
 		ext->subflow_seq = 0;
 		ext->data_len = 1;
+		ext->csum = 0;
 	} else if (ext->data_seq + ext->data_len == data_fin_tx_seq) {
 		/* If there's an existing DSS mapping and it is the
 		 * final mapping, DATA_FIN consumes 1 additional byte of
 	/* here we can process OoO, in-window pkts, only in-sequence 4th ack
 	 * will make the subflow fully established
 	 */
-	if (likely(READ_ONCE(subflow->fully_established))) {
+	if (likely(subflow->fully_established)) {
 		/* on passive sockets, check for 3rd ack retransmission
 		 * note that msk is always set by subflow_syn_recv_sock()
 		 * for mp_join subflows

net/mptcp/pm.c+1−1

 	add_entry->addr = *addr;
 	add_entry->sock = msk;
-	add_entry->retrans_times = 0;
 	timer_setup(&add_entry->timer, mptcp_pm_add_addr_timer, 0);
 reset_timer:
+	add_entry->retrans_times = 0;
 	add_entry->timer_done = false;
 	timeout = mptcp_adjust_add_addr_timeout(msk);
 	if (timeout)

net/mptcp/pm_kernel.c+8−0

 	while ((msk = mptcp_token_iter_next(net, &s_slot, &s_num)) != NULL) {
 		struct sock *sk = (struct sock *)msk;
 		struct mptcp_addr_info msk_local;
+		struct mptcp_addr_info anno_addr;
+		bool announced;
 		if (list_empty(&msk->conn_list) || mptcp_pm_is_userspace(msk))
 			goto next;
 			goto next;
 		lock_sock(sk);
+		/* Drop a possibly pending ADD_ADDR for this address. */
+		anno_addr = msk_local;
+		anno_addr.port = 0;
+		announced = mptcp_pm_announced_remove(msk, &anno_addr);
 		spin_lock_bh(&msk->pm.lock);
+		if (announced)
+			msk->pm.add_addr_signaled--;
 		mptcp_pm_remove_addr(msk, &list);
 		mptcp_pm_rm_subflow(msk, &list);
 		__mark_subflow_endp_available(msk, 0);

net/mptcp/pm_userspace.c+14−4

 	}
 	if (!addr_match && !id_match) {
+		unsigned int id;
+		if (!entry->addr.id && needs_id) {
+			id = find_next_zero_bit(id_bitmap,
+						MPTCP_PM_MAX_ADDR_ID + 1, 1);
+			if (id > MPTCP_PM_MAX_ADDR_ID) {
+				ret = -ENOSPC;
+				goto append_err;
+			}
+		} else {
+			id = entry->addr.id;
+		}
 		/* Memory for the entry is allocated from the
 		 * sock option buffer.
 		 */
 			goto append_err;
 		}
-		if (!e->addr.id && needs_id)
+		e->addr.id = id;
-			e->addr.id = find_next_zero_bit(id_bitmap,
-							MPTCP_PM_MAX_ADDR_ID + 1,
-							1);
 		list_add_tail_rcu(&e->list, &msk->pm.userspace_pm_local_addr_list);
 		msk->pm.local_addr_used++;
 		ret = e->addr.id;

net/mptcp/protocol.c+27−14

 	msk->allow_subflows = false;
 	set_bit(MPTCP_FALLBACK_DONE, &msk->flags);
+	clear_bit(MPTCP_RTX_ENABLED, &msk->flags);
 	__MPTCP_INC_STATS(net, fb_mib);
 	spin_unlock_bh(&msk->fallback_lock);
 	return true;
  */
 static bool mptcp_can_ingest(const struct sock *sk)
 {
-	return unlikely(sk_rmem_alloc_get(sk) <= READ_ONCE(sk->sk_rcvbuf)) ||
+	return likely(sk_rmem_alloc_get(sk) <= READ_ONCE(sk->sk_rcvbuf)) ||
-			__mptcp_check_fallback(mptcp_sk(sk));
+	       __mptcp_check_fallback(mptcp_sk(sk));
 }
 static bool mptcp_try_rmem_schedule(struct sock *sk, const struct sk_buff *skb)
 	u64 seq, end_seq, max_seq;
 	struct sk_buff *skb1;
-	if (!mptcp_try_rmem_schedule(sk, skb)) {
-		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RCVPRUNED);
-		mptcp_drop(sk, skb);
-		return;
-	}
 	seq = MPTCP_SKB_CB(skb)->map_seq;
 	end_seq = MPTCP_SKB_CB(skb)->end_seq;
 	max_seq = atomic64_read(&msk->rcv_wnd_sent);
 		return;
 	}
+	if (!mptcp_try_rmem_schedule(sk, skb)) {
+		MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_RCVPRUNED);
+		mptcp_drop(sk, skb);
+		return;
+	}
 	p = &msk->out_of_order_queue.rb_node;
 	MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_OFOQUEUE);
 	if (RB_EMPTY_ROOT(&msk->out_of_order_queue)) {
 static void mptcp_reset_rtx_timer(struct sock *sk)
 {
+	struct mptcp_sock *msk = mptcp_sk(sk);
 	unsigned long tout;
-	/* prevent rescheduling on close */
+	/* Prevent rescheduling on close and in case of fallback. */
-	if (unlikely(inet_sk_state_load(sk) == TCP_CLOSE))
+	if (!test_bit(MPTCP_RTX_ENABLED, &msk->flags))
 		return;
-	tout = mptcp_sk(sk)->timer_ival;
+	tout = msk->timer_ival;
 	sk_reset_timer(sk, &sk->mptcp_retransmit_timer, jiffies + tout);
 }
 		 * transition from TCP_SYN_RECV to TCP_CLOSE_WAIT.
 		 */
 		break;
+	case TCP_CLOSE:
+		clear_bit(MPTCP_RTX_ENABLED, &mptcp_sk(sk)->flags);
+		fallthrough;
 	default:
 		if (oldstate == TCP_ESTABLISHED || oldstate == TCP_CLOSE_WAIT)
 			MPTCP_DEC_STATS(sock_net(sk), MPTCP_MIB_CURRESTAB);
 static int mptcp_disconnect(struct sock *sk, int flags)
 {
+	struct inet_connection_sock *icsk = inet_csk(sk);
 	struct mptcp_sock *msk = mptcp_sk(sk);
 	/* We are on the fastopen error path. We can't call straight into the
 	mptcp_check_listen_stop(sk);
 	mptcp_set_state(sk, TCP_CLOSE);
-	mptcp_stop_rtx_timer(sk);
+	/* The later subflow close can not kick again the tout timer,
-	mptcp_stop_tout_timer(sk);
+	 * as the msk is already in closed status.
+	 */
+	msk->timer_ival = icsk->icsk_rto_min;
+	sk_stop_timer_sync(sk, &sk->mptcp_retransmit_timer);
+	icsk->icsk_mtup.probe_timestamp = 0;
+	sk_stop_timer_sync(sk, &icsk->mptcp_tout_timer);
 	mptcp_pm_connection_closed(msk);
 	struct tcp_sock *tp = tcp_sk(ssk);
 	unsigned long timeout;
-	if (READ_ONCE(mptcp_subflow_ctx(ssk)->fully_established))
+	if (mptcp_subflow_ctx(ssk)->fully_established)
 		return;
 	/* reschedule with a timeout above RTT, as we must look only for drop */
 	if (IS_ERR(ssk))
 		return PTR_ERR(ssk);
+	set_bit(MPTCP_RTX_ENABLED, &msk->flags);
 	mptcp_set_state(sk, TCP_SYN_SENT);
 	subflow = mptcp_subflow_ctx(ssk);
 #ifdef CONFIG_TCP_MD5SIG
 		goto unlock;
 	}
+	set_bit(MPTCP_RTX_ENABLED, &msk->flags);
 	mptcp_set_state(sk, TCP_LISTEN);
 	sock_set_flag(sk, SOCK_RCU_FREE);

net/mptcp/protocol.h+1−0

@@ -116,6 +116,7 @@ · 115 unmodified lines
116	#define MPTCP_WORK_RTX 1	116	#define MPTCP_WORK_RTX 1
117	#define MPTCP_FALLBACK_DONE 2	117	#define MPTCP_FALLBACK_DONE 2
118	#define MPTCP_WORK_CLOSE_SUBFLOW 3	118	#define MPTCP_WORK_CLOSE_SUBFLOW 3
		119	#define MPTCP_RTX_ENABLED 4
119		120
120	/* MPTCP socket release cb flags */	121	/* MPTCP socket release cb flags */
121	#define MPTCP_PUSH_PENDING 1	122	#define MPTCP_PUSH_PENDING 1

net/mptcp/subflow.c+0−1

@@ -2084,7 +2084,6 @@ static void subflow_ulp_clone(const struct request_sock *req, · 2083 unmodified lines
2084	new_ctx->request_bkup = subflow_req->request_bkup;	2084	new_ctx->request_bkup = subflow_req->request_bkup;
2085	WRITE_ONCE(new_ctx->remote_id, subflow_req->remote_id);	2085	WRITE_ONCE(new_ctx->remote_id, subflow_req->remote_id);
2086	new_ctx->token = subflow_req->token;	2086	new_ctx->token = subflow_req->token;
2087	new_ctx->thmac = subflow_req->thmac;
2088		2087
2089	/* the subflow req id is valid, fetched via subflow_check_req()	2088	/* the subflow req id is valid, fetched via subflow_check_req()
2090	* and subflow_token_join_request()	2089	* and subflow_token_join_request()

net/mptcp/syncookies.c+4−1

 	u32 local_nonce;
 	u8 join_id;
 	u8 local_id;
-	u8 backup;
+	u8 backup:1,
+	   request_bkup:1;
 	u8 valid;
 };
 	entry->remote_nonce = subflow_req->remote_nonce;
 	entry->local_nonce = subflow_req->local_nonce;
 	entry->backup = subflow_req->backup;
+	entry->request_bkup = subflow_req->request_bkup;
 	entry->join_id = subflow_req->remote_id;
 	entry->local_id = subflow_req->local_id;
 	entry->valid = 1;
 	subflow_req->remote_nonce = e->remote_nonce;
 	subflow_req->local_nonce = e->local_nonce;
 	subflow_req->backup = e->backup;
+	subflow_req->request_bkup = e->request_bkup;
 	subflow_req->remote_id = e->join_id;
 	subflow_req->local_id = e->local_id;
 	subflow_req->token = e->token;

tools/testing/selftests/net/mptcp/mptcp_connect.c+3−1

 	hints.ai_family = pf;
+	/* Keep the resolved address alive for the whole execution: it is
+	 * used again when reconnecting, and will be released at exit time.
+	 */
 	xgetaddrinfo(remoteaddr, port, &hints, &addr);
 	for (a = addr; a; a = a->ai_next) {
 		sock = socket(a->ai_family, a->ai_socktype, proto);
 		sock = -1;
 	}
-	freeaddrinfo(addr);
 	if (sock != -1)
 		SOCK_TEST_TCPULP(sock, proto);
 	return sock;

tools/testing/selftests/net/mptcp/mptcp_lib.sh+15−11

 mptcp_lib_pr_nstat() {
 	local ns="${1}"
-	local hist="/tmp/${ns}.out"
+	local cache="/tmp/${ns}.out"
+	local hist="/tmp/${ns}.nstat"
-	if [ -f "${hist}" ]; then
+	if [ -f "${cache}" ]; then
-		awk '$2 != 0 { print "  "$0 }' "${hist}"
+		awk '$2 != 0 { print "  "$0 }' "${cache}"
 	else
-		ip netns exec "${ns}" nstat -as | grep Tcp
+		NSTAT_HISTORY="${hist}" ip netns exec "${ns}" nstat -s |
+			grep Tcp
 	fi
 }
 }
 # $1: ns, $2: MIB counter
-# Get the counter from the history (mptcp_lib_nstat_{init,get}()) if available.
+# Get the counter from the cache (mptcp_lib_nstat_{init,get}()) if available.
-# If not, get the counter from nstat ignoring any history.
+# If not, get the counter from nstat ignoring any cache, but using the history.
 mptcp_lib_get_counter() {
 	local ns="${1}"
 	local counter="${2}"
-	local hist="/tmp/${ns}.out"
+	local cache="/tmp/${ns}.out"
+	local hist="/tmp/${ns}.nstat"
 	local count
-	if [[ -s "${hist}" && "${counter}" == *"Tcp"* ]]; then
+	if [[ -s "${cache}" && "${counter}" == *"Tcp"* ]]; then
-		count=$(awk "/^${counter} / {print \$2; exit}" "${hist}")
+		count=$(awk "/^${counter} / {print \$2; exit}" "${cache}")
 	else
-		count=$(ip netns exec "${ns}" nstat -asz "${counter}" |
+		count=$(NSTAT_HISTORY="${hist}" ip netns exec "${ns}" \
-			awk 'NR==1 {next} {print $2}')
+			nstat -sz "${counter}" |
+				awk 'NR==1 {next} {print $2}')
 	fi
 	if [ -z "${count}" ]; then
 		mptcp_lib_fail_if_expected_feature "${counter} counter"