| Message ID | 77387e8589417f2ccb445136c74d16ed44a344d1.1619035356.git.pabeni@redhat.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | Paolo Abeni |
| Headers | show |
| Series | mptcp: data checksum support | expand |
Hi Paolo, Paolo Abeni <pabeni@redhat.com> 于2021年4月22日周四 上午4:20写道： > > always parse any csum/nocsum combination and delay the > presence check to later code, to allow reset if missing. > > Additionally, in the TX path, use the newly introduce ext > field to avoid MPTCP csum recomputation on tcp retransmission > and unneeded csum update on when setting the data fin_flag. > > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > --- > include/net/mptcp.h | 3 ++- > net/mptcp/options.c | 32 ++++++++++++++++++++------------ > net/mptcp/protocol.h | 2 ++ > 3 files changed, 24 insertions(+), 13 deletions(-) > > diff --git a/include/net/mptcp.h b/include/net/mptcp.h > index 8f86c05ddbfd..bd272c34b53c 100644 > --- a/include/net/mptcp.h > +++ b/include/net/mptcp.h > @@ -32,7 +32,8 @@ struct mptcp_ext { > mpc_map:1, > frozen:1, > reset_transient:1; > - u8 reset_reason:4; > + u8 reset_reason:4, > + csum_reqd:1; I want to move these lines to the patch ([PATCH v3 mptcp-next 03/21] Squash-to: "mptcp: generate the data checksum") in the next version. > }; > > #define MPTCP_RM_IDS_MAX 8 > diff --git a/net/mptcp/options.c b/net/mptcp/options.c > index cd205ad09c00..83a981684bbf 100644 > --- a/net/mptcp/options.c > +++ b/net/mptcp/options.c > @@ -39,8 +39,6 @@ static void mptcp_parse_option(const struct sock *sk, > if (!(TCP_SKB_CB(skb)->tcp_flags & TCPHDR_SYN)) { > if (skb->len > tcp_hdr(skb)->doff << 2) { > expected_opsize = TCPOLEN_MPTCP_MPC_ACK_DATA; > - if (READ_ONCE(msk->csum_enabled)) > - expected_opsize += TCPOLEN_MPTCP_DSS_CHECKSUM; > } else { > expected_opsize = TCPOLEN_MPTCP_MPC_ACK; > } I'll drop these braces '{}' too, it looks like: if (skb->len > tcp_hdr(skb)->doff << 2) expected_opsize = TCPOLEN_MPTCP_MPC_ACK_DATA; else expected_opsize = TCPOLEN_MPTCP_MPC_ACK; > @@ -50,7 +48,20 @@ static void mptcp_parse_option(const struct sock *sk, > else > expected_opsize = TCPOLEN_MPTCP_MPC_SYN; > } > - if (opsize != expected_opsize) > + > + /* Cfr RFC 8684 Section 3.3.0: > + * If a checksum is present but its use had > + * not been negotiated in the MP_CAPABLE handshake, the receiver MUST > + * close the subflow with a RST, as it is not behaving as negotiated. > + * If a checksum is not present when its use has been negotiated, the > + * receiver MUST close the subflow with a RST, as it is considered > + * broken > + * We parse even option with mismatching csum presence, so that > + * later in subflow_data_ready we can trigger the reset. > + */ > + if ((opsize != expected_opsize) && > + (expected_opsize != TCPOLEN_MPTCP_MPC_ACK_DATA || > + opsize != TCPOLEN_MPTCP_MPC_ACK_DATA_CSUM)) > break; > > /* try to be gentle vs future versions on the initial syn */ > @@ -72,11 +83,6 @@ static void mptcp_parse_option(const struct sock *sk, > * host requires the use of checksums, checksums MUST be used. > * In other words, the only way for checksums not to be used > * is if both hosts in their SYNs set A=0." > - * > - * Section 3.3.0: > - * "If a checksum is not present when its use has been > - * negotiated, the receiver MUST close the subflow with a RST as > - * it is considered broken." > */ > mp_opt->csum_reqd = READ_ONCE(msk->csum_enabled); > if (flags & MPTCP_CAP_CHECKSUM_REQD) > @@ -103,8 +109,9 @@ static void mptcp_parse_option(const struct sock *sk, > mp_opt->data_len = get_unaligned_be16(ptr); > ptr += 2; > } > - if (opsize == TCPOLEN_MPTCP_MPC_ACK_DATA + TCPOLEN_MPTCP_DSS_CHECKSUM) { > + if (opsize == TCPOLEN_MPTCP_MPC_ACK_DATA_CSUM) { > mp_opt->csum = (__force __sum16)get_unaligned_be16(ptr); > + mp_opt->csum_reqd = 1; > ptr += 2; > } > pr_debug("MP_CAPABLE version=%x, flags=%x, optlen=%d sndr=%llu, rcvr=%llu len=%d csum=%u", > @@ -353,7 +360,6 @@ void mptcp_get_options(const struct sock *sk, > mp_opt->mp_prio = 0; > mp_opt->reset = 0; > mp_opt->csum_reqd = 0; > - mp_opt->csum = 0; I want to keep this "mp_opt->csum = 0;" here. > > length = (th->doff * 4) - sizeof(struct tcphdr); > ptr = (const unsigned char *)(th + 1); > @@ -543,7 +549,8 @@ static void mptcp_write_data_fin(struct mptcp_subflow_context *subflow, > ext->data_len++; > > /* the pseudo header has changed, update the csum accordingly */ > - csum_replace2(&ext->csum, ext->data_len - 1, ext->data_len); > + if (ext->csum_reqd) > + csum_replace2(&ext->csum, ext->data_len - 1, ext->data_len); And I want to move these lines to the patch ([PATCH v3 mptcp-next 03/21] Squash-to: "mptcp: generate the data checksum") too. WDYT? Thanks. -Geliang > } > } > > @@ -1129,8 +1136,9 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb) > } > mpext->data_len = mp_opt.data_len; > mpext->use_map = 1; > + mpext->csum_reqd = mp_opt.csum_reqd; > > - if (READ_ONCE(msk->csum_enabled)) > + if (mpext->csum_reqd) > mpext->csum = mp_opt.csum; > } > } > diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h > index d2b00f8c4040..3ac61b8178fd 100644 > --- a/net/mptcp/protocol.h > +++ b/net/mptcp/protocol.h > @@ -68,6 +68,8 @@ > #define TCPOLEN_MPTCP_FASTCLOSE 12 > #define TCPOLEN_MPTCP_RST 4 > > +#define TCPOLEN_MPTCP_MPC_ACK_DATA_CSUM (TCPOLEN_MPTCP_DSS_CHECKSUM + TCPOLEN_MPTCP_MPC_ACK_DATA) > + > /* MPTCP MP_JOIN flags */ > #define MPTCPOPT_BACKUP BIT(0) > #define MPTCPOPT_HMAC_LEN 20 > -- > 2.26.2 >
On Fri, 2021-04-23 at 11:17 +0800, Geliang Tang wrote: > Hi Paolo, > > Paolo Abeni <pabeni@redhat.com> 于2021年4月22日周四 上午4:20写道： > > always parse any csum/nocsum combination and delay the > > presence check to later code, to allow reset if missing. > > > > Additionally, in the TX path, use the newly introduce ext > > field to avoid MPTCP csum recomputation on tcp retransmission > > and unneeded csum update on when setting the data fin_flag. > > > > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > > --- > > include/net/mptcp.h | 3 ++- > > net/mptcp/options.c | 32 ++++++++++++++++++++------------ > > net/mptcp/protocol.h | 2 ++ > > 3 files changed, 24 insertions(+), 13 deletions(-) > > > > diff --git a/include/net/mptcp.h b/include/net/mptcp.h > > index 8f86c05ddbfd..bd272c34b53c 100644 > > --- a/include/net/mptcp.h > > +++ b/include/net/mptcp.h > > @@ -32,7 +32,8 @@ struct mptcp_ext { > > mpc_map:1, > > frozen:1, > > reset_transient:1; > > - u8 reset_reason:4; > > + u8 reset_reason:4, > > + csum_reqd:1; > > I want to move these lines to the patch ([PATCH v3 mptcp-next 03/21] > Squash-to: "mptcp: generate the data checksum") in the next version. The 'csum_reqd' bit is used a validity status for the parsed csum: we know a csum was present in the parsed DSS if and only if csum_reqd != 0. I think it needs to stay here. > > }; > > > > #define MPTCP_RM_IDS_MAX 8 > > diff --git a/net/mptcp/options.c b/net/mptcp/options.c > > index cd205ad09c00..83a981684bbf 100644 > > --- a/net/mptcp/options.c > > +++ b/net/mptcp/options.c > > @@ -39,8 +39,6 @@ static void mptcp_parse_option(const struct sock *sk, > > if (!(TCP_SKB_CB(skb)->tcp_flags & TCPHDR_SYN)) { > > if (skb->len > tcp_hdr(skb)->doff << 2) { > > expected_opsize = TCPOLEN_MPTCP_MPC_ACK_DATA; > > - if (READ_ONCE(msk->csum_enabled)) > > - expected_opsize += TCPOLEN_MPTCP_DSS_CHECKSUM; > > } else { > > expected_opsize = TCPOLEN_MPTCP_MPC_ACK; > > } > > I'll drop these braces '{}' too, it looks like: > > if (skb->len > tcp_hdr(skb)->doff << 2) > expected_opsize = TCPOLEN_MPTCP_MPC_ACK_DATA; > else > expected_opsize = TCPOLEN_MPTCP_MPC_ACK; Agreed. > > @@ -50,7 +48,20 @@ static void mptcp_parse_option(const struct sock *sk, > > else > > expected_opsize = TCPOLEN_MPTCP_MPC_SYN; > > } > > - if (opsize != expected_opsize) > > + > > + /* Cfr RFC 8684 Section 3.3.0: > > + * If a checksum is present but its use had > > + * not been negotiated in the MP_CAPABLE handshake, the receiver MUST > > + * close the subflow with a RST, as it is not behaving as negotiated. > > + * If a checksum is not present when its use has been negotiated, the > > + * receiver MUST close the subflow with a RST, as it is considered > > + * broken > > + * We parse even option with mismatching csum presence, so that > > + * later in subflow_data_ready we can trigger the reset. > > + */ > > + if ((opsize != expected_opsize) && > > + (expected_opsize != TCPOLEN_MPTCP_MPC_ACK_DATA || > > + opsize != TCPOLEN_MPTCP_MPC_ACK_DATA_CSUM)) > > break; > > > > /* try to be gentle vs future versions on the initial syn */ > > @@ -72,11 +83,6 @@ static void mptcp_parse_option(const struct sock *sk, > > * host requires the use of checksums, checksums MUST be used. > > * In other words, the only way for checksums not to be used > > * is if both hosts in their SYNs set A=0." > > - * > > - * Section 3.3.0: > > - * "If a checksum is not present when its use has been > > - * negotiated, the receiver MUST close the subflow with a RST as > > - * it is considered broken." > > */ > > mp_opt->csum_reqd = READ_ONCE(msk->csum_enabled); > > if (flags & MPTCP_CAP_CHECKSUM_REQD) > > @@ -103,8 +109,9 @@ static void mptcp_parse_option(const struct sock *sk, > > mp_opt->data_len = get_unaligned_be16(ptr); > > ptr += 2; > > } > > - if (opsize == TCPOLEN_MPTCP_MPC_ACK_DATA + TCPOLEN_MPTCP_DSS_CHECKSUM) { > > + if (opsize == TCPOLEN_MPTCP_MPC_ACK_DATA_CSUM) { > > mp_opt->csum = (__force __sum16)get_unaligned_be16(ptr); > > + mp_opt->csum_reqd = 1; > > ptr += 2; > > } > > pr_debug("MP_CAPABLE version=%x, flags=%x, optlen=%d sndr=%llu, rcvr=%llu len=%d csum=%u", > > @@ -353,7 +360,6 @@ void mptcp_get_options(const struct sock *sk, > > mp_opt->mp_prio = 0; > > mp_opt->reset = 0; > > mp_opt->csum_reqd = 0; > > - mp_opt->csum = 0; > > I want to keep this "mp_opt->csum = 0;" here. No, this is not needed. zeoring the csum is not needed. The csum should be accessed if and only if csum_reqd != 0, and in that case 'csum' will always be initialized. > > length = (th->doff * 4) - sizeof(struct tcphdr); > > ptr = (const unsigned char *)(th + 1); > > @@ -543,7 +549,8 @@ static void mptcp_write_data_fin(struct mptcp_subflow_context *subflow, > > ext->data_len++; > > > > /* the pseudo header has changed, update the csum accordingly */ > > - csum_replace2(&ext->csum, ext->data_len - 1, ext->data_len); > > + if (ext->csum_reqd) > > + csum_replace2(&ext->csum, ext->data_len - 1, ext->data_len); > > And I want to move these lines to the patch ([PATCH v3 mptcp-next 03/21] > Squash-to: "mptcp: generate the data checksum") too. Agreed. Side note: I can send the relevant changes, n.p. /P
Hi Paolo, Paolo Abeni <pabeni@redhat.com> 于2021年4月23日周五 下午4:52写道： > > On Fri, 2021-04-23 at 11:17 +0800, Geliang Tang wrote: > > Hi Paolo, > > > > Paolo Abeni <pabeni@redhat.com> 于2021年4月22日周四 上午4:20写道： > > > always parse any csum/nocsum combination and delay the > > > presence check to later code, to allow reset if missing. > > > > > > Additionally, in the TX path, use the newly introduce ext > > > field to avoid MPTCP csum recomputation on tcp retransmission > > > and unneeded csum update on when setting the data fin_flag. > > > > > > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > > > --- > > > include/net/mptcp.h | 3 ++- > > > net/mptcp/options.c | 32 ++++++++++++++++++++------------ > > > net/mptcp/protocol.h | 2 ++ > > > 3 files changed, 24 insertions(+), 13 deletions(-) > > > > > > diff --git a/include/net/mptcp.h b/include/net/mptcp.h > > > index 8f86c05ddbfd..bd272c34b53c 100644 > > > --- a/include/net/mptcp.h > > > +++ b/include/net/mptcp.h > > > @@ -32,7 +32,8 @@ struct mptcp_ext { > > > mpc_map:1, > > > frozen:1, > > > reset_transient:1; > > > - u8 reset_reason:4; > > > + u8 reset_reason:4, > > > + csum_reqd:1; > > > > I want to move these lines to the patch ([PATCH v3 mptcp-next 03/21] > > Squash-to: "mptcp: generate the data checksum") in the next version. > > The 'csum_reqd' bit is used a validity status for the parsed csum: we > know a csum was present in the parsed DSS if and only if csum_reqd != > 0. I think it needs to stay here. If we keep this csum_reqd in this patch ... > > > > }; > > > > > > #define MPTCP_RM_IDS_MAX 8 > > > diff --git a/net/mptcp/options.c b/net/mptcp/options.c > > > index cd205ad09c00..83a981684bbf 100644 > > > --- a/net/mptcp/options.c > > > +++ b/net/mptcp/options.c > > > @@ -39,8 +39,6 @@ static void mptcp_parse_option(const struct sock *sk, > > > if (!(TCP_SKB_CB(skb)->tcp_flags & TCPHDR_SYN)) { > > > if (skb->len > tcp_hdr(skb)->doff << 2) { > > > expected_opsize = TCPOLEN_MPTCP_MPC_ACK_DATA; > > > - if (READ_ONCE(msk->csum_enabled)) > > > - expected_opsize += TCPOLEN_MPTCP_DSS_CHECKSUM; > > > } else { > > > expected_opsize = TCPOLEN_MPTCP_MPC_ACK; > > > } > > > > I'll drop these braces '{}' too, it looks like: > > > > if (skb->len > tcp_hdr(skb)->doff << 2) > > expected_opsize = TCPOLEN_MPTCP_MPC_ACK_DATA; > > else > > expected_opsize = TCPOLEN_MPTCP_MPC_ACK; > > Agreed. > > > > @@ -50,7 +48,20 @@ static void mptcp_parse_option(const struct sock *sk, > > > else > > > expected_opsize = TCPOLEN_MPTCP_MPC_SYN; > > > } > > > - if (opsize != expected_opsize) > > > + > > > + /* Cfr RFC 8684 Section 3.3.0: > > > + * If a checksum is present but its use had > > > + * not been negotiated in the MP_CAPABLE handshake, the receiver MUST > > > + * close the subflow with a RST, as it is not behaving as negotiated. > > > + * If a checksum is not present when its use has been negotiated, the > > > + * receiver MUST close the subflow with a RST, as it is considered > > > + * broken > > > + * We parse even option with mismatching csum presence, so that > > > + * later in subflow_data_ready we can trigger the reset. > > > + */ > > > + if ((opsize != expected_opsize) && > > > + (expected_opsize != TCPOLEN_MPTCP_MPC_ACK_DATA || > > > + opsize != TCPOLEN_MPTCP_MPC_ACK_DATA_CSUM)) > > > break; > > > > > > /* try to be gentle vs future versions on the initial syn */ > > > @@ -72,11 +83,6 @@ static void mptcp_parse_option(const struct sock *sk, > > > * host requires the use of checksums, checksums MUST be used. > > > * In other words, the only way for checksums not to be used > > > * is if both hosts in their SYNs set A=0." > > > - * > > > - * Section 3.3.0: > > > - * "If a checksum is not present when its use has been > > > - * negotiated, the receiver MUST close the subflow with a RST as > > > - * it is considered broken." > > > */ > > > mp_opt->csum_reqd = READ_ONCE(msk->csum_enabled); > > > if (flags & MPTCP_CAP_CHECKSUM_REQD) > > > @@ -103,8 +109,9 @@ static void mptcp_parse_option(const struct sock *sk, > > > mp_opt->data_len = get_unaligned_be16(ptr); > > > ptr += 2; > > > } > > > - if (opsize == TCPOLEN_MPTCP_MPC_ACK_DATA + TCPOLEN_MPTCP_DSS_CHECKSUM) { > > > + if (opsize == TCPOLEN_MPTCP_MPC_ACK_DATA_CSUM) { > > > mp_opt->csum = (__force __sum16)get_unaligned_be16(ptr); > > > + mp_opt->csum_reqd = 1; > > > ptr += 2; > > > } > > > pr_debug("MP_CAPABLE version=%x, flags=%x, optlen=%d sndr=%llu, rcvr=%llu len=%d csum=%u", > > > @@ -353,7 +360,6 @@ void mptcp_get_options(const struct sock *sk, > > > mp_opt->mp_prio = 0; > > > mp_opt->reset = 0; > > > mp_opt->csum_reqd = 0; > > > - mp_opt->csum = 0; > > > > I want to keep this "mp_opt->csum = 0;" here. > > No, this is not needed. zeoring the csum is not needed. The csum should > be accessed if and only if csum_reqd != 0, and in that case 'csum' will > always be initialized. > > > > length = (th->doff * 4) - sizeof(struct tcphdr); > > > ptr = (const unsigned char *)(th + 1); > > > @@ -543,7 +549,8 @@ static void mptcp_write_data_fin(struct mptcp_subflow_context *subflow, > > > ext->data_len++; > > > > > > /* the pseudo header has changed, update the csum accordingly */ > > > - csum_replace2(&ext->csum, ext->data_len - 1, ext->data_len); > > > + if (ext->csum_reqd) > > > + csum_replace2(&ext->csum, ext->data_len - 1, ext->data_len); > > > > And I want to move these lines to the patch ([PATCH v3 mptcp-next 03/21] > > Squash-to: "mptcp: generate the data checksum") too. > > Agreed. ... we need to keep these lines in this patch too. Is that right? > > Side note: I can send the relevant changes, n.p. Please send the changes for me, thanks. -Geliang > > /P >
On Fri, 2021-04-23 at 17:16 +0800, Geliang Tang wrote: > Hi Paolo, > > Paolo Abeni <pabeni@redhat.com> 于2021年4月23日周五 下午4:52写道： > > On Fri, 2021-04-23 at 11:17 +0800, Geliang Tang wrote: > > > Hi Paolo, > > > > > > Paolo Abeni <pabeni@redhat.com> 于2021年4月22日周四 上午4:20写道： > > > > always parse any csum/nocsum combination and delay the > > > > presence check to later code, to allow reset if missing. > > > > > > > > Additionally, in the TX path, use the newly introduce ext > > > > field to avoid MPTCP csum recomputation on tcp retransmission > > > > and unneeded csum update on when setting the data fin_flag. > > > > > > > > Signed-off-by: Paolo Abeni <pabeni@redhat.com> > > > > --- > > > > include/net/mptcp.h | 3 ++- > > > > net/mptcp/options.c | 32 ++++++++++++++++++++------------ > > > > net/mptcp/protocol.h | 2 ++ > > > > 3 files changed, 24 insertions(+), 13 deletions(-) > > > > > > > > diff --git a/include/net/mptcp.h b/include/net/mptcp.h > > > > index 8f86c05ddbfd..bd272c34b53c 100644 > > > > --- a/include/net/mptcp.h > > > > +++ b/include/net/mptcp.h > > > > @@ -32,7 +32,8 @@ struct mptcp_ext { > > > > mpc_map:1, > > > > frozen:1, > > > > reset_transient:1; > > > > - u8 reset_reason:4; > > > > + u8 reset_reason:4, > > > > + csum_reqd:1; > > > > > > I want to move these lines to the patch ([PATCH v3 mptcp-next 03/21] > > > Squash-to: "mptcp: generate the data checksum") in the next version. > > > > The 'csum_reqd' bit is used a validity status for the parsed csum: we > > know a csum was present in the parsed DSS if and only if csum_reqd != > > 0. I think it needs to stay here. > > If we keep this csum_reqd in this patch ... > > > > > }; > > > > > > > > #define MPTCP_RM_IDS_MAX 8 > > > > diff --git a/net/mptcp/options.c b/net/mptcp/options.c > > > > index cd205ad09c00..83a981684bbf 100644 > > > > --- a/net/mptcp/options.c > > > > +++ b/net/mptcp/options.c > > > > @@ -39,8 +39,6 @@ static void mptcp_parse_option(const struct sock *sk, > > > > if (!(TCP_SKB_CB(skb)->tcp_flags & TCPHDR_SYN)) { > > > > if (skb->len > tcp_hdr(skb)->doff << 2) { > > > > expected_opsize = TCPOLEN_MPTCP_MPC_ACK_DATA; > > > > - if (READ_ONCE(msk->csum_enabled)) > > > > - expected_opsize += TCPOLEN_MPTCP_DSS_CHECKSUM; > > > > } else { > > > > expected_opsize = TCPOLEN_MPTCP_MPC_ACK; > > > > } > > > > > > I'll drop these braces '{}' too, it looks like: > > > > > > if (skb->len > tcp_hdr(skb)->doff << 2) > > > expected_opsize = TCPOLEN_MPTCP_MPC_ACK_DATA; > > > else > > > expected_opsize = TCPOLEN_MPTCP_MPC_ACK; > > > > Agreed. > > > > > > @@ -50,7 +48,20 @@ static void mptcp_parse_option(const struct sock *sk, > > > > else > > > > expected_opsize = TCPOLEN_MPTCP_MPC_SYN; > > > > } > > > > - if (opsize != expected_opsize) > > > > + > > > > + /* Cfr RFC 8684 Section 3.3.0: > > > > + * If a checksum is present but its use had > > > > + * not been negotiated in the MP_CAPABLE handshake, the receiver MUST > > > > + * close the subflow with a RST, as it is not behaving as negotiated. > > > > + * If a checksum is not present when its use has been negotiated, the > > > > + * receiver MUST close the subflow with a RST, as it is considered > > > > + * broken > > > > + * We parse even option with mismatching csum presence, so that > > > > + * later in subflow_data_ready we can trigger the reset. > > > > + */ > > > > + if ((opsize != expected_opsize) && > > > > + (expected_opsize != TCPOLEN_MPTCP_MPC_ACK_DATA || > > > > + opsize != TCPOLEN_MPTCP_MPC_ACK_DATA_CSUM)) > > > > break; > > > > > > > > /* try to be gentle vs future versions on the initial syn */ > > > > @@ -72,11 +83,6 @@ static void mptcp_parse_option(const struct sock *sk, > > > > * host requires the use of checksums, checksums MUST be used. > > > > * In other words, the only way for checksums not to be used > > > > * is if both hosts in their SYNs set A=0." > > > > - * > > > > - * Section 3.3.0: > > > > - * "If a checksum is not present when its use has been > > > > - * negotiated, the receiver MUST close the subflow with a RST as > > > > - * it is considered broken." > > > > */ > > > > mp_opt->csum_reqd = READ_ONCE(msk->csum_enabled); > > > > if (flags & MPTCP_CAP_CHECKSUM_REQD) > > > > @@ -103,8 +109,9 @@ static void mptcp_parse_option(const struct sock *sk, > > > > mp_opt->data_len = get_unaligned_be16(ptr); > > > > ptr += 2; > > > > } > > > > - if (opsize == TCPOLEN_MPTCP_MPC_ACK_DATA + TCPOLEN_MPTCP_DSS_CHECKSUM) { > > > > + if (opsize == TCPOLEN_MPTCP_MPC_ACK_DATA_CSUM) { > > > > mp_opt->csum = (__force __sum16)get_unaligned_be16(ptr); > > > > + mp_opt->csum_reqd = 1; > > > > ptr += 2; > > > > } > > > > pr_debug("MP_CAPABLE version=%x, flags=%x, optlen=%d sndr=%llu, rcvr=%llu len=%d csum=%u", > > > > @@ -353,7 +360,6 @@ void mptcp_get_options(const struct sock *sk, > > > > mp_opt->mp_prio = 0; > > > > mp_opt->reset = 0; > > > > mp_opt->csum_reqd = 0; > > > > - mp_opt->csum = 0; > > > > > > I want to keep this "mp_opt->csum = 0;" here. > > > > No, this is not needed. zeoring the csum is not needed. The csum should > > be accessed if and only if csum_reqd != 0, and in that case 'csum' will > > always be initialized. > > > > > > length = (th->doff * 4) - sizeof(struct tcphdr); > > > > ptr = (const unsigned char *)(th + 1); > > > > @@ -543,7 +549,8 @@ static void mptcp_write_data_fin(struct mptcp_subflow_context *subflow, > > > > ext->data_len++; > > > > > > > > /* the pseudo header has changed, update the csum accordingly */ > > > > - csum_replace2(&ext->csum, ext->data_len - 1, ext->data_len); > > > > + if (ext->csum_reqd) > > > > + csum_replace2(&ext->csum, ext->data_len - 1, ext->data_len); > > > > > > And I want to move these lines to the patch ([PATCH v3 mptcp-next 03/21] > > > Squash-to: "mptcp: generate the data checksum") too. > > > > Agreed. > > ... we need to keep these lines in this patch too. > > Is that right? You are right! > > Side note: I can send the relevant changes, n.p. > > Please send the changes for me, thanks. Will do! Thanks! Paolo
diff --git a/include/net/mptcp.h b/include/net/mptcp.h index 8f86c05ddbfd..bd272c34b53c 100644 --- a/include/net/mptcp.h +++ b/include/net/mptcp.h @@ -32,7 +32,8 @@ struct mptcp_ext { mpc_map:1, frozen:1, reset_transient:1; - u8 reset_reason:4; + u8 reset_reason:4, + csum_reqd:1; }; #define MPTCP_RM_IDS_MAX 8 diff --git a/net/mptcp/options.c b/net/mptcp/options.c index cd205ad09c00..83a981684bbf 100644 --- a/net/mptcp/options.c +++ b/net/mptcp/options.c @@ -39,8 +39,6 @@ static void mptcp_parse_option(const struct sock *sk, if (!(TCP_SKB_CB(skb)->tcp_flags & TCPHDR_SYN)) { if (skb->len > tcp_hdr(skb)->doff << 2) { expected_opsize = TCPOLEN_MPTCP_MPC_ACK_DATA; - if (READ_ONCE(msk->csum_enabled)) - expected_opsize += TCPOLEN_MPTCP_DSS_CHECKSUM; } else { expected_opsize = TCPOLEN_MPTCP_MPC_ACK; } @@ -50,7 +48,20 @@ static void mptcp_parse_option(const struct sock *sk, else expected_opsize = TCPOLEN_MPTCP_MPC_SYN; } - if (opsize != expected_opsize) + + /* Cfr RFC 8684 Section 3.3.0: + * If a checksum is present but its use had + * not been negotiated in the MP_CAPABLE handshake, the receiver MUST + * close the subflow with a RST, as it is not behaving as negotiated. + * If a checksum is not present when its use has been negotiated, the + * receiver MUST close the subflow with a RST, as it is considered + * broken + * We parse even option with mismatching csum presence, so that + * later in subflow_data_ready we can trigger the reset. + */ + if ((opsize != expected_opsize) && + (expected_opsize != TCPOLEN_MPTCP_MPC_ACK_DATA || + opsize != TCPOLEN_MPTCP_MPC_ACK_DATA_CSUM)) break; /* try to be gentle vs future versions on the initial syn */ @@ -72,11 +83,6 @@ static void mptcp_parse_option(const struct sock *sk, * host requires the use of checksums, checksums MUST be used. * In other words, the only way for checksums not to be used * is if both hosts in their SYNs set A=0." - * - * Section 3.3.0: - * "If a checksum is not present when its use has been - * negotiated, the receiver MUST close the subflow with a RST as - * it is considered broken." */ mp_opt->csum_reqd = READ_ONCE(msk->csum_enabled); if (flags & MPTCP_CAP_CHECKSUM_REQD) @@ -103,8 +109,9 @@ static void mptcp_parse_option(const struct sock *sk, mp_opt->data_len = get_unaligned_be16(ptr); ptr += 2; } - if (opsize == TCPOLEN_MPTCP_MPC_ACK_DATA + TCPOLEN_MPTCP_DSS_CHECKSUM) { + if (opsize == TCPOLEN_MPTCP_MPC_ACK_DATA_CSUM) { mp_opt->csum = (__force __sum16)get_unaligned_be16(ptr); + mp_opt->csum_reqd = 1; ptr += 2; } pr_debug("MP_CAPABLE version=%x, flags=%x, optlen=%d sndr=%llu, rcvr=%llu len=%d csum=%u", @@ -353,7 +360,6 @@ void mptcp_get_options(const struct sock *sk, mp_opt->mp_prio = 0; mp_opt->reset = 0; mp_opt->csum_reqd = 0; - mp_opt->csum = 0; length = (th->doff * 4) - sizeof(struct tcphdr); ptr = (const unsigned char *)(th + 1); @@ -543,7 +549,8 @@ static void mptcp_write_data_fin(struct mptcp_subflow_context *subflow, ext->data_len++; /* the pseudo header has changed, update the csum accordingly */ - csum_replace2(&ext->csum, ext->data_len - 1, ext->data_len); + if (ext->csum_reqd) + csum_replace2(&ext->csum, ext->data_len - 1, ext->data_len); } } @@ -1129,8 +1136,9 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb) } mpext->data_len = mp_opt.data_len; mpext->use_map = 1; + mpext->csum_reqd = mp_opt.csum_reqd; - if (READ_ONCE(msk->csum_enabled)) + if (mpext->csum_reqd) mpext->csum = mp_opt.csum; } } diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index d2b00f8c4040..3ac61b8178fd 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -68,6 +68,8 @@ #define TCPOLEN_MPTCP_FASTCLOSE 12 #define TCPOLEN_MPTCP_RST 4 +#define TCPOLEN_MPTCP_MPC_ACK_DATA_CSUM (TCPOLEN_MPTCP_DSS_CHECKSUM + TCPOLEN_MPTCP_MPC_ACK_DATA) + /* MPTCP MP_JOIN flags */ #define MPTCPOPT_BACKUP BIT(0) #define MPTCPOPT_HMAC_LEN 20
always parse any csum/nocsum combination and delay the presence check to later code, to allow reset if missing. Additionally, in the TX path, use the newly introduce ext field to avoid MPTCP csum recomputation on tcp retransmission and unneeded csum update on when setting the data fin_flag. Signed-off-by: Paolo Abeni <pabeni@redhat.com> --- include/net/mptcp.h | 3 ++- net/mptcp/options.c | 32 ++++++++++++++++++++------------ net/mptcp/protocol.h | 2 ++ 3 files changed, 24 insertions(+), 13 deletions(-)