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(-)
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.
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 <yangang@kylinos.cn>
---
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 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;
--
2.43.0
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 -- Sponsored by the NGI0 Core fund.
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 -- Sponsored by the NGI0 Core fund.
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 -- Sponsored by the NGI0 Core fund.
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)
© 2016 - 2024 Red Hat, Inc.