From a0e59c48fefd0afbcc65544257a8acf882225faa Mon Sep 17 00:00:00 2001 From: YANGANG Date: Mon, 9 Sep 2024 09:14:47 +0800 Subject: [PATCH] mptcp: Add data race when accessing the fully_established of subflow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We introduce the same handling for potential data races with the 'fully_established' flag in subflow as previously done for msk->fully_established. Additionally, we make a crucial change: convert the subflow's 'fully_established' from 'bit_field' to 'bool' type. This is necessary because methods for avoiding data races don't work well with 'bit_field'. Specifically, the 'READ_ONCE' needs to know the size of the variable being accessed, which is not supported in 'bit_field'. Also, 'test_bit' expect the address of 'bit_field'. This change was prompted by compilation errors we encountered, as detailed below. ''' READ_ONCE: 477 | (sizeof(t) == sizeof(char) || \ | ^ sizeof(t) == sizeof(short)|| \ ././include/linux/compiler_types.h:490:23: note: in definition of macro ‘__compiletime_assert’ 490 | if (!(condition))\ | ^~~~~~~~~ ././include/linux/compiler_types.h:510:9: note: in expansion of macro ‘_compiletime_assert’ 510 | _compiletime_assert(condition, msg, \ __compiletime_assert_, __COUNTER__) | ^~~~~~~~~~~~~~~~~~~ ./include/asm-generic/rwonce.h:36:9: note: in expansion of macro ‘compiletime_assert’ 36 | compiletime_assert(__native_word(t) || \ sizeof(t) == sizeof(long long), \ | ^~~~~~~~~~~~~~~~~~ ./include/asm-generic/rwonce.h:36:28: note: in expansion of macro ‘__native_word’ 36 | compiletime_assert(__native_word(t) || \ | ^~~~~~~~~~~~~ sizeof(t) == sizeof(long long), \ ./include/asm-generic/rwonce.h:49:9: note: in expansion of macro ‘compiletime_assert_rwonce_type’ 49 | compiletime_assert_rwonce_type(x); \ | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ net/mptcp/protocol.h:781:39: note: in expansion of macro ‘READ_ONCE’ 781 | if (subflow->request_join && \ | !READ_ONCE(subflow->fully_established)) | ^~~~~~~~~ test_bit: error: cannot take address of bit-field 'fully_established' 50 | if (test_bit(1, &sf->fully_established)) | ''' Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/516 Signed-off-by: YANGANG --- net/mptcp/diag.c | 2 +- net/mptcp/options.c | 4 ++-- net/mptcp/protocol.c | 2 +- net/mptcp/protocol.h | 6 +++--- net/mptcp/subflow.c | 6 +++--- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/net/mptcp/diag.c b/net/mptcp/diag.c index 2d3efb405437d8..02205f7994d752 100644 --- a/net/mptcp/diag.c +++ b/net/mptcp/diag.c @@ -47,7 +47,7 @@ static int subflow_get_info(struct sock *sk, struct sk_buff *skb) flags |= MPTCP_SUBFLOW_FLAG_BKUP_REM; if (sf->request_bkup) flags |= MPTCP_SUBFLOW_FLAG_BKUP_LOC; - if (sf->fully_established) + if (READ_ONCE(sf->fully_established)) flags |= MPTCP_SUBFLOW_FLAG_FULLY_ESTABLISHED; if (sf->conn_finished) flags |= MPTCP_SUBFLOW_FLAG_CONNECTED; diff --git a/net/mptcp/options.c b/net/mptcp/options.c index 66eab2cdb702d3..ad22622843a28d 100644 --- a/net/mptcp/options.c +++ b/net/mptcp/options.c @@ -461,7 +461,7 @@ static bool mptcp_established_options_mp(struct sock *sk, struct sk_buff *skb, return false; /* MPC/MPJ needed only on 3rd ack packet, DATA_FIN and TCP shutdown take precedence */ - if (subflow->fully_established || snd_data_fin_enable || + if (READ_ONCE(subflow->fully_established) || snd_data_fin_enable || subflow->snd_isn != TCP_SKB_CB(skb)->seq || sk->sk_state != TCP_ESTABLISHED) return false; @@ -930,7 +930,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk, /* here we can process OoO, in-window pkts, only in-sequence 4th ack * will make the subflow fully established */ - if (likely(subflow->fully_established)) { + if (likely(READ_ONCE(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 diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index 7ef59e17d03a94..1854049f4e6a98 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -3493,7 +3493,7 @@ static void schedule_3rdack_retransmission(struct sock *ssk) struct tcp_sock *tp = tcp_sk(ssk); unsigned long timeout; - if (mptcp_subflow_ctx(ssk)->fully_established) + if (READ_ONCE(mptcp_subflow_ctx(ssk)->fully_established)) return; /* reschedule with a timeout above RTT, as we must look only for drop */ diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index d25d2dac88a560..99642d343e66ca 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -513,7 +513,6 @@ struct mptcp_subflow_context { request_bkup : 1, mp_capable : 1, /* remote is MPTCP capable */ mp_join : 1, /* remote is JOINing */ - fully_established : 1, /* path validated */ pm_notified : 1, /* PM hook called for established status */ conn_finished : 1, map_valid : 1, @@ -531,9 +530,10 @@ struct mptcp_subflow_context { valid_csum_seen : 1, /* at least one csum validated */ is_mptfo : 1, /* subflow is doing TFO */ close_event_done : 1, /* has done the post-closed part */ - __unused : 9; + __unused : 10; bool data_avail; bool scheduled; + bool fully_established; /* path validated */ u32 remote_nonce; u64 thmac; u32 local_nonce; @@ -778,7 +778,7 @@ static inline bool __tcp_can_send(const struct sock *ssk) static inline bool __mptcp_subflow_active(struct mptcp_subflow_context *subflow) { /* can't send if JOIN hasn't completed yet (i.e. is usable for mptcp) */ - if (subflow->request_join && !subflow->fully_established) + if (subflow->request_join && !READ_ONCE(subflow->fully_established)) return false; return __tcp_can_send(mptcp_subflow_tcp_sock(subflow)); diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c index b9b14e75e8c2ed..dcaf807013f614 100644 --- a/net/mptcp/subflow.c +++ b/net/mptcp/subflow.c @@ -785,7 +785,7 @@ void __mptcp_subflow_fully_established(struct mptcp_sock *msk, const struct mptcp_options_received *mp_opt) { subflow_set_remote_key(msk, subflow, mp_opt); - subflow->fully_established = 1; + WRITE_ONCE(subflow->fully_established, true); WRITE_ONCE(msk->fully_established, true); if (subflow->is_mptfo) @@ -1276,7 +1276,7 @@ static bool subflow_can_fallback(struct mptcp_subflow_context *subflow) else if (READ_ONCE(msk->csum_enabled)) return !subflow->valid_csum_seen; else - return !subflow->fully_established; + return !READ_ONCE(subflow->fully_established); } static void mptcp_subflow_fail(struct mptcp_sock *msk, struct sock *ssk) @@ -2045,7 +2045,7 @@ static void subflow_ulp_clone(const struct request_sock *req, } else if (subflow_req->mp_join) { new_ctx->ssn_offset = subflow_req->ssn_offset; new_ctx->mp_join = 1; - new_ctx->fully_established = 1; + WRITE_ONCE(new_ctx->fully_established, true); new_ctx->remote_key_valid = 1; new_ctx->backup = subflow_req->backup; new_ctx->request_bkup = subflow_req->request_bkup;