| Message ID | tencent_604F30643CD92D084CEB0AA3BE951B30C009@qq.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | 633d4f19df42421ad9d4b666c19328643fdacfc6 |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | [mptcp-next] mptcp: Add data race when accessing the fully_established of subflow | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/build | success | Build and static analysis OK |
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 82 lines checked |
| matttbe/shellcheck | success | MPTCP selftests files have not been modified |
| matttbe/KVM_Validation__normal | success | Success! ✅ |
| matttbe/KVM_Validation__debug | success | Success! ✅ |
| matttbe/KVM_Validation__btf__only_bpftest_all_ | success | Success! ✅ |
Hi YANGANG,
Thank you for your modifications, that's great!
Our CI did some validations and here is its report:
- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/10764974403
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/e6e94402fe1c
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=888189
If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:
$ cd [kernel source code]
$ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
--pull always mptcp/mptcp-upstream-virtme-docker:latest \
auto-normal
For more details:
https://github.com/multipath-tcp/mptcp-upstream-virtme-docker
Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)
Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)
Hi Gang Yan, On 09/09/2024 03:14, gang_yan@foxmail.com wrote: > From: YANGANG <yangang@kylinos.cn> > > We introduce the same handling for potential data races with the > 'fully_established' flag in subflow as previously done for > msk->fully_established. Thank you for this patch! > 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'. Indeed, bitfields cannot be used with READ/WRITE_ONCE. (I think we can remove the following text from the commit message ...) > 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)) > | > ''' (... and add a blank line here: I can fix that when applying the patch) The rest looks good to me: Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> Cheers, Matt
Hi Gang Yan, On 09/09/2024 03:14, gang_yan@foxmail.com wrote: > From: YANGANG <yangang@kylinos.cn> > > We introduce the same handling for potential data races with the > 'fully_established' flag in subflow as previously done for > msk->fully_established. Thank you for this patch! Now in our tree (feat. for net-next) with the suggested modifications from my previous review, but also a different subject, I hope that's OK: New patches for t/upstream: - 633d4f19df42: mptcp: annotate data-races around subflow->fully_established - Results: 2272658b40f1..42ffd3cd6ef4 (export) Tests are now in progress: - export: https://github.com/multipath-tcp/mptcp_net-next/commit/f1203796582877704db22e3519e514d0864a131b/checks Cheers, Matt
Hi Matt, On Mon, Sep 09, 2024 at 05:39:45PM +0200, Matthieu Baerts wrote: > Hi Gang Yan, > > On 09/09/2024 03:14, gang_yan@foxmail.com wrote: > > From: YANGANG <yangang@kylinos.cn> > > > > We introduce the same handling for potential data races with the > > 'fully_established' flag in subflow as previously done for > > msk->fully_established. > Thank you for this patch! > > Now in our tree (feat. for net-next) with the suggested modifications > from my previous review, but also a different subject, I hope that's OK: Thanks for applying this patch for me. I have an extra request. Please update my email address as 'Gang Yan <yangang@kylinos.cn>', both after 'From:' and 'Signed-off-by:'. Best wishes! Gang Yan > > New patches for t/upstream: > - 633d4f19df42: mptcp: annotate data-races around subflow->fully_established > - Results: 2272658b40f1..42ffd3cd6ef4 (export) > > Tests are now in progress: > > - export: > https://github.com/multipath-tcp/mptcp_net-next/commit/f1203796582877704db22e3519e514d0864a131b/checks > > Cheers, > Matt > -- > Sponsored by the NGI0 Core fund. > >
Hi Gang Yan, On 10/09/2024 03:26, Gang Yan wrote: > Hi Matt, > > On Mon, Sep 09, 2024 at 05:39:45PM +0200, Matthieu Baerts wrote: >> Hi Gang Yan, >> >> On 09/09/2024 03:14, gang_yan@foxmail.com wrote: >>> From: YANGANG <yangang@kylinos.cn> >>> >>> We introduce the same handling for potential data races with the >>> 'fully_established' flag in subflow as previously done for >>> msk->fully_established. >> Thank you for this patch! >> >> Now in our tree (feat. for net-next) with the suggested modifications >> from my previous review, but also a different subject, I hope that's OK: > > Thanks for applying this patch for me. I have an extra request. Please update > my email address as 'Gang Yan <yangang@kylinos.cn>', both after 'From:' and > 'Signed-off-by:'. Sure, done! New patches for t/upstream: - 3556880e322e: tg:msg: fix Gang Yan's name - Results: 7b4520aa7386..5d7105311119 (export) Tests are now in progress: - export: https://github.com/multipath-tcp/mptcp_net-next/commit/81759bc01d332f97adaf46f8de6ed7afa8a3b3c4/checks Cheers, Matt
diff --git a/net/mptcp/diag.c b/net/mptcp/diag.c index 2d3efb405437..02205f7994d7 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 66eab2cdb702..ad22622843a2 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 7ef59e17d03a..1854049f4e6a 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 d25d2dac88a5..99642d343e66 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 b9b14e75e8c2..dcaf807013f6 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;