| Message ID | e2c96390fa3980d3aca9c41671b90f645eab6523.1727254866.git.pabeni@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Commit | 7515f1139853ea3169f6f161eb5b1f9db9dfa3d1 |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | [mptcp-net,1/2] mptcp: handle consistently DSS corruption. | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/build | success | Build and static analysis OK |
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 64 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 Paolo, On 25/09/2024 11:01, Paolo Abeni wrote: > Bugged peer implementation can send corrupted DSS options, consistently > hitting a few warning in the data path. Use DEBUG_NET assertions, to > avoid the splat on some builds and handle consistently the error, dumping > related MIBs and performing fallback and/or reset according to the > subflow type. Thank you for this fix! > > Fixes: 6771bfd9ee24 ("mptcp: update mptcp ack sequence from work queue") > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- > net/mptcp/mib.c | 2 ++ > net/mptcp/mib.h | 2 ++ > net/mptcp/protocol.c | 24 +++++++++++++++++++++--- > net/mptcp/subflow.c | 4 +++- > 4 files changed, 28 insertions(+), 4 deletions(-) > > diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c > index 38c2efc82b94..3168cde41593 100644 > --- a/net/mptcp/mib.c > +++ b/net/mptcp/mib.c > @@ -76,6 +76,8 @@ static const struct snmp_mib mptcp_snmp_list[] = { > SNMP_MIB_ITEM("RcvWndConflict", MPTCP_MIB_RCVWNDCONFLICT), > SNMP_MIB_ITEM("MPCurrEstab", MPTCP_MIB_CURRESTAB), > SNMP_MIB_ITEM("Blackhole", MPTCP_MIB_BLACKHOLE), > + SNMP_MIB_ITEM("DssCorruptionFallaback", MPTCP_MIB_DSSCORRUPTIONFALLBACK), Small typo: Fall*a*back. While at it, I suggest placing these two new entries after DSSNotMatching to ease the backports, and write "DSS" with capital letters, like the others. > + SNMP_MIB_ITEM("DssCorruptionReset", MPTCP_MIB_DSSCORRUPTIONRESET), > SNMP_MIB_SENTINEL > }; > > diff --git a/net/mptcp/mib.h b/net/mptcp/mib.h > index c8ffe18a8722..40f66556297a 100644 > --- a/net/mptcp/mib.h > +++ b/net/mptcp/mib.h > @@ -77,6 +77,8 @@ enum linux_mptcp_mib_field { > MPTCP_MIB_RCVWNDCONFLICT, /* Conflict with while updating msk rcv wnd */ > MPTCP_MIB_CURRESTAB, /* Current established MPTCP connections */ > MPTCP_MIB_BLACKHOLE, /* A blackhole has been detected */ > + MPTCP_MIB_DSSCORRUPTIONFALLBACK,/* DSS corruption detected, fallback */ > + MPTCP_MIB_DSSCORRUPTIONRESET, /* DSS corruption detected, MPJ subflow reset */ Same here: moving them after MPTCP_MIB_DSSNOMATCH to ease the backports. > __MPTCP_MIB_MAX > }; > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > index c2317919fc14..23231c758ee0 100644 > --- a/net/mptcp/protocol.c > +++ b/net/mptcp/protocol.c > @@ -620,6 +620,18 @@ static bool mptcp_check_data_fin(struct sock *sk) > return ret; > } > > +static void mptcp_dss_corruption(struct mptcp_sock *msk, struct sock *ssk) > +{ > + if (msk->first == ssk) { Is this condition enough to do a fallback? Should we not do a fallback only if there are no other subflows? Or enough to consider only having the first subflow: list_is_singular(&msk->conn_list) && msk->first == ssk > + MPTCP_INC_STATS(sock_net(ssk), > + MPTCP_MIB_DSSCORRUPTIONFALLBACK); > + mptcp_do_fallback(ssk); > + } else { > + MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_DSSCORRUPTIONRESET); > + mptcp_subflow_reset(ssk); > + } > +} > + > static bool __mptcp_move_skbs_from_subflow(struct mptcp_sock *msk, > struct sock *ssk, > unsigned int *bytes) The rest of this patch and patch 2/2 look good to me! Cheers, Matt
diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c index 38c2efc82b94..3168cde41593 100644 --- a/net/mptcp/mib.c +++ b/net/mptcp/mib.c @@ -76,6 +76,8 @@ static const struct snmp_mib mptcp_snmp_list[] = { SNMP_MIB_ITEM("RcvWndConflict", MPTCP_MIB_RCVWNDCONFLICT), SNMP_MIB_ITEM("MPCurrEstab", MPTCP_MIB_CURRESTAB), SNMP_MIB_ITEM("Blackhole", MPTCP_MIB_BLACKHOLE), + SNMP_MIB_ITEM("DssCorruptionFallaback", MPTCP_MIB_DSSCORRUPTIONFALLBACK), + SNMP_MIB_ITEM("DssCorruptionReset", MPTCP_MIB_DSSCORRUPTIONRESET), SNMP_MIB_SENTINEL }; diff --git a/net/mptcp/mib.h b/net/mptcp/mib.h index c8ffe18a8722..40f66556297a 100644 --- a/net/mptcp/mib.h +++ b/net/mptcp/mib.h @@ -77,6 +77,8 @@ enum linux_mptcp_mib_field { MPTCP_MIB_RCVWNDCONFLICT, /* Conflict with while updating msk rcv wnd */ MPTCP_MIB_CURRESTAB, /* Current established MPTCP connections */ MPTCP_MIB_BLACKHOLE, /* A blackhole has been detected */ + MPTCP_MIB_DSSCORRUPTIONFALLBACK,/* DSS corruption detected, fallback */ + MPTCP_MIB_DSSCORRUPTIONRESET, /* DSS corruption detected, MPJ subflow reset */ __MPTCP_MIB_MAX }; diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index c2317919fc14..23231c758ee0 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -620,6 +620,18 @@ static bool mptcp_check_data_fin(struct sock *sk) return ret; } +static void mptcp_dss_corruption(struct mptcp_sock *msk, struct sock *ssk) +{ + if (msk->first == ssk) { + MPTCP_INC_STATS(sock_net(ssk), + MPTCP_MIB_DSSCORRUPTIONFALLBACK); + mptcp_do_fallback(ssk); + } else { + MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_DSSCORRUPTIONRESET); + mptcp_subflow_reset(ssk); + } +} + static bool __mptcp_move_skbs_from_subflow(struct mptcp_sock *msk, struct sock *ssk, unsigned int *bytes) @@ -692,10 +704,16 @@ static bool __mptcp_move_skbs_from_subflow(struct mptcp_sock *msk, moved += len; seq += len; - if (WARN_ON_ONCE(map_remaining < len)) - break; + if (unlikely(map_remaining < len)) { + DEBUG_NET_WARN_ON_ONCE(1); + mptcp_dss_corruption(msk, ssk); + } } else { - WARN_ON_ONCE(!fin); + if (unlikely(!fin)) { + DEBUG_NET_WARN_ON_ONCE(1); + mptcp_dss_corruption(msk, ssk); + } + sk_eat_skb(ssk, skb); done = true; } diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c index 1040b3b9696b..e1046a696ab5 100644 --- a/net/mptcp/subflow.c +++ b/net/mptcp/subflow.c @@ -975,8 +975,10 @@ static bool skb_is_fully_mapped(struct sock *ssk, struct sk_buff *skb) unsigned int skb_consumed; skb_consumed = tcp_sk(ssk)->copied_seq - TCP_SKB_CB(skb)->seq; - if (WARN_ON_ONCE(skb_consumed >= skb->len)) + if (unlikely(skb_consumed >= skb->len)) { + DEBUG_NET_WARN_ON_ONCE(1); return true; + } return skb->len - skb_consumed <= subflow->map_data_len - mptcp_subflow_get_map_offset(subflow);
Bugged peer implementation can send corrupted DSS options, consistently hitting a few warning in the data path. Use DEBUG_NET assertions, to avoid the splat on some builds and handle consistently the error, dumping related MIBs and performing fallback and/or reset according to the subflow type. Fixes: 6771bfd9ee24 ("mptcp: update mptcp ack sequence from work queue") Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- net/mptcp/mib.c | 2 ++ net/mptcp/mib.h | 2 ++ net/mptcp/protocol.c | 24 +++++++++++++++++++++--- net/mptcp/subflow.c | 4 +++- 4 files changed, 28 insertions(+), 4 deletions(-)