| Message ID | 20220112221523.1829397-5-kishen.maloor@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | mptcp: support userspace path management | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 34 lines checked |
| matttbe/build | success | Build and static analysis OK |
| matttbe/KVM_Validation__normal | warning | Unstable: 2 failed test(s): packetdrill_add_addr selftest_mptcp_join |
On Wed, 12 Jan 2022, Kishen Maloor wrote: > This change updates internal logic to permit subflows to be > established from either the client or server ends of MPTCP > connections. This symmetry and added flexibility may be > harnessed by PM implementations running on either end in > creating new subflows. > > The essence of this change lies in not relying on the > "server_side" flag (which continues to be available if needed). > After this patch, server_side is unused except for some debug output. If it's really no longer referenced (see below first), may be better to just remove it. > v2: check for 3rd ACK retransmission only on passive side > of the MPJ handshake > > Signed-off-by: Kishen Maloor <kishen.maloor@intel.com> > --- > net/mptcp/options.c | 2 +- > net/mptcp/protocol.c | 5 +---- > net/mptcp/protocol.h | 2 -- > 3 files changed, 2 insertions(+), 7 deletions(-) > > diff --git a/net/mptcp/options.c b/net/mptcp/options.c > index cceba8c7806d..89808816db06 100644 > --- a/net/mptcp/options.c > +++ b/net/mptcp/options.c > @@ -921,7 +921,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk, > if (TCP_SKB_CB(skb)->seq == subflow->ssn_offset + 1 && > TCP_SKB_CB(skb)->end_seq == TCP_SKB_CB(skb)->seq && > subflow->mp_join && (mp_opt->suboptions & OPTIONS_MPTCP_MPJ) && > - READ_ONCE(msk->pm.server_side)) > + !subflow->request_join) > tcp_send_ack(ssk); > goto fully_established; > } > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > index 4012f844eec1..408a05bff633 100644 > --- a/net/mptcp/protocol.c > +++ b/net/mptcp/protocol.c > @@ -3248,15 +3248,12 @@ bool mptcp_finish_join(struct sock *ssk) > return false; > } > > - if (!msk->pm.server_side) > + if (!list_empty(&subflow->node)) > goto out; > > if (!mptcp_pm_allow_new_subflow(msk)) > goto err_prohibited; > > - if (WARN_ON_ONCE(!list_empty(&subflow->node))) > - goto err_prohibited; > - > /* active connections are already on conn_list. > * If we can't acquire msk socket lock here, let the release callback > * handle it > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h > index e2a67d3469f6..c50247673c7e 100644 > --- a/net/mptcp/protocol.h > +++ b/net/mptcp/protocol.h > @@ -908,10 +908,8 @@ static inline bool mptcp_check_infinite_map(struct sk_buff *skb) > static inline bool subflow_simultaneous_connect(struct sock *sk) > { > struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk); > - struct sock *parent = subflow->conn; > > return sk->sk_state == TCP_ESTABLISHED && > - !mptcp_sk(parent)->pm.server_side && > !subflow->conn_finished; > } Are changes in this function needed? This code was added to address https://github.com/multipath-tcp/mptcp_net-next/issues/35 which was a weird case encountered with syzkaller where a socket tried to connect to itself, triggering the "simultaneous open" code path. It doesn't seem to be applicable to MP_JOINs. -- Mat Martineau Intel
On Fri, 2022-01-14 at 14:43 -0800, Mat Martineau wrote: > On Wed, 12 Jan 2022, Kishen Maloor wrote: > > > This change updates internal logic to permit subflows to be > > established from either the client or server ends of MPTCP > > connections. This symmetry and added flexibility may be > > harnessed by PM implementations running on either end in > > creating new subflows. > > > > The essence of this change lies in not relying on the > > "server_side" flag (which continues to be available if needed). > > > > After this patch, server_side is unused except for some debug output. If > it's really no longer referenced (see below first), may be better to just > remove it. > > > v2: check for 3rd ACK retransmission only on passive side > > of the MPJ handshake > > > > Signed-off-by: Kishen Maloor <kishen.maloor@intel.com> > > --- > > net/mptcp/options.c | 2 +- > > net/mptcp/protocol.c | 5 +---- > > net/mptcp/protocol.h | 2 -- > > 3 files changed, 2 insertions(+), 7 deletions(-) > > > > diff --git a/net/mptcp/options.c b/net/mptcp/options.c > > index cceba8c7806d..89808816db06 100644 > > --- a/net/mptcp/options.c > > +++ b/net/mptcp/options.c > > @@ -921,7 +921,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk, > > if (TCP_SKB_CB(skb)->seq == subflow->ssn_offset + 1 && > > TCP_SKB_CB(skb)->end_seq == TCP_SKB_CB(skb)->seq && > > subflow->mp_join && (mp_opt->suboptions & OPTIONS_MPTCP_MPJ) && > > - READ_ONCE(msk->pm.server_side)) > > + !subflow->request_join) > > tcp_send_ack(ssk); > > goto fully_established; > > } > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > > index 4012f844eec1..408a05bff633 100644 > > --- a/net/mptcp/protocol.c > > +++ b/net/mptcp/protocol.c > > @@ -3248,15 +3248,12 @@ bool mptcp_finish_join(struct sock *ssk) > > return false; > > } > > > > - if (!msk->pm.server_side) > > + if (!list_empty(&subflow->node)) > > goto out; > > > > if (!mptcp_pm_allow_new_subflow(msk)) > > goto err_prohibited; > > > > - if (WARN_ON_ONCE(!list_empty(&subflow->node))) > > - goto err_prohibited; > > - > > /* active connections are already on conn_list. > > * If we can't acquire msk socket lock here, let the release callback > > * handle it > > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h > > index e2a67d3469f6..c50247673c7e 100644 > > --- a/net/mptcp/protocol.h > > +++ b/net/mptcp/protocol.h > > @@ -908,10 +908,8 @@ static inline bool mptcp_check_infinite_map(struct sk_buff *skb) > > static inline bool subflow_simultaneous_connect(struct sock *sk) > > { > > struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk); > > - struct sock *parent = subflow->conn; > > > > return sk->sk_state == TCP_ESTABLISHED && > > - !mptcp_sk(parent)->pm.server_side && > > !subflow->conn_finished; > > } > > Are changes in this function needed? This code was added to address > https://github.com/multipath-tcp/mptcp_net-next/issues/35 > which was a weird case encountered with syzkaller where a socket tried to > connect to itself, triggering the "simultaneous open" code path. It > doesn't seem to be applicable to MP_JOINs. I *think* we can reach here even with MPJ, e.g. if we have port-based endpoint matching the src address/port - possibly syzkaller could be able to reach such scenario. Additionally I fear dropping the '!mptcp_sk(parent)->pm.server_side &&' check could re-introduce the initial issue, at least for the MPC handshake. /P
On 1/17/22 12:59 AM, Paolo Abeni wrote: > On Fri, 2022-01-14 at 14:43 -0800, Mat Martineau wrote: >> On Wed, 12 Jan 2022, Kishen Maloor wrote: >> >>> This change updates internal logic to permit subflows to be >>> established from either the client or server ends of MPTCP >>> connections. This symmetry and added flexibility may be >>> harnessed by PM implementations running on either end in >>> creating new subflows. >>> >>> The essence of this change lies in not relying on the >>> "server_side" flag (which continues to be available if needed). >>> >> >> After this patch, server_side is unused except for some debug output. If >> it's really no longer referenced (see below first), may be better to just >> remove it. I think we'll need it. One possible use is to inform the PM daemon of the type of application (client/server) associated with a connection. As we're discussing the idea of creating listening sockets only upon request, the knowledge that we are on the client end of a connection is something a PM daemon could use to request creation of listening sockets whenever it advertises an address. >> >>> v2: check for 3rd ACK retransmission only on passive side >>> of the MPJ handshake >>> >>> Signed-off-by: Kishen Maloor <kishen.maloor@intel.com> >>> --- >>> net/mptcp/options.c | 2 +- >>> net/mptcp/protocol.c | 5 +---- >>> net/mptcp/protocol.h | 2 -- >>> 3 files changed, 2 insertions(+), 7 deletions(-) >>> >>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c >>> index cceba8c7806d..89808816db06 100644 >>> --- a/net/mptcp/options.c >>> +++ b/net/mptcp/options.c >>> @@ -921,7 +921,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk, >>> if (TCP_SKB_CB(skb)->seq == subflow->ssn_offset + 1 && >>> TCP_SKB_CB(skb)->end_seq == TCP_SKB_CB(skb)->seq && >>> subflow->mp_join && (mp_opt->suboptions & OPTIONS_MPTCP_MPJ) && >>> - READ_ONCE(msk->pm.server_side)) >>> + !subflow->request_join) >>> tcp_send_ack(ssk); >>> goto fully_established; >>> } >>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c >>> index 4012f844eec1..408a05bff633 100644 >>> --- a/net/mptcp/protocol.c >>> +++ b/net/mptcp/protocol.c >>> @@ -3248,15 +3248,12 @@ bool mptcp_finish_join(struct sock *ssk) >>> return false; >>> } >>> >>> - if (!msk->pm.server_side) >>> + if (!list_empty(&subflow->node)) >>> goto out; >>> >>> if (!mptcp_pm_allow_new_subflow(msk)) >>> goto err_prohibited; >>> >>> - if (WARN_ON_ONCE(!list_empty(&subflow->node))) >>> - goto err_prohibited; >>> - >>> /* active connections are already on conn_list. >>> * If we can't acquire msk socket lock here, let the release callback >>> * handle it >>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h >>> index e2a67d3469f6..c50247673c7e 100644 >>> --- a/net/mptcp/protocol.h >>> +++ b/net/mptcp/protocol.h >>> @@ -908,10 +908,8 @@ static inline bool mptcp_check_infinite_map(struct sk_buff *skb) >>> static inline bool subflow_simultaneous_connect(struct sock *sk) >>> { >>> struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk); >>> - struct sock *parent = subflow->conn; >>> >>> return sk->sk_state == TCP_ESTABLISHED && >>> - !mptcp_sk(parent)->pm.server_side && >>> !subflow->conn_finished; >>> } >> >> Are changes in this function needed? This code was added to address >> https://github.com/multipath-tcp/mptcp_net-next/issues/35 >> which was a weird case encountered with syzkaller where a socket tried to >> connect to itself, triggering the "simultaneous open" code path. It >> doesn't seem to be applicable to MP_JOINs. > > I *think* we can reach here even with MPJ, e.g. if we have port-based > endpoint matching the src address/port - possibly syzkaller could be > able to reach such scenario. > > Additionally I fear dropping the '!mptcp_sk(parent)->pm.server_side &&' > check could re-introduce the initial issue, at least for the MPC > handshake. The purpose of this commit is to permit the MPJ sequence to happen at either end of the connection. The change in mptcp_finish_join() had the most direct impact for enabling this, so I looked at other server_side checks mostly in terms of establishing that bi-directional symmetry. If the above condition holds only with MPCs, i.e. for the initial connection, then I think it would be fine to preserve the server_side check. But if it also holds for MPJs, then we might have to account for both cases. Perhaps replace: !mptcp_sk(parent)->pm.server_side) with something like this? ((subflow->request_mptcp && !mptcp_sk(parent)->pm.server_side) || subflow->request_join) > > /P >
On Tue, 2022-01-18 at 17:26 -0800, Kishen Maloor wrote: > On 1/17/22 12:59 AM, Paolo Abeni wrote: > > On Fri, 2022-01-14 at 14:43 -0800, Mat Martineau wrote: > > > On Wed, 12 Jan 2022, Kishen Maloor wrote: > > > > > > > This change updates internal logic to permit subflows to be > > > > established from either the client or server ends of MPTCP > > > > connections. This symmetry and added flexibility may be > > > > harnessed by PM implementations running on either end in > > > > creating new subflows. > > > > > > > > The essence of this change lies in not relying on the > > > > "server_side" flag (which continues to be available if needed). > > > > > > > > > > After this patch, server_side is unused except for some debug output. If > > > it's really no longer referenced (see below first), may be better to just > > > remove it. > > I think we'll need it. > > One possible use is to inform the PM daemon of the type of application > (client/server) associated with a connection. > > As we're discussing the idea of creating listening sockets only upon request, the > knowledge that we are on the client end of a connection is something a PM daemon could > use to request creation of listening sockets whenever it advertises an address. > > > > > > > > v2: check for 3rd ACK retransmission only on passive side > > > > of the MPJ handshake > > > > > > > > Signed-off-by: Kishen Maloor <kishen.maloor@intel.com> > > > > --- > > > > net/mptcp/options.c | 2 +- > > > > net/mptcp/protocol.c | 5 +---- > > > > net/mptcp/protocol.h | 2 -- > > > > 3 files changed, 2 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/net/mptcp/options.c b/net/mptcp/options.c > > > > index cceba8c7806d..89808816db06 100644 > > > > --- a/net/mptcp/options.c > > > > +++ b/net/mptcp/options.c > > > > @@ -921,7 +921,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk, > > > > if (TCP_SKB_CB(skb)->seq == subflow->ssn_offset + 1 && > > > > TCP_SKB_CB(skb)->end_seq == TCP_SKB_CB(skb)->seq && > > > > subflow->mp_join && (mp_opt->suboptions & OPTIONS_MPTCP_MPJ) && > > > > - READ_ONCE(msk->pm.server_side)) > > > > + !subflow->request_join) > > > > tcp_send_ack(ssk); > > > > goto fully_established; > > > > } > > > > diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c > > > > index 4012f844eec1..408a05bff633 100644 > > > > --- a/net/mptcp/protocol.c > > > > +++ b/net/mptcp/protocol.c > > > > @@ -3248,15 +3248,12 @@ bool mptcp_finish_join(struct sock *ssk) > > > > return false; > > > > } > > > > > > > > - if (!msk->pm.server_side) > > > > + if (!list_empty(&subflow->node)) > > > > goto out; > > > > > > > > if (!mptcp_pm_allow_new_subflow(msk)) > > > > goto err_prohibited; > > > > > > > > - if (WARN_ON_ONCE(!list_empty(&subflow->node))) > > > > - goto err_prohibited; > > > > - > > > > /* active connections are already on conn_list. > > > > * If we can't acquire msk socket lock here, let the release callback > > > > * handle it > > > > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h > > > > index e2a67d3469f6..c50247673c7e 100644 > > > > --- a/net/mptcp/protocol.h > > > > +++ b/net/mptcp/protocol.h > > > > @@ -908,10 +908,8 @@ static inline bool mptcp_check_infinite_map(struct sk_buff *skb) > > > > static inline bool subflow_simultaneous_connect(struct sock *sk) > > > > { > > > > struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk); > > > > - struct sock *parent = subflow->conn; > > > > > > > > return sk->sk_state == TCP_ESTABLISHED && > > > > - !mptcp_sk(parent)->pm.server_side && > > > > !subflow->conn_finished; > > > > } > > > > > > Are changes in this function needed? This code was added to address > > > https://github.com/multipath-tcp/mptcp_net-next/issues/35 > > > which was a weird case encountered with syzkaller where a socket tried to > > > connect to itself, triggering the "simultaneous open" code path. It > > > doesn't seem to be applicable to MP_JOINs. > > > > I *think* we can reach here even with MPJ, e.g. if we have port-based > > endpoint matching the src address/port - possibly syzkaller could be > > able to reach such scenario. > > > > Additionally I fear dropping the '!mptcp_sk(parent)->pm.server_side &&' > > check could re-introduce the initial issue, at least for the MPC > > handshake. > > The purpose of this commit is to permit the MPJ sequence to happen at either end of > the connection. The change in mptcp_finish_join() had the most direct impact for enabling > this, so I looked at other server_side checks mostly in terms of establishing that > bi-directional symmetry. > > If the above condition holds only with MPCs, i.e. for the initial connection, then I think it would > be fine to preserve the server_side check. But if it also holds for MPJs, then we might > have to account for both cases. Perhaps replace: > > !mptcp_sk(parent)->pm.server_side) > > with something like this? > > ((subflow->request_mptcp && !mptcp_sk(parent)->pm.server_side) || > subflow->request_join) It looks like 'request_mptcp' is 0 on passive sockets, so: subflow->request_mptcp || subflow->request_join should suffice. Possibly factoring out an helper for the above condition would make the code more readable. /P
On 1/19/22 4:01 AM, Paolo Abeni wrote: > On Tue, 2022-01-18 at 17:26 -0800, Kishen Maloor wrote: >> On 1/17/22 12:59 AM, Paolo Abeni wrote: >>> On Fri, 2022-01-14 at 14:43 -0800, Mat Martineau wrote: >>>> On Wed, 12 Jan 2022, Kishen Maloor wrote: >>>> >>>>> This change updates internal logic to permit subflows to be >>>>> established from either the client or server ends of MPTCP >>>>> connections. This symmetry and added flexibility may be >>>>> harnessed by PM implementations running on either end in >>>>> creating new subflows. >>>>> >>>>> The essence of this change lies in not relying on the >>>>> "server_side" flag (which continues to be available if needed). >>>>> >>>> >>>> After this patch, server_side is unused except for some debug output. If >>>> it's really no longer referenced (see below first), may be better to just >>>> remove it. >> >> I think we'll need it. >> >> One possible use is to inform the PM daemon of the type of application >> (client/server) associated with a connection. >> >> As we're discussing the idea of creating listening sockets only upon request, the >> knowledge that we are on the client end of a connection is something a PM daemon could >> use to request creation of listening sockets whenever it advertises an address. >> >>>> >>>>> v2: check for 3rd ACK retransmission only on passive side >>>>> of the MPJ handshake >>>>> >>>>> Signed-off-by: Kishen Maloor <kishen.maloor@intel.com> >>>>> --- >>>>> net/mptcp/options.c | 2 +- >>>>> net/mptcp/protocol.c | 5 +---- >>>>> net/mptcp/protocol.h | 2 -- >>>>> 3 files changed, 2 insertions(+), 7 deletions(-) >>>>> >>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c >>>>> index cceba8c7806d..89808816db06 100644 >>>>> --- a/net/mptcp/options.c >>>>> +++ b/net/mptcp/options.c >>>>> @@ -921,7 +921,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk, >>>>> if (TCP_SKB_CB(skb)->seq == subflow->ssn_offset + 1 && >>>>> TCP_SKB_CB(skb)->end_seq == TCP_SKB_CB(skb)->seq && >>>>> subflow->mp_join && (mp_opt->suboptions & OPTIONS_MPTCP_MPJ) && >>>>> - READ_ONCE(msk->pm.server_side)) >>>>> + !subflow->request_join) >>>>> tcp_send_ack(ssk); >>>>> goto fully_established; >>>>> } >>>>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c >>>>> index 4012f844eec1..408a05bff633 100644 >>>>> --- a/net/mptcp/protocol.c >>>>> +++ b/net/mptcp/protocol.c >>>>> @@ -3248,15 +3248,12 @@ bool mptcp_finish_join(struct sock *ssk) >>>>> return false; >>>>> } >>>>> >>>>> - if (!msk->pm.server_side) >>>>> + if (!list_empty(&subflow->node)) >>>>> goto out; >>>>> >>>>> if (!mptcp_pm_allow_new_subflow(msk)) >>>>> goto err_prohibited; >>>>> >>>>> - if (WARN_ON_ONCE(!list_empty(&subflow->node))) >>>>> - goto err_prohibited; >>>>> - >>>>> /* active connections are already on conn_list. >>>>> * If we can't acquire msk socket lock here, let the release callback >>>>> * handle it >>>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h >>>>> index e2a67d3469f6..c50247673c7e 100644 >>>>> --- a/net/mptcp/protocol.h >>>>> +++ b/net/mptcp/protocol.h >>>>> @@ -908,10 +908,8 @@ static inline bool mptcp_check_infinite_map(struct sk_buff *skb) >>>>> static inline bool subflow_simultaneous_connect(struct sock *sk) >>>>> { >>>>> struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk); >>>>> - struct sock *parent = subflow->conn; >>>>> >>>>> return sk->sk_state == TCP_ESTABLISHED && >>>>> - !mptcp_sk(parent)->pm.server_side && >>>>> !subflow->conn_finished; >>>>> } >>>> >>>> Are changes in this function needed? This code was added to address >>>> https://github.com/multipath-tcp/mptcp_net-next/issues/35 >>>> which was a weird case encountered with syzkaller where a socket tried to >>>> connect to itself, triggering the "simultaneous open" code path. It >>>> doesn't seem to be applicable to MP_JOINs. >>> >>> I *think* we can reach here even with MPJ, e.g. if we have port-based >>> endpoint matching the src address/port - possibly syzkaller could be >>> able to reach such scenario. >>> >>> Additionally I fear dropping the '!mptcp_sk(parent)->pm.server_side &&' >>> check could re-introduce the initial issue, at least for the MPC >>> handshake. >> >> The purpose of this commit is to permit the MPJ sequence to happen at either end of >> the connection. The change in mptcp_finish_join() had the most direct impact for enabling >> this, so I looked at other server_side checks mostly in terms of establishing that >> bi-directional symmetry. >> >> If the above condition holds only with MPCs, i.e. for the initial connection, then I think it would >> be fine to preserve the server_side check. But if it also holds for MPJs, then we might >> have to account for both cases. Perhaps replace: >> >> !mptcp_sk(parent)->pm.server_side) >> >> with something like this? >> >> ((subflow->request_mptcp && !mptcp_sk(parent)->pm.server_side) || >> subflow->request_join) > > It looks like 'request_mptcp' is 0 on passive sockets, so: > > subflow->request_mptcp || subflow->request_join > > should suffice. That's right. Makes sense. > > Possibly factoring out an helper for the above condition would make the > code more readable. Thanks! Will do so. > > /P >
On Wed, 19 Jan 2022, Kishen Maloor wrote: > On 1/19/22 4:01 AM, Paolo Abeni wrote: >> On Tue, 2022-01-18 at 17:26 -0800, Kishen Maloor wrote: >>> On 1/17/22 12:59 AM, Paolo Abeni wrote: >>>> On Fri, 2022-01-14 at 14:43 -0800, Mat Martineau wrote: >>>>> On Wed, 12 Jan 2022, Kishen Maloor wrote: >>>>> >>>>>> This change updates internal logic to permit subflows to be >>>>>> established from either the client or server ends of MPTCP >>>>>> connections. This symmetry and added flexibility may be >>>>>> harnessed by PM implementations running on either end in >>>>>> creating new subflows. >>>>>> >>>>>> The essence of this change lies in not relying on the >>>>>> "server_side" flag (which continues to be available if needed). >>>>>> >>>>> >>>>> After this patch, server_side is unused except for some debug output. If >>>>> it's really no longer referenced (see below first), may be better to just >>>>> remove it. >>> >>> I think we'll need it. >>> >>> One possible use is to inform the PM daemon of the type of application >>> (client/server) associated with a connection. >>> >>> As we're discussing the idea of creating listening sockets only upon request, the >>> knowledge that we are on the client end of a connection is something a PM daemon could >>> use to request creation of listening sockets whenever it advertises an address. >>> Ok, makes sense. >>>>> >>>>>> v2: check for 3rd ACK retransmission only on passive side >>>>>> of the MPJ handshake >>>>>> >>>>>> Signed-off-by: Kishen Maloor <kishen.maloor@intel.com> >>>>>> --- >>>>>> net/mptcp/options.c | 2 +- >>>>>> net/mptcp/protocol.c | 5 +---- >>>>>> net/mptcp/protocol.h | 2 -- >>>>>> 3 files changed, 2 insertions(+), 7 deletions(-) >>>>>> >>>>>> diff --git a/net/mptcp/options.c b/net/mptcp/options.c >>>>>> index cceba8c7806d..89808816db06 100644 >>>>>> --- a/net/mptcp/options.c >>>>>> +++ b/net/mptcp/options.c >>>>>> @@ -921,7 +921,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk, >>>>>> if (TCP_SKB_CB(skb)->seq == subflow->ssn_offset + 1 && >>>>>> TCP_SKB_CB(skb)->end_seq == TCP_SKB_CB(skb)->seq && >>>>>> subflow->mp_join && (mp_opt->suboptions & OPTIONS_MPTCP_MPJ) && >>>>>> - READ_ONCE(msk->pm.server_side)) >>>>>> + !subflow->request_join) >>>>>> tcp_send_ack(ssk); >>>>>> goto fully_established; >>>>>> } >>>>>> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c >>>>>> index 4012f844eec1..408a05bff633 100644 >>>>>> --- a/net/mptcp/protocol.c >>>>>> +++ b/net/mptcp/protocol.c >>>>>> @@ -3248,15 +3248,12 @@ bool mptcp_finish_join(struct sock *ssk) >>>>>> return false; >>>>>> } >>>>>> >>>>>> - if (!msk->pm.server_side) >>>>>> + if (!list_empty(&subflow->node)) >>>>>> goto out; >>>>>> >>>>>> if (!mptcp_pm_allow_new_subflow(msk)) >>>>>> goto err_prohibited; >>>>>> >>>>>> - if (WARN_ON_ONCE(!list_empty(&subflow->node))) >>>>>> - goto err_prohibited; >>>>>> - >>>>>> /* active connections are already on conn_list. >>>>>> * If we can't acquire msk socket lock here, let the release callback >>>>>> * handle it >>>>>> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h >>>>>> index e2a67d3469f6..c50247673c7e 100644 >>>>>> --- a/net/mptcp/protocol.h >>>>>> +++ b/net/mptcp/protocol.h >>>>>> @@ -908,10 +908,8 @@ static inline bool mptcp_check_infinite_map(struct sk_buff *skb) >>>>>> static inline bool subflow_simultaneous_connect(struct sock *sk) >>>>>> { >>>>>> struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk); >>>>>> - struct sock *parent = subflow->conn; >>>>>> >>>>>> return sk->sk_state == TCP_ESTABLISHED && >>>>>> - !mptcp_sk(parent)->pm.server_side && >>>>>> !subflow->conn_finished; >>>>>> } >>>>> >>>>> Are changes in this function needed? This code was added to address >>>>> https://github.com/multipath-tcp/mptcp_net-next/issues/35 >>>>> which was a weird case encountered with syzkaller where a socket tried to >>>>> connect to itself, triggering the "simultaneous open" code path. It >>>>> doesn't seem to be applicable to MP_JOINs. >>>> >>>> I *think* we can reach here even with MPJ, e.g. if we have port-based >>>> endpoint matching the src address/port - possibly syzkaller could be >>>> able to reach such scenario. >>>> >>>> Additionally I fear dropping the '!mptcp_sk(parent)->pm.server_side &&' >>>> check could re-introduce the initial issue, at least for the MPC >>>> handshake. >>> >>> The purpose of this commit is to permit the MPJ sequence to happen at either end of >>> the connection. The change in mptcp_finish_join() had the most direct impact for enabling >>> this, so I looked at other server_side checks mostly in terms of establishing that >>> bi-directional symmetry. >>> >>> If the above condition holds only with MPCs, i.e. for the initial connection, then I think it would >>> be fine to preserve the server_side check. But if it also holds for MPJs, then we might >>> have to account for both cases. Perhaps replace: >>> >>> !mptcp_sk(parent)->pm.server_side) >>> >>> with something like this? >>> >>> ((subflow->request_mptcp && !mptcp_sk(parent)->pm.server_side) || >>> subflow->request_join) >> >> It looks like 'request_mptcp' is 0 on passive sockets, so: >> >> subflow->request_mptcp || subflow->request_join >> >> should suffice. > > That's right. Makes sense. > Yeah, good to simplify this even if 'pm.server_side' is still around. >> >> Possibly factoring out an helper for the above condition would make the >> code more readable. > > Thanks! Will do so. > -- Mat Martineau Intel
diff --git a/net/mptcp/options.c b/net/mptcp/options.c index cceba8c7806d..89808816db06 100644 --- a/net/mptcp/options.c +++ b/net/mptcp/options.c @@ -921,7 +921,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk, if (TCP_SKB_CB(skb)->seq == subflow->ssn_offset + 1 && TCP_SKB_CB(skb)->end_seq == TCP_SKB_CB(skb)->seq && subflow->mp_join && (mp_opt->suboptions & OPTIONS_MPTCP_MPJ) && - READ_ONCE(msk->pm.server_side)) + !subflow->request_join) tcp_send_ack(ssk); goto fully_established; } diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index 4012f844eec1..408a05bff633 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -3248,15 +3248,12 @@ bool mptcp_finish_join(struct sock *ssk) return false; } - if (!msk->pm.server_side) + if (!list_empty(&subflow->node)) goto out; if (!mptcp_pm_allow_new_subflow(msk)) goto err_prohibited; - if (WARN_ON_ONCE(!list_empty(&subflow->node))) - goto err_prohibited; - /* active connections are already on conn_list. * If we can't acquire msk socket lock here, let the release callback * handle it diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index e2a67d3469f6..c50247673c7e 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -908,10 +908,8 @@ static inline bool mptcp_check_infinite_map(struct sk_buff *skb) static inline bool subflow_simultaneous_connect(struct sock *sk) { struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk); - struct sock *parent = subflow->conn; return sk->sk_state == TCP_ESTABLISHED && - !mptcp_sk(parent)->pm.server_side && !subflow->conn_finished; }
This change updates internal logic to permit subflows to be established from either the client or server ends of MPTCP connections. This symmetry and added flexibility may be harnessed by PM implementations running on either end in creating new subflows. The essence of this change lies in not relying on the "server_side" flag (which continues to be available if needed). v2: check for 3rd ACK retransmission only on passive side of the MPJ handshake Signed-off-by: Kishen Maloor <kishen.maloor@intel.com> --- net/mptcp/options.c | 2 +- net/mptcp/protocol.c | 5 +---- net/mptcp/protocol.h | 2 -- 3 files changed, 2 insertions(+), 7 deletions(-)