| Message ID | d1935d23fff36674a64477cbf4f9b205a301624c.1621521884.git.pabeni@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | 68379f4d6133cecab05b04da890427a1891f77c1 |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | [v3,mptcp-net,1/3] mptcp: always parse mptcp options for MPC reqsk | expand |
On Thu, 20 May 2021, Paolo Abeni wrote: > When some mapping related errors occours we close the main > MPC subflow with a RST. We should instead fallback gracefully > to TCP, and do the reset only for MPJ subflows. > > Fixes: d22f4988ffec ("mptcp: process MP_CAPABLE data option") > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/192 > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- > v2 -> v3: > - change the fallback/rst test to better suite the RFC > --- > net/mptcp/subflow.c | 40 ++++++++++++++++++++-------------------- > 1 file changed, 20 insertions(+), 20 deletions(-) > > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c > index 278986585088..a39d99d96900 100644 > --- a/net/mptcp/subflow.c > +++ b/net/mptcp/subflow.c > @@ -1110,10 +1110,9 @@ 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)) { > - ssk->sk_err = EBADMSG; > - goto fatal; > - } > + if (unlikely(status == MAPPING_INVALID)) > + goto fallback; > + > if (unlikely(status == MAPPING_DUMMY)) > goto fallback; > > @@ -1128,10 +1127,8 @@ static bool subflow_check_data_avail(struct sock *ssk) > * MP_CAPABLE-based mapping > */ > if (unlikely(!READ_ONCE(msk->can_ack))) { > - if (!subflow->mpc_map) { > - ssk->sk_err = EBADMSG; > - goto fatal; > - } > + if (!subflow->mpc_map) > + goto fallback; > WRITE_ONCE(msk->remote_key, subflow->remote_key); > WRITE_ONCE(msk->ack_seq, subflow->map_seq); > WRITE_ONCE(msk->can_ack, true); > @@ -1160,19 +1157,22 @@ static bool subflow_check_data_avail(struct sock *ssk) > subflow_sched_work_if_closed(msk, ssk); > return false; > > -fatal: > - /* fatal protocol error, close the socket */ > - /* This barrier is coupled with smp_rmb() in tcp_poll() */ > - smp_wmb(); > - ssk->sk_error_report(ssk); > - tcp_set_state(ssk, TCP_CLOSE); > - subflow->reset_transient = 0; > - subflow->reset_reason = MPTCP_RST_EMPTCP; > - tcp_send_active_reset(ssk, GFP_ATOMIC); > - subflow->data_avail = 0; > - return false; > - > fallback: > + /* RFC 8684 section 3.7. */ > + if (subflow->mp_join || subflow->fully_established) { I think this is sufficient to prevent bad fallbacks, thanks for the revision. Looks good for the export branch. Packetdrill tests (including https://github.com/multipath-tcp/packetdrill/pull/58) pass for me. For the series: Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com> > + /* fatal protocol error, close the socket. > + * subflow_error_report() will introduce the apprpriate barriers Minor tweak for Matthieu when applying - "appropriate" ^^^^^^^^^^ > + */ > + ssk->sk_err = EBADMSG; > + ssk->sk_error_report(ssk); > + tcp_set_state(ssk, TCP_CLOSE); > + subflow->reset_transient = 0; > + subflow->reset_reason = MPTCP_RST_EMPTCP; > + tcp_send_active_reset(ssk, GFP_ATOMIC); > + subflow->data_avail = 0; > + return false; > + } > + > __mptcp_do_fallback(msk); > skb = skb_peek(&ssk->sk_receive_queue); > subflow->map_valid = 1; > -- > 2.26.3 > > > -- Mat Martineau Intel
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c index 278986585088..a39d99d96900 100644 --- a/net/mptcp/subflow.c +++ b/net/mptcp/subflow.c @@ -1110,10 +1110,9 @@ 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)) { - ssk->sk_err = EBADMSG; - goto fatal; - } + if (unlikely(status == MAPPING_INVALID)) + goto fallback; + if (unlikely(status == MAPPING_DUMMY)) goto fallback; @@ -1128,10 +1127,8 @@ static bool subflow_check_data_avail(struct sock *ssk) * MP_CAPABLE-based mapping */ if (unlikely(!READ_ONCE(msk->can_ack))) { - if (!subflow->mpc_map) { - ssk->sk_err = EBADMSG; - goto fatal; - } + if (!subflow->mpc_map) + goto fallback; WRITE_ONCE(msk->remote_key, subflow->remote_key); WRITE_ONCE(msk->ack_seq, subflow->map_seq); WRITE_ONCE(msk->can_ack, true); @@ -1160,19 +1157,22 @@ static bool subflow_check_data_avail(struct sock *ssk) subflow_sched_work_if_closed(msk, ssk); return false; -fatal: - /* fatal protocol error, close the socket */ - /* This barrier is coupled with smp_rmb() in tcp_poll() */ - smp_wmb(); - ssk->sk_error_report(ssk); - tcp_set_state(ssk, TCP_CLOSE); - subflow->reset_transient = 0; - subflow->reset_reason = MPTCP_RST_EMPTCP; - tcp_send_active_reset(ssk, GFP_ATOMIC); - subflow->data_avail = 0; - return false; - fallback: + /* RFC 8684 section 3.7. */ + if (subflow->mp_join || subflow->fully_established) { + /* fatal protocol error, close the socket. + * subflow_error_report() will introduce the apprpriate barriers + */ + ssk->sk_err = EBADMSG; + ssk->sk_error_report(ssk); + tcp_set_state(ssk, TCP_CLOSE); + subflow->reset_transient = 0; + subflow->reset_reason = MPTCP_RST_EMPTCP; + tcp_send_active_reset(ssk, GFP_ATOMIC); + subflow->data_avail = 0; + return false; + } + __mptcp_do_fallback(msk); skb = skb_peek(&ssk->sk_receive_queue); subflow->map_valid = 1;
When some mapping related errors occours we close the main MPC subflow with a RST. We should instead fallback gracefully to TCP, and do the reset only for MPJ subflows. Fixes: d22f4988ffec ("mptcp: process MP_CAPABLE data option") Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/192 Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- v2 -> v3: - change the fallback/rst test to better suite the RFC --- net/mptcp/subflow.c | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-)