* [PATCH mptcp-net v3 1/6] mptcp: fix error mibs accounting
2022-06-17 10:05 [PATCH mptcp-net v3 0/6] mptcp: mp_fail related fixes Paolo Abeni
@ 2022-06-17 10:05 ` Paolo Abeni
2022-06-17 10:05 ` [PATCH mptcp-net v3 2/6] mptcp: introduce MAPPING_BAD_CSUM Paolo Abeni
` (5 subsequent siblings)
6 siblings, 0 replies; 16+ messages in thread
From: Paolo Abeni @ 2022-06-17 10:05 UTC (permalink / raw)
To: mptcp
The current accounting for MP_FAIL and FASTCLOSE is not very
accurate: both can be increased even when the related option is
not really sent. Move the accounting into the correct place.
Fixes: eb7f33654dc1 ("mptcp: add the mibs for MP_FAIL")
Fixes: 1e75629cb964 ("mptcp: add the mibs for MP_FASTCLOSE")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/mptcp/options.c | 6 +++---
net/mptcp/pm.c | 1 -
net/mptcp/subflow.c | 4 +---
3 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index be3b918a6d15..0bfa6662447c 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -765,6 +765,7 @@ static noinline bool mptcp_established_options_rst(struct sock *sk, struct sk_bu
opts->suboptions |= OPTION_MPTCP_RST;
opts->reset_transient = subflow->reset_transient;
opts->reset_reason = subflow->reset_reason;
+ MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPRSTTX);
return true;
}
@@ -788,6 +789,7 @@ static bool mptcp_established_options_fastclose(struct sock *sk,
opts->rcvr_key = msk->remote_key;
pr_debug("FASTCLOSE key=%llu", opts->rcvr_key);
+ MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPFASTCLOSETX);
return true;
}
@@ -807,7 +809,7 @@ static bool mptcp_established_options_mp_fail(struct sock *sk,
*size = TCPOLEN_MPTCP_FAIL;
opts->suboptions |= OPTION_MPTCP_FAIL;
opts->fail_seq = subflow->map_seq;
-
+ MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPFAILTX);
pr_debug("MP_FAIL fail_seq=%llu", opts->fail_seq);
return true;
@@ -833,13 +835,11 @@ bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts)) {
*size += opt_size;
remaining -= opt_size;
- MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPFASTCLOSETX);
}
/* MP_RST can be used with MP_FASTCLOSE and MP_FAIL if there is room */
if (mptcp_established_options_rst(sk, skb, &opt_size, remaining, opts)) {
*size += opt_size;
remaining -= opt_size;
- MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPRSTTX);
}
return true;
}
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 2a57d95d5492..3c7f07bb124e 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -309,7 +309,6 @@ void mptcp_pm_mp_fail_received(struct sock *sk, u64 fail_seq)
pr_debug("send MP_FAIL response and infinite map");
subflow->send_mp_fail = 1;
- MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_MPFAILTX);
subflow->send_infinite_map = 1;
} else {
pr_debug("MP_FAIL response received");
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 5351d54e514a..57d2d8d933d0 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -958,10 +958,8 @@ static enum mapping_status validate_data_csum(struct sock *ssk, struct sk_buff *
subflow->map_data_csum);
if (unlikely(csum)) {
MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_DATACSUMERR);
- if (subflow->mp_join || subflow->valid_csum_seen) {
+ if (subflow->mp_join || subflow->valid_csum_seen)
subflow->send_mp_fail = 1;
- MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_MPFAILTX);
- }
return subflow->mp_join ? MAPPING_INVALID : MAPPING_DUMMY;
}
--
2.35.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH mptcp-net v3 2/6] mptcp: introduce MAPPING_BAD_CSUM
2022-06-17 10:05 [PATCH mptcp-net v3 0/6] mptcp: mp_fail related fixes Paolo Abeni
2022-06-17 10:05 ` [PATCH mptcp-net v3 1/6] mptcp: fix error mibs accounting Paolo Abeni
@ 2022-06-17 10:05 ` Paolo Abeni
2022-06-17 23:13 ` Mat Martineau
2022-06-17 10:05 ` [PATCH mptcp-net v3 3/6] Squash-to: "mptcp: invoke MP_FAIL response when needed" Paolo Abeni
` (4 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Paolo Abeni @ 2022-06-17 10:05 UTC (permalink / raw)
To: mptcp
This allow moving a couple of conditional out of the fast path,
making the code more easy to follow and will simplify the next
patch.
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/mptcp/subflow.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 57d2d8d933d0..98b12a9c4eb5 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -843,7 +843,8 @@ enum mapping_status {
MAPPING_INVALID,
MAPPING_EMPTY,
MAPPING_DATA_FIN,
- MAPPING_DUMMY
+ MAPPING_DUMMY,
+ MAPPING_BAD_CSUM
};
static void dbg_bad_map(struct mptcp_subflow_context *subflow, u32 ssn)
@@ -958,9 +959,7 @@ static enum mapping_status validate_data_csum(struct sock *ssk, struct sk_buff *
subflow->map_data_csum);
if (unlikely(csum)) {
MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_DATACSUMERR);
- if (subflow->mp_join || subflow->valid_csum_seen)
- subflow->send_mp_fail = 1;
- return subflow->mp_join ? MAPPING_INVALID : MAPPING_DUMMY;
+ return MAPPING_BAD_CSUM;
}
subflow->valid_csum_seen = 1;
@@ -1178,10 +1177,8 @@ static bool subflow_check_data_avail(struct sock *ssk)
status = get_mapping_status(ssk, msk);
trace_subflow_check_data_avail(status, skb_peek(&ssk->sk_receive_queue));
- if (unlikely(status == MAPPING_INVALID))
- goto fallback;
-
- if (unlikely(status == MAPPING_DUMMY))
+ if (unlikely(status == MAPPING_INVALID || status == MAPPING_DUMMY ||
+ status == MAPPING_BAD_CSUM))
goto fallback;
if (status != MAPPING_OK)
@@ -1223,7 +1220,10 @@ static bool subflow_check_data_avail(struct sock *ssk)
fallback:
if (!__mptcp_check_fallback(msk)) {
/* RFC 8684 section 3.7. */
- if (subflow->send_mp_fail) {
+ if (status == MAPPING_BAD_CSUM &&
+ ((subflow->mp_join || subflow->valid_csum_seen))) {
+ subflow->send_mp_fail = 1;
+
if (!READ_ONCE(msk->allow_infinite_fallback)) {
ssk->sk_err = EBADMSG;
tcp_set_state(ssk, TCP_CLOSE);
--
2.35.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH mptcp-net v3 2/6] mptcp: introduce MAPPING_BAD_CSUM
2022-06-17 10:05 ` [PATCH mptcp-net v3 2/6] mptcp: introduce MAPPING_BAD_CSUM Paolo Abeni
@ 2022-06-17 23:13 ` Mat Martineau
2022-06-20 9:24 ` Paolo Abeni
0 siblings, 1 reply; 16+ messages in thread
From: Mat Martineau @ 2022-06-17 23:13 UTC (permalink / raw)
To: Paolo Abeni; +Cc: mptcp
On Fri, 17 Jun 2022, Paolo Abeni wrote:
> This allow moving a couple of conditional out of the fast path,
> making the code more easy to follow and will simplify the next
> patch.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
I've had a few rounds of feedback from Jakub about the Fixes: tag in the
last few months - whether missing for -net or questionable inclusion in
net-next... So I'd like to get this right :)
Given that this is tied to the following patch ("mptcp: invoke MP_FAIL
response when needed"), and that patch "Fixes:" a patch that's only in the
5.19rc series, I don't think a missing tag here is harmful. But I'm not
sure what Jakub will want.
One option is to duplicate the "Fixes: d9fb797046c5" line from "mptcp:
invoke MP_FAIL response when needed".
What do you think Paolo?
- Mat
> ---
> net/mptcp/subflow.c | 18 +++++++++---------
> 1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 57d2d8d933d0..98b12a9c4eb5 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -843,7 +843,8 @@ enum mapping_status {
> MAPPING_INVALID,
> MAPPING_EMPTY,
> MAPPING_DATA_FIN,
> - MAPPING_DUMMY
> + MAPPING_DUMMY,
> + MAPPING_BAD_CSUM
> };
>
> static void dbg_bad_map(struct mptcp_subflow_context *subflow, u32 ssn)
> @@ -958,9 +959,7 @@ static enum mapping_status validate_data_csum(struct sock *ssk, struct sk_buff *
> subflow->map_data_csum);
> if (unlikely(csum)) {
> MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_DATACSUMERR);
> - if (subflow->mp_join || subflow->valid_csum_seen)
> - subflow->send_mp_fail = 1;
> - return subflow->mp_join ? MAPPING_INVALID : MAPPING_DUMMY;
> + return MAPPING_BAD_CSUM;
> }
>
> subflow->valid_csum_seen = 1;
> @@ -1178,10 +1177,8 @@ static bool subflow_check_data_avail(struct sock *ssk)
>
> status = get_mapping_status(ssk, msk);
> trace_subflow_check_data_avail(status, skb_peek(&ssk->sk_receive_queue));
> - if (unlikely(status == MAPPING_INVALID))
> - goto fallback;
> -
> - if (unlikely(status == MAPPING_DUMMY))
> + if (unlikely(status == MAPPING_INVALID || status == MAPPING_DUMMY ||
> + status == MAPPING_BAD_CSUM))
> goto fallback;
>
> if (status != MAPPING_OK)
> @@ -1223,7 +1220,10 @@ static bool subflow_check_data_avail(struct sock *ssk)
> fallback:
> if (!__mptcp_check_fallback(msk)) {
> /* RFC 8684 section 3.7. */
> - if (subflow->send_mp_fail) {
> + if (status == MAPPING_BAD_CSUM &&
> + ((subflow->mp_join || subflow->valid_csum_seen))) {
> + subflow->send_mp_fail = 1;
> +
> if (!READ_ONCE(msk->allow_infinite_fallback)) {
> ssk->sk_err = EBADMSG;
> tcp_set_state(ssk, TCP_CLOSE);
> --
> 2.35.3
>
>
>
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH mptcp-net v3 2/6] mptcp: introduce MAPPING_BAD_CSUM
2022-06-17 23:13 ` Mat Martineau
@ 2022-06-20 9:24 ` Paolo Abeni
0 siblings, 0 replies; 16+ messages in thread
From: Paolo Abeni @ 2022-06-20 9:24 UTC (permalink / raw)
To: Mat Martineau; +Cc: mptcp
On Fri, 2022-06-17 at 16:13 -0700, Mat Martineau wrote:
> On Fri, 17 Jun 2022, Paolo Abeni wrote:
>
> > This allow moving a couple of conditional out of the fast path,
> > making the code more easy to follow and will simplify the next
> > patch.
> >
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>
> I've had a few rounds of feedback from Jakub about the Fixes: tag in the
> last few months - whether missing for -net or questionable inclusion in
> net-next... So I'd like to get this right :)
>
> Given that this is tied to the following patch ("mptcp: invoke MP_FAIL
> response when needed"), and that patch "Fixes:" a patch that's only in the
> 5.19rc series, I don't think a missing tag here is harmful. But I'm not
> sure what Jakub will want.
>
> One option is to duplicate the "Fixes: d9fb797046c5" line from "mptcp:
> invoke MP_FAIL response when needed".
>
> What do you think Paolo?
I thought about the tagging, and I think this should be fine as is.
Thanks,
Paolo
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH mptcp-net v3 3/6] Squash-to: "mptcp: invoke MP_FAIL response when needed"
2022-06-17 10:05 [PATCH mptcp-net v3 0/6] mptcp: mp_fail related fixes Paolo Abeni
2022-06-17 10:05 ` [PATCH mptcp-net v3 1/6] mptcp: fix error mibs accounting Paolo Abeni
2022-06-17 10:05 ` [PATCH mptcp-net v3 2/6] mptcp: introduce MAPPING_BAD_CSUM Paolo Abeni
@ 2022-06-17 10:05 ` Paolo Abeni
2022-06-17 14:01 ` Paolo Abeni
2022-06-17 10:05 ` [PATCH mptcp-net v3 4/6] mptcp: fix shutdown vs fallback race Paolo Abeni
` (3 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: Paolo Abeni @ 2022-06-17 10:05 UTC (permalink / raw)
To: mptcp
This tries to address a few issues outstanding in the mentioned
patch:
- we explicitly need to reset the timeout timer for mp_fail's sake
- we need to explicitly generate a tcp ack for mp_fail, otherwise
there are no guarantees for suck option being sent out
- the timeout timer needs handling need some caring, as it's still
shared between mp_fail and msk socket timeout.
- we can re-use msk->first for msk->fail_ssk, as only the first/mpc
subflow can fail without reset. That additionally avoid the need
to clear fail_ssk on the relevant subflow close.
- fail_tout would need some additional annotation. Just to be on the
safe side move its manipulaiton under the ssk socket lock.
Last 2 paragraph of the squash to commit should be replaced with:
"""
It leverages the fact that only the MPC/first subflow can gracefully
fail to avoid unneeded subflows traversal: the failing subflow can
be only msk->first.
A new 'fail_tout' field is added to the subflow context to record the
MP_FAIL response timeout and use such field to reliably share the
timeout timer between the MP_FAIL event and the MPTCP socket close timeout.
Finally, a new ack is generated to send out MP_FAIL notification as soon
as we hit the relevant condition, instead of waiting a possibly unbound
time for the next data packet.
"""
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/mptcp/pm.c | 4 +++-
net/mptcp/protocol.c | 50 ++++++++++++++++++++++++++++++++++++--------
net/mptcp/protocol.h | 4 ++--
net/mptcp/subflow.c | 30 ++++++++++++++++++++++++--
4 files changed, 74 insertions(+), 14 deletions(-)
diff --git a/net/mptcp/pm.c b/net/mptcp/pm.c
index 3c7f07bb124e..45e2a48397b9 100644
--- a/net/mptcp/pm.c
+++ b/net/mptcp/pm.c
@@ -305,13 +305,15 @@ void mptcp_pm_mp_fail_received(struct sock *sk, u64 fail_seq)
if (!READ_ONCE(msk->allow_infinite_fallback))
return;
- if (!msk->fail_ssk) {
+ if (!subflow->fail_tout) {
pr_debug("send MP_FAIL response and infinite map");
subflow->send_mp_fail = 1;
subflow->send_infinite_map = 1;
+ tcp_send_ack(sk);
} else {
pr_debug("MP_FAIL response received");
+ WRITE_ONCE(subflow->fail_tout, 0);
}
}
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index a0f9f3831509..725fd417ebb1 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -500,7 +500,7 @@ static void mptcp_set_timeout(struct sock *sk)
__mptcp_set_timeout(sk, tout);
}
-static bool tcp_can_send_ack(const struct sock *ssk)
+static inline bool tcp_can_send_ack(const struct sock *ssk)
{
return !((1 << inet_sk_state_load(ssk)) &
(TCPF_SYN_SENT | TCPF_SYN_RECV | TCPF_TIME_WAIT | TCPF_CLOSE | TCPF_LISTEN));
@@ -2490,24 +2490,50 @@ static void __mptcp_retrans(struct sock *sk)
mptcp_reset_timer(sk);
}
+/* schedule the timeout timer for the relevant event: either close timeout
+ * or mp_fail timeout. The close timeout takes precedence on the mp_fail one
+ */
+void mptcp_reset_timeout(struct mptcp_sock *msk, unsigned long fail_tout)
+{
+ struct sock *sk = (struct sock *)msk;
+ unsigned long timeout, close_timeout;
+
+ if (!fail_tout && !sock_flag(sk, SOCK_DEAD))
+ return;
+
+ close_timeout = inet_csk(sk)->icsk_mtup.probe_timestamp - tcp_jiffies32 + jiffies + TCP_TIMEWAIT_LEN;
+
+ /* the close timeout takes precedence on the fail one, and here at least one of
+ * them is active
+ */
+ timeout = sock_flag(sk, SOCK_DEAD) ? close_timeout : fail_tout;
+
+ sk_reset_timer(sk, &sk->sk_timer, timeout);
+}
+
static void mptcp_mp_fail_no_response(struct mptcp_sock *msk)
{
- struct sock *ssk = msk->fail_ssk;
+ struct sock *ssk = msk->first;
bool slow;
+ if (!ssk)
+ return;
+
pr_debug("MP_FAIL doesn't respond, reset the subflow");
slow = lock_sock_fast(ssk);
mptcp_subflow_reset(ssk);
+ WRITE_ONCE(mptcp_subflow_ctx(ssk)->fail_tout, 0);
unlock_sock_fast(ssk, slow);
- msk->fail_ssk = NULL;
+ mptcp_reset_timeout(msk, 0);
}
static void mptcp_worker(struct work_struct *work)
{
struct mptcp_sock *msk = container_of(work, struct mptcp_sock, work);
struct sock *sk = &msk->sk.icsk_inet.sk;
+ unsigned long fail_tout;
int state;
lock_sock(sk);
@@ -2544,7 +2570,8 @@ static void mptcp_worker(struct work_struct *work)
if (test_and_clear_bit(MPTCP_WORK_RTX, &msk->flags))
__mptcp_retrans(sk);
- if (msk->fail_ssk && time_after(jiffies, msk->fail_tout))
+ fail_tout = msk->first ? READ_ONCE(mptcp_subflow_ctx(msk->first)->fail_tout) : 0;
+ if (fail_tout && time_after(jiffies, fail_tout))
mptcp_mp_fail_no_response(msk);
unlock:
@@ -2572,8 +2599,6 @@ static int __mptcp_init_sock(struct sock *sk)
WRITE_ONCE(msk->csum_enabled, mptcp_is_checksum_enabled(sock_net(sk)));
WRITE_ONCE(msk->allow_infinite_fallback, true);
msk->recovery = false;
- msk->fail_ssk = NULL;
- msk->fail_tout = 0;
mptcp_pm_data_init(msk);
@@ -2804,6 +2829,7 @@ static void __mptcp_destroy_sock(struct sock *sk)
static void mptcp_close(struct sock *sk, long timeout)
{
struct mptcp_subflow_context *subflow;
+ struct mptcp_sock *msk = mptcp_sk(sk);
bool do_cancel_work = false;
lock_sock(sk);
@@ -2822,10 +2848,16 @@ static void mptcp_close(struct sock *sk, long timeout)
cleanup:
/* orphan all the subflows */
inet_csk(sk)->icsk_mtup.probe_timestamp = tcp_jiffies32;
- mptcp_for_each_subflow(mptcp_sk(sk), subflow) {
+ mptcp_for_each_subflow(msk, subflow) {
struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
bool slow = lock_sock_fast_nested(ssk);
+ /* since the close timeout takes precedence on the fail one,
+ * cancel the latter
+ */
+ if (ssk == msk->first)
+ subflow->fail_tout = 0;
+
sock_orphan(ssk);
unlock_sock_fast(ssk, slow);
}
@@ -2834,13 +2866,13 @@ static void mptcp_close(struct sock *sk, long timeout)
sock_hold(sk);
pr_debug("msk=%p state=%d", sk, sk->sk_state);
if (mptcp_sk(sk)->token)
- mptcp_event(MPTCP_EVENT_CLOSED, mptcp_sk(sk), NULL, GFP_KERNEL);
+ mptcp_event(MPTCP_EVENT_CLOSED, msk, NULL, GFP_KERNEL);
if (sk->sk_state == TCP_CLOSE) {
__mptcp_destroy_sock(sk);
do_cancel_work = true;
} else {
- sk_reset_timer(sk, &sk->sk_timer, jiffies + TCP_TIMEWAIT_LEN);
+ mptcp_reset_timeout(msk, 0);
}
release_sock(sk);
if (do_cancel_work)
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index bef7dea9f358..077a717799a0 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -306,8 +306,6 @@ struct mptcp_sock {
u32 setsockopt_seq;
char ca_name[TCP_CA_NAME_MAX];
- struct sock *fail_ssk;
- unsigned long fail_tout;
};
#define mptcp_data_lock(sk) spin_lock_bh(&(sk)->sk_lock.slock)
@@ -484,6 +482,7 @@ struct mptcp_subflow_context {
u8 stale_count;
long delegated_status;
+ unsigned long fail_tout;
);
@@ -677,6 +676,7 @@ void mptcp_get_options(const struct sk_buff *skb,
void mptcp_finish_connect(struct sock *sk);
void __mptcp_set_connected(struct sock *sk);
+void mptcp_reset_timeout(struct mptcp_sock *msk, unsigned long fail_tout);
static inline bool mptcp_is_fully_established(struct sock *sk)
{
return inet_sk_state_load(sk) == TCP_ESTABLISHED &&
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 98b12a9c4eb5..c82a9a6e0267 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1158,6 +1158,33 @@ static bool subflow_can_fallback(struct mptcp_subflow_context *subflow)
return !subflow->fully_established;
}
+static void mptcp_subflow_fail(struct mptcp_sock *msk, struct sock *ssk)
+{
+ struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
+ unsigned long fail_tout;
+
+ /* grecefull failure can happen only on the MPC subflow */
+ if (WARN_ON_ONCE(ssk != READ_ONCE(msk->first)))
+ return;
+
+ /* since the close timeout take precedence on the fail one,
+ * no need to starte the latter when the first is already set
+ */
+ if (sock_flag((struct sock *)msk, SOCK_DEAD))
+ return;
+
+ /* we don't need extreme accuracy here, use a zero fail_tout as special
+ * value meaning no fail timeout at all;
+ */
+ fail_tout = jiffies + TCP_RTO_MAX;
+ if (!fail_tout)
+ fail_tout = 1;
+ WRITE_ONCE(subflow->fail_tout, fail_tout);
+ tcp_send_ack(ssk);
+
+ mptcp_reset_timeout(msk, subflow->fail_tout);
+}
+
static bool subflow_check_data_avail(struct sock *ssk)
{
struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
@@ -1233,8 +1260,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
while ((skb = skb_peek(&ssk->sk_receive_queue)))
sk_eat_skb(ssk, skb);
} else {
- msk->fail_ssk = ssk;
- msk->fail_tout = jiffies + TCP_RTO_MAX;
+ mptcp_subflow_fail(msk, ssk);
}
WRITE_ONCE(subflow->data_avail, MPTCP_SUBFLOW_NODATA);
return true;
--
2.35.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [PATCH mptcp-net v3 3/6] Squash-to: "mptcp: invoke MP_FAIL response when needed"
2022-06-17 10:05 ` [PATCH mptcp-net v3 3/6] Squash-to: "mptcp: invoke MP_FAIL response when needed" Paolo Abeni
@ 2022-06-17 14:01 ` Paolo Abeni
2022-06-17 23:06 ` Mat Martineau
0 siblings, 1 reply; 16+ messages in thread
From: Paolo Abeni @ 2022-06-17 14:01 UTC (permalink / raw)
To: mptcp
On Fri, 2022-06-17 at 12:05 +0200, Paolo Abeni wrote:
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 98b12a9c4eb5..c82a9a6e0267 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1158,6 +1158,33 @@ static bool subflow_can_fallback(struct mptcp_subflow_context *subflow)
> return !subflow->fully_established;
> }
>
> +static void mptcp_subflow_fail(struct mptcp_sock *msk, struct sock *ssk)
> +{
> + struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
> + unsigned long fail_tout;
> +
> + /* grecefull failure can happen only on the MPC subflow */
> + if (WARN_ON_ONCE(ssk != READ_ONCE(msk->first)))
> + return;
> +
> + /* since the close timeout take precedence on the fail one,
> + * no need to starte the latter when the first is already set
Typo here ("starte"). It would be great if it could be addressed
without reposting the whole series... A later squash-to patch could be
ok?
Thanks!
Paolo
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH mptcp-net v3 3/6] Squash-to: "mptcp: invoke MP_FAIL response when needed"
2022-06-17 14:01 ` Paolo Abeni
@ 2022-06-17 23:06 ` Mat Martineau
0 siblings, 0 replies; 16+ messages in thread
From: Mat Martineau @ 2022-06-17 23:06 UTC (permalink / raw)
To: Paolo Abeni; +Cc: mptcp
On Fri, 17 Jun 2022, Paolo Abeni wrote:
> On Fri, 2022-06-17 at 12:05 +0200, Paolo Abeni wrote:
>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>> index 98b12a9c4eb5..c82a9a6e0267 100644
>> --- a/net/mptcp/subflow.c
>> +++ b/net/mptcp/subflow.c
>> @@ -1158,6 +1158,33 @@ static bool subflow_can_fallback(struct mptcp_subflow_context *subflow)
>> return !subflow->fully_established;
>> }
>>
>> +static void mptcp_subflow_fail(struct mptcp_sock *msk, struct sock *ssk)
>> +{
>> + struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
>> + unsigned long fail_tout;
>> +
>> + /* grecefull failure can happen only on the MPC subflow */
>> + if (WARN_ON_ONCE(ssk != READ_ONCE(msk->first)))
>> + return;
>> +
>> + /* since the close timeout take precedence on the fail one,
>> + * no need to starte the latter when the first is already set
>
> Typo here ("starte"). It would be great if it could be addressed
> without reposting the whole series... A later squash-to patch could be
> ok?
>
Typo fix via squash or edit is fine by me. Also, grecefull->graceful a
few lines earlier.
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH mptcp-net v3 4/6] mptcp: fix shutdown vs fallback race
2022-06-17 10:05 [PATCH mptcp-net v3 0/6] mptcp: mp_fail related fixes Paolo Abeni
` (2 preceding siblings ...)
2022-06-17 10:05 ` [PATCH mptcp-net v3 3/6] Squash-to: "mptcp: invoke MP_FAIL response when needed" Paolo Abeni
@ 2022-06-17 10:05 ` Paolo Abeni
2022-06-17 10:05 ` [PATCH mptcp-net v3 5/6] mptcp: consistent map handling on failure Paolo Abeni
` (2 subsequent siblings)
6 siblings, 0 replies; 16+ messages in thread
From: Paolo Abeni @ 2022-06-17 10:05 UTC (permalink / raw)
To: mptcp
If the MPTCP socket shutdown happens before a fallback
to TCP, and all the pending data have been already spooled,
we never close the TCP connection.
Address the issue explicitly checking for critical condition
at fallback time.
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/mptcp/options.c | 2 +-
net/mptcp/protocol.c | 2 +-
net/mptcp/protocol.h | 19 ++++++++++++++++---
net/mptcp/subflow.c | 2 +-
4 files changed, 19 insertions(+), 6 deletions(-)
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 0bfa6662447c..57eab237c837 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -966,7 +966,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
goto reset;
subflow->mp_capable = 0;
pr_fallback(msk);
- __mptcp_do_fallback(msk);
+ mptcp_do_fallback(ssk);
return false;
}
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 725fd417ebb1..00ba9c44933a 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1245,7 +1245,7 @@ static void mptcp_update_infinite_map(struct mptcp_sock *msk,
MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_INFINITEMAPTX);
mptcp_subflow_ctx(ssk)->send_infinite_map = 0;
pr_fallback(msk);
- __mptcp_do_fallback(msk);
+ mptcp_do_fallback(ssk);
}
static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 077a717799a0..ad9b02b1b3e6 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -941,12 +941,25 @@ static inline void __mptcp_do_fallback(struct mptcp_sock *msk)
set_bit(MPTCP_FALLBACK_DONE, &msk->flags);
}
-static inline void mptcp_do_fallback(struct sock *sk)
+static inline void mptcp_do_fallback(struct sock *ssk)
{
- struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
- struct mptcp_sock *msk = mptcp_sk(subflow->conn);
+ struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
+ struct sock *sk = subflow->conn;
+ struct mptcp_sock *msk;
+ msk = mptcp_sk(sk);
__mptcp_do_fallback(msk);
+ if (READ_ONCE(msk->snd_data_fin_enable) && !(ssk->sk_shutdown & SEND_SHUTDOWN)) {
+ gfp_t saved_allocation = ssk->sk_allocation;
+
+ /* we are in a atomic (BH) scope, override ssk default for data
+ * fin allocation
+ */
+ ssk->sk_allocation = GFP_ATOMIC;
+ ssk->sk_shutdown |= SEND_SHUTDOWN;
+ tcp_shutdown(ssk, SEND_SHUTDOWN);
+ ssk->sk_allocation = saved_allocation;
+ }
}
#define pr_fallback(a) pr_debug("%s:fallback to TCP (msk=%p)", __func__, a)
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index c82a9a6e0267..b157e275315e 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1279,7 +1279,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
return false;
}
- __mptcp_do_fallback(msk);
+ mptcp_do_fallback(ssk);
}
skb = skb_peek(&ssk->sk_receive_queue);
--
2.35.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH mptcp-net v3 5/6] mptcp: consistent map handling on failure
2022-06-17 10:05 [PATCH mptcp-net v3 0/6] mptcp: mp_fail related fixes Paolo Abeni
` (3 preceding siblings ...)
2022-06-17 10:05 ` [PATCH mptcp-net v3 4/6] mptcp: fix shutdown vs fallback race Paolo Abeni
@ 2022-06-17 10:05 ` Paolo Abeni
2022-06-17 10:05 ` [PATCH mptcp-net v3 6/6] mptcp: fix race on unaccepted mptcp sockets Paolo Abeni
2022-06-17 23:05 ` [PATCH mptcp-net v3 0/6] mptcp: mp_fail related fixes Mat Martineau
6 siblings, 0 replies; 16+ messages in thread
From: Paolo Abeni @ 2022-06-17 10:05 UTC (permalink / raw)
To: mptcp
When the MPTCP receive path reach a non fatal fall-back condition, e.g.
when the MPC sockets must fall-back to TCP, the existing code is a little
self-inconsistent: it reports that new data is available - return true -
but sets the MPC flag to the opposite value.
As the consequence read operations in some exceptional scenario may block
unexpectedly.
Address the issue setting the correct MPC read status. Additionally avoid
some code duplication in the fatal fall-back scenario.
Fixes: 9c81be0dbc89 ("mptcp: add MP_FAIL response support")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/mptcp/subflow.c | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index b157e275315e..1e182301e58b 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1252,17 +1252,12 @@ static bool subflow_check_data_avail(struct sock *ssk)
subflow->send_mp_fail = 1;
if (!READ_ONCE(msk->allow_infinite_fallback)) {
- ssk->sk_err = EBADMSG;
- tcp_set_state(ssk, TCP_CLOSE);
subflow->reset_transient = 0;
subflow->reset_reason = MPTCP_RST_EMIDDLEBOX;
- tcp_send_active_reset(ssk, GFP_ATOMIC);
- while ((skb = skb_peek(&ssk->sk_receive_queue)))
- sk_eat_skb(ssk, skb);
- } else {
- mptcp_subflow_fail(msk, ssk);
+ goto reset;
}
- WRITE_ONCE(subflow->data_avail, MPTCP_SUBFLOW_NODATA);
+ mptcp_subflow_fail(msk, ssk);
+ WRITE_ONCE(subflow->data_avail, MPTCP_SUBFLOW_DATA_AVAIL);
return true;
}
@@ -1270,10 +1265,14 @@ static bool subflow_check_data_avail(struct sock *ssk)
/* fatal protocol error, close the socket.
* subflow_error_report() will introduce the appropriate barriers
*/
- ssk->sk_err = EBADMSG;
- tcp_set_state(ssk, TCP_CLOSE);
subflow->reset_transient = 0;
subflow->reset_reason = MPTCP_RST_EMPTCP;
+
+reset:
+ ssk->sk_err = EBADMSG;
+ tcp_set_state(ssk, TCP_CLOSE);
+ while ((skb = skb_peek(&ssk->sk_receive_queue)))
+ sk_eat_skb(ssk, skb);
tcp_send_active_reset(ssk, GFP_ATOMIC);
WRITE_ONCE(subflow->data_avail, MPTCP_SUBFLOW_NODATA);
return false;
--
2.35.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* [PATCH mptcp-net v3 6/6] mptcp: fix race on unaccepted mptcp sockets
2022-06-17 10:05 [PATCH mptcp-net v3 0/6] mptcp: mp_fail related fixes Paolo Abeni
` (4 preceding siblings ...)
2022-06-17 10:05 ` [PATCH mptcp-net v3 5/6] mptcp: consistent map handling on failure Paolo Abeni
@ 2022-06-17 10:05 ` Paolo Abeni
2022-06-17 11:57 ` mptcp: fix race on unaccepted mptcp sockets: Tests Results MPTCP CI
2022-06-18 0:51 ` [PATCH mptcp-net v3 6/6] mptcp: fix race on unaccepted mptcp sockets Mat Martineau
2022-06-17 23:05 ` [PATCH mptcp-net v3 0/6] mptcp: mp_fail related fixes Mat Martineau
6 siblings, 2 replies; 16+ messages in thread
From: Paolo Abeni @ 2022-06-17 10:05 UTC (permalink / raw)
To: mptcp
When the listener socket owning the relevant request is closed,
it frees the unaccepted subflows and that causes later deletion
of the paired MPTCP sockets.
The mptcp socket's worker can run in the time interval between such delete
operations. When that happens, any access to msk->first will cause an UaF
access, as the subflow cleanup did not cleared such field in the mptcp
socket.
Address the issue explictly traversing the listener socket accept
queue at close time and performing the needed cleanup on the pending
msk.
Note that the locking is a bit tricky, as we need to acquire the msk
socket lock, while still owning the subflow socket one.
Fixes: 86e39e04482b ("mptcp: keep track of local endpoint still available for each msk")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/mptcp/protocol.c | 5 +++++
net/mptcp/protocol.h | 2 ++
net/mptcp/subflow.c | 50 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 57 insertions(+)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 00ba9c44933a..6d2aa41390e7 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -2318,6 +2318,11 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
kfree_rcu(subflow, rcu);
} else {
/* otherwise tcp will dispose of the ssk and subflow ctx */
+ if (ssk->sk_state == TCP_LISTEN) {
+ tcp_set_state(ssk, TCP_CLOSE);
+ mptcp_subflow_queue_clean(ssk);
+ inet_csk_listen_stop(ssk);
+ }
__tcp_close(ssk, 0);
/* close acquired an extra ref */
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index ad9b02b1b3e6..95c9ace1437b 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -306,6 +306,7 @@ struct mptcp_sock {
u32 setsockopt_seq;
char ca_name[TCP_CA_NAME_MAX];
+ struct mptcp_sock *dl_next;
};
#define mptcp_data_lock(sk) spin_lock_bh(&(sk)->sk_lock.slock)
@@ -610,6 +611,7 @@ void mptcp_close_ssk(struct sock *sk, struct sock *ssk,
struct mptcp_subflow_context *subflow);
void mptcp_subflow_send_ack(struct sock *ssk);
void mptcp_subflow_reset(struct sock *ssk);
+void mptcp_subflow_queue_clean(struct sock *ssk);
void mptcp_sock_graft(struct sock *sk, struct socket *parent);
struct socket *__mptcp_nmpc_socket(const struct mptcp_sock *msk);
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 1e182301e58b..db83db1b3c4c 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1723,6 +1723,56 @@ static void subflow_state_change(struct sock *sk)
}
}
+void mptcp_subflow_queue_clean(struct sock *listener_ssk)
+{
+ struct request_sock_queue *queue = &inet_csk(listener_ssk)->icsk_accept_queue;
+ struct mptcp_sock *msk, *next, *head = NULL;
+ struct request_sock *req;
+
+ /* build a list of all unaccepted mptcp sockets */
+ spin_lock_bh(&queue->rskq_lock);
+ for (req = queue->rskq_accept_head; req; req = req->dl_next) {
+ struct mptcp_subflow_context *subflow;
+ struct sock *ssk = req->sk;
+ struct mptcp_sock *msk;
+
+ if (!sk_is_mptcp(ssk))
+ continue;
+
+ subflow = mptcp_subflow_ctx(ssk);
+ if (!subflow || !subflow->conn)
+ continue;
+
+ /* skip if already in list */
+ msk = mptcp_sk(subflow->conn);
+ if (msk->dl_next || msk == head)
+ continue;
+
+ msk->dl_next = head;
+ head = msk;
+ }
+ spin_unlock_bh(&queue->rskq_lock);
+ if (!head)
+ return;
+
+ /* can't acquire the msk socket lock under the subflow one,
+ * or will cause ABBA deadlock
+ */
+ release_sock(listener_ssk);
+
+ for (msk = head; msk; msk = next) {
+ struct sock *sk = (struct sock *)msk;
+ bool slow;
+
+ slow = lock_sock_fast_nested(sk);
+ next = msk->dl_next;
+ msk->first = NULL;
+ msk->dl_next = NULL;
+ unlock_sock_fast(sk, slow);
+ }
+ lock_sock(listener_ssk);
+}
+
static int subflow_ulp_init(struct sock *sk)
{
struct inet_connection_sock *icsk = inet_csk(sk);
--
2.35.3
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: mptcp: fix race on unaccepted mptcp sockets: Tests Results
2022-06-17 10:05 ` [PATCH mptcp-net v3 6/6] mptcp: fix race on unaccepted mptcp sockets Paolo Abeni
@ 2022-06-17 11:57 ` MPTCP CI
2022-06-18 0:51 ` [PATCH mptcp-net v3 6/6] mptcp: fix race on unaccepted mptcp sockets Mat Martineau
1 sibling, 0 replies; 16+ messages in thread
From: MPTCP CI @ 2022-06-17 11:57 UTC (permalink / raw)
To: Paolo Abeni; +Cc: mptcp
Hi Paolo,
Thank you for your modifications, that's great!
Our CI did some validations and here is its report:
- KVM Validation: normal:
- Success! ✅:
- Task: https://cirrus-ci.com/task/6043371033591808
- Summary: https://api.cirrus-ci.com/v1/artifact/task/6043371033591808/summary/summary.txt
- KVM Validation: debug:
- Unstable: 2 failed test(s): packetdrill_add_addr selftest_diag 🔴:
- Task: https://cirrus-ci.com/task/5480421080170496
- Summary: https://api.cirrus-ci.com/v1/artifact/task/5480421080170496/summary/summary.txt
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/aed85d6d96bd
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-debug
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 (Tessares)
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH mptcp-net v3 6/6] mptcp: fix race on unaccepted mptcp sockets
2022-06-17 10:05 ` [PATCH mptcp-net v3 6/6] mptcp: fix race on unaccepted mptcp sockets Paolo Abeni
2022-06-17 11:57 ` mptcp: fix race on unaccepted mptcp sockets: Tests Results MPTCP CI
@ 2022-06-18 0:51 ` Mat Martineau
2022-06-20 10:15 ` Paolo Abeni
1 sibling, 1 reply; 16+ messages in thread
From: Mat Martineau @ 2022-06-18 0:51 UTC (permalink / raw)
To: Paolo Abeni; +Cc: mptcp
On Fri, 17 Jun 2022, Paolo Abeni wrote:
> When the listener socket owning the relevant request is closed,
> it frees the unaccepted subflows and that causes later deletion
> of the paired MPTCP sockets.
>
> The mptcp socket's worker can run in the time interval between such delete
> operations. When that happens, any access to msk->first will cause an UaF
> access, as the subflow cleanup did not cleared such field in the mptcp
> socket.
Did you run in to this UaF in a self test? Was it possible before this
patch series?
>
> Address the issue explictly traversing the listener socket accept
> queue at close time and performing the needed cleanup on the pending
> msk.
>
> Note that the locking is a bit tricky, as we need to acquire the msk
> socket lock, while still owning the subflow socket one.
>
> Fixes: 86e39e04482b ("mptcp: keep track of local endpoint still available for each msk")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> net/mptcp/protocol.c | 5 +++++
> net/mptcp/protocol.h | 2 ++
> net/mptcp/subflow.c | 50 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 57 insertions(+)
>
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 00ba9c44933a..6d2aa41390e7 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -2318,6 +2318,11 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
> kfree_rcu(subflow, rcu);
> } else {
> /* otherwise tcp will dispose of the ssk and subflow ctx */
> + if (ssk->sk_state == TCP_LISTEN) {
> + tcp_set_state(ssk, TCP_CLOSE);
> + mptcp_subflow_queue_clean(ssk);
> + inet_csk_listen_stop(ssk);
> + }
> __tcp_close(ssk, 0);
>
> /* close acquired an extra ref */
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index ad9b02b1b3e6..95c9ace1437b 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -306,6 +306,7 @@ struct mptcp_sock {
>
> u32 setsockopt_seq;
> char ca_name[TCP_CA_NAME_MAX];
> + struct mptcp_sock *dl_next;
> };
>
> #define mptcp_data_lock(sk) spin_lock_bh(&(sk)->sk_lock.slock)
> @@ -610,6 +611,7 @@ void mptcp_close_ssk(struct sock *sk, struct sock *ssk,
> struct mptcp_subflow_context *subflow);
> void mptcp_subflow_send_ack(struct sock *ssk);
> void mptcp_subflow_reset(struct sock *ssk);
> +void mptcp_subflow_queue_clean(struct sock *ssk);
> void mptcp_sock_graft(struct sock *sk, struct socket *parent);
> struct socket *__mptcp_nmpc_socket(const struct mptcp_sock *msk);
>
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 1e182301e58b..db83db1b3c4c 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1723,6 +1723,56 @@ static void subflow_state_change(struct sock *sk)
> }
> }
>
> +void mptcp_subflow_queue_clean(struct sock *listener_ssk)
> +{
> + struct request_sock_queue *queue = &inet_csk(listener_ssk)->icsk_accept_queue;
> + struct mptcp_sock *msk, *next, *head = NULL;
> + struct request_sock *req;
> +
> + /* build a list of all unaccepted mptcp sockets */
> + spin_lock_bh(&queue->rskq_lock);
> + for (req = queue->rskq_accept_head; req; req = req->dl_next) {
> + struct mptcp_subflow_context *subflow;
> + struct sock *ssk = req->sk;
> + struct mptcp_sock *msk;
> +
> + if (!sk_is_mptcp(ssk))
> + continue;
> +
> + subflow = mptcp_subflow_ctx(ssk);
> + if (!subflow || !subflow->conn)
> + continue;
> +
> + /* skip if already in list */
> + msk = mptcp_sk(subflow->conn);
> + if (msk->dl_next || msk == head)
> + continue;
> +
> + msk->dl_next = head;
Why is it ok to read/modify msk->dl_next without the msk locked here, but
the msk lock is needed below?
Are the only msks in this queue created by incoming MP_CAPABLE SYNs? Or
could MP_JOIN subflow request socks be involved too, which would have
msk->first pointers to completely different listener ssks?
> + head = msk;
> + }
> + spin_unlock_bh(&queue->rskq_lock);
> + if (!head)
> + return;
> +
> + /* can't acquire the msk socket lock under the subflow one,
> + * or will cause ABBA deadlock
> + */
> + release_sock(listener_ssk);
> +
> + for (msk = head; msk; msk = next) {
> + struct sock *sk = (struct sock *)msk;
> + bool slow;
> +
> + slow = lock_sock_fast_nested(sk);
> + next = msk->dl_next;
> + msk->first = NULL;
Related to my question above, does it make sense to check that msk->first
== listener_ssk before setting to NULL?
> + msk->dl_next = NULL;
> + unlock_sock_fast(sk, slow);
> + }
> + lock_sock(listener_ssk);
The lock being re-acquired here was locked by the caller using:
lock_sock_nested(ssk, SINGLE_DEPTH_NESTING);
Should that notation be used here too?
> +}
> +
> static int subflow_ulp_init(struct sock *sk)
> {
> struct inet_connection_sock *icsk = inet_csk(sk);
> --
> 2.35.3
>
>
>
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH mptcp-net v3 6/6] mptcp: fix race on unaccepted mptcp sockets
2022-06-18 0:51 ` [PATCH mptcp-net v3 6/6] mptcp: fix race on unaccepted mptcp sockets Mat Martineau
@ 2022-06-20 10:15 ` Paolo Abeni
2022-06-20 10:47 ` Paolo Abeni
0 siblings, 1 reply; 16+ messages in thread
From: Paolo Abeni @ 2022-06-20 10:15 UTC (permalink / raw)
To: Mat Martineau; +Cc: mptcp
On Fri, 2022-06-17 at 17:51 -0700, Mat Martineau wrote:
> On Fri, 17 Jun 2022, Paolo Abeni wrote:
>
> > When the listener socket owning the relevant request is closed,
> > it frees the unaccepted subflows and that causes later deletion
> > of the paired MPTCP sockets.
> >
> > The mptcp socket's worker can run in the time interval between such delete
> > operations. When that happens, any access to msk->first will cause an UaF
> > access, as the subflow cleanup did not cleared such field in the mptcp
> > socket.
>
> Did you run in to this UaF in a self test?
IIRC, via packetdril
> Was it possible before this patch series?
Yes, AFAICS since the commit mentioned in the fixes tag. What really
change is the frequency. With this series it become almost
deterministic, while before this series was very hard to trigger.
> >
> > Address the issue explictly traversing the listener socket accept
> > queue at close time and performing the needed cleanup on the pending
> > msk.
> >
> > Note that the locking is a bit tricky, as we need to acquire the msk
> > socket lock, while still owning the subflow socket one.
> >
> > Fixes: 86e39e04482b ("mptcp: keep track of local endpoint still available for each msk")
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> > net/mptcp/protocol.c | 5 +++++
> > net/mptcp/protocol.h | 2 ++
> > net/mptcp/subflow.c | 50 ++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 57 insertions(+)
> >
> > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> > index 00ba9c44933a..6d2aa41390e7 100644
> > --- a/net/mptcp/protocol.c
> > +++ b/net/mptcp/protocol.c
> > @@ -2318,6 +2318,11 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
> > kfree_rcu(subflow, rcu);
> > } else {
> > /* otherwise tcp will dispose of the ssk and subflow ctx */
> > + if (ssk->sk_state == TCP_LISTEN) {
> > + tcp_set_state(ssk, TCP_CLOSE);
> > + mptcp_subflow_queue_clean(ssk);
> > + inet_csk_listen_stop(ssk);
> > + }
> > __tcp_close(ssk, 0);
> >
> > /* close acquired an extra ref */
> > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> > index ad9b02b1b3e6..95c9ace1437b 100644
> > --- a/net/mptcp/protocol.h
> > +++ b/net/mptcp/protocol.h
> > @@ -306,6 +306,7 @@ struct mptcp_sock {
> >
> > u32 setsockopt_seq;
> > char ca_name[TCP_CA_NAME_MAX];
> > + struct mptcp_sock *dl_next;
> > };
> >
> > #define mptcp_data_lock(sk) spin_lock_bh(&(sk)->sk_lock.slock)
> > @@ -610,6 +611,7 @@ void mptcp_close_ssk(struct sock *sk, struct sock *ssk,
> > struct mptcp_subflow_context *subflow);
> > void mptcp_subflow_send_ack(struct sock *ssk);
> > void mptcp_subflow_reset(struct sock *ssk);
> > +void mptcp_subflow_queue_clean(struct sock *ssk);
> > void mptcp_sock_graft(struct sock *sk, struct socket *parent);
> > struct socket *__mptcp_nmpc_socket(const struct mptcp_sock *msk);
> >
> > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > index 1e182301e58b..db83db1b3c4c 100644
> > --- a/net/mptcp/subflow.c
> > +++ b/net/mptcp/subflow.c
> > @@ -1723,6 +1723,56 @@ static void subflow_state_change(struct sock *sk)
> > }
> > }
> >
> > +void mptcp_subflow_queue_clean(struct sock *listener_ssk)
> > +{
> > + struct request_sock_queue *queue = &inet_csk(listener_ssk)->icsk_accept_queue;
> > + struct mptcp_sock *msk, *next, *head = NULL;
> > + struct request_sock *req;
> > +
> > + /* build a list of all unaccepted mptcp sockets */
> > + spin_lock_bh(&queue->rskq_lock);
> > + for (req = queue->rskq_accept_head; req; req = req->dl_next) {
> > + struct mptcp_subflow_context *subflow;
> > + struct sock *ssk = req->sk;
> > + struct mptcp_sock *msk;
> > +
> > + if (!sk_is_mptcp(ssk))
> > + continue;
> > +
> > + subflow = mptcp_subflow_ctx(ssk);
> > + if (!subflow || !subflow->conn)
> > + continue;
> > +
> > + /* skip if already in list */
> > + msk = mptcp_sk(subflow->conn);
> > + if (msk->dl_next || msk == head)
> > + continue;
> > +
> > + msk->dl_next = head;
>
> Why is it ok to read/modify msk->dl_next without the msk locked here, but
> the msk lock is needed below?
the dl_next field is protected from the listener socket lock. Such lock
is held both here and below, see the caller of this function.
>
> Are the only msks in this queue created by incoming MP_CAPABLE SYNs?
The queue contains only the subflows, and specifically the MPC
subflows. MPJ subflows never land into the request queue - the helper
inserting the req_sk there is inet_csk_reqsk_queue_add(), invoked by
tcp_check_req() via inet_csk_complete_hashdance().
> Or
> could MP_JOIN subflow request socks be involved too, which would have
> msk->first pointers to completely different listener ssks?
msk->first does not point to the listener socket, it points to the mpc
subflow (ssk, in the above code).
>
>
> > + head = msk;
> > + }
> > + spin_unlock_bh(&queue->rskq_lock);
> > + if (!head)
> > + return;
> > +
> > + /* can't acquire the msk socket lock under the subflow one,
> > + * or will cause ABBA deadlock
> > + */
> > + release_sock(listener_ssk);
> > +
> > + for (msk = head; msk; msk = next) {
> > + struct sock *sk = (struct sock *)msk;
> > + bool slow;
> > +
> > + slow = lock_sock_fast_nested(sk);
> > + next = msk->dl_next;
> > + msk->first = NULL;
>
> Related to my question above, does it make sense to check that msk->first
> == listener_ssk before setting to NULL?
should be msk->first == ssk, in the previous loop. Should be
rendundant, too. See subflow_syn_recv_sock().
>
>
> > + msk->dl_next = NULL;
> > + unlock_sock_fast(sk, slow);
> > + }
> > + lock_sock(listener_ssk);
>
> The lock being re-acquired here was locked by the caller using:
>
> lock_sock_nested(ssk, SINGLE_DEPTH_NESTING);
>
> Should that notation be used here too?
Indeed, that is needed. I'll send a v4.
Thanks!
Paolo
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH mptcp-net v3 6/6] mptcp: fix race on unaccepted mptcp sockets
2022-06-20 10:15 ` Paolo Abeni
@ 2022-06-20 10:47 ` Paolo Abeni
0 siblings, 0 replies; 16+ messages in thread
From: Paolo Abeni @ 2022-06-20 10:47 UTC (permalink / raw)
To: Mat Martineau; +Cc: mptcp
On Mon, 2022-06-20 at 12:15 +0200, Paolo Abeni wrote:
> On Fri, 2022-06-17 at 17:51 -0700, Mat Martineau wrote:
> > On Fri, 17 Jun 2022, Paolo Abeni wrote:
> > > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > > index 1e182301e58b..db83db1b3c4c 100644
> > > --- a/net/mptcp/subflow.c
> > > +++ b/net/mptcp/subflow.c
> > > @@ -1723,6 +1723,56 @@ static void subflow_state_change(struct sock *sk)
> > > }
> > > }
> > >
> > > +void mptcp_subflow_queue_clean(struct sock *listener_ssk)
> > > +{
> > > + struct request_sock_queue *queue = &inet_csk(listener_ssk)->icsk_accept_queue;
> > > + struct mptcp_sock *msk, *next, *head = NULL;
> > > + struct request_sock *req;
> > > +
> > > + /* build a list of all unaccepted mptcp sockets */
> > > + spin_lock_bh(&queue->rskq_lock);
> > > + for (req = queue->rskq_accept_head; req; req = req->dl_next) {
> > > + struct mptcp_subflow_context *subflow;
> > > + struct sock *ssk = req->sk;
> > > + struct mptcp_sock *msk;
> > > +
> > > + if (!sk_is_mptcp(ssk))
> > > + continue;
> > > +
> > > + subflow = mptcp_subflow_ctx(ssk);
> > > + if (!subflow || !subflow->conn)
> > > + continue;
> > > +
> > > + /* skip if already in list */
> > > + msk = mptcp_sk(subflow->conn);
> > > + if (msk->dl_next || msk == head)
> > > + continue;
> > > +
> > > + msk->dl_next = head;
> >
> > Why is it ok to read/modify msk->dl_next without the msk locked here, but
> > the msk lock is needed below?
>
> the dl_next field is protected from the listener socket lock. Such lock
> is held both here and below, see the caller of this function.
>
I see the above is not very clear; I mean: this whole function runs
under the listener _msk_ socket lock. The 'dl_next' list is under such
protection.
/P
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH mptcp-net v3 0/6] mptcp: mp_fail related fixes
2022-06-17 10:05 [PATCH mptcp-net v3 0/6] mptcp: mp_fail related fixes Paolo Abeni
` (5 preceding siblings ...)
2022-06-17 10:05 ` [PATCH mptcp-net v3 6/6] mptcp: fix race on unaccepted mptcp sockets Paolo Abeni
@ 2022-06-17 23:05 ` Mat Martineau
6 siblings, 0 replies; 16+ messages in thread
From: Mat Martineau @ 2022-06-17 23:05 UTC (permalink / raw)
To: Paolo Abeni; +Cc: mptcp
On Fri, 17 Jun 2022, Paolo Abeni wrote:
> While cooking a follow-up for "mptcp: invoke MP_FAIL response when
> needed" I stumbled upon a few other MP_FAIL related issues hit my
> the self-tests while mangling the above.
>
> All the patches for for the -net tree, and should be applied in-order
> around to the squashed-to patch.
>
> v2 -> v3:
> - do not schedule mp_fail timeout on closed socket in patch 3/6 (mat)
>
Thanks for the v3 Paolo. I have a (non-blocking) question for patch 2 and
a locking concern for patch 6. But for patches 1-5:
Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
> v1 -> v2:
> - take care of UaF in mptcp_worker with new patch 6/6
>
> Paolo Abeni (6):
> mptcp: fix error mibs accounting
> mptcp: introduce MAPPING_BAD_CSUM
> Squash-to: "mptcp: invoke MP_FAIL response when needed"
> mptcp: fix shutdown vs fallback race
> mptcp: consistent map handling on failure
> mptcp: fix race on unaccepted mptcp sockets
>
> net/mptcp/options.c | 8 +--
> net/mptcp/pm.c | 5 +-
> net/mptcp/protocol.c | 57 +++++++++++++++++----
> net/mptcp/protocol.h | 25 +++++++--
> net/mptcp/subflow.c | 119 ++++++++++++++++++++++++++++++++++---------
> 5 files changed, 170 insertions(+), 44 deletions(-)
>
> --
> 2.35.3
>
>
>
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 16+ messages in thread