7:14 p.m.
This is a very early RFC showcasing the fix discussed in yday mtg - the
first 3 patches. It contains also fix/warkaround for the last warn() I'm
able to hit with syzbot repro provided by Christoph.
With this series and https://marc.info/?l=linux-netdev&m=158772695009150&w=2
on top of -net I haven't hit any of the 'known' oops/warn so far, but
I'm hitting a WARN_ONCE("Bad mapping: "...) I'm unsure it's
introduced here or not.
Paolo Abeni (5):
mptcp: add and use wrappers to manipulate mptcp opt flags
mptcp: use a single sub_option field in mptcp_incoming_option
tcp: init MPTCP option before parsing.
mptcp: avoid a WARN on bad input.
mptcp: fix 'use_ack' option access.
include/linux/tcp.h | 27 +++++++++-----------
net/ipv4/tcp_input.c | 1 +
net/mptcp/options.c | 41 +++++++++++++++---------------
net/mptcp/protocol.c | 2 +-
net/mptcp/protocol.h | 60 ++++++++++++++++++++++++++++++++++++++++++++
net/mptcp/subflow.c | 23 ++++++++---------
6 files changed, 105 insertions(+), 49 deletions(-)
--
2.21.1
7:14 p.m.
New subject: [RFC PATCH 1/5] mptcp: add and use wrappers to manipulate mptcp opt flags
While at it, also replace a warning reachable via external
input to warn_on_once and use the mptcp incoming option
init helper instead of open-coded and more costly alternatives.
Other than that, no functional change intented.
This will make next patches more readable.
Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
---
include/linux/tcp.h | 17 ++++++++-----
net/mptcp/options.c | 36 +++++++++++++-------------
net/mptcp/protocol.c | 2 +-
net/mptcp/protocol.h | 60 ++++++++++++++++++++++++++++++++++++++++++++
net/mptcp/subflow.c | 23 ++++++++---------
5 files changed, 101 insertions(+), 37 deletions(-)
diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index 421c99c12291..f24b25e50b30 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -141,13 +141,8 @@ struct tcp_options_received {
#endif
};
-static inline void tcp_clear_options(struct tcp_options_received *rx_opt)
+static inline void mptcp_init_options(struct tcp_options_received *rx_opt)
{
- rx_opt->tstamp_ok = rx_opt->sack_ok = 0;
- rx_opt->wscale_ok = rx_opt->snd_wscale = 0;
-#if IS_ENABLED(CONFIG_SMC)
- rx_opt->smc_ok = 0;
-#endif
#if IS_ENABLED(CONFIG_MPTCP)
rx_opt->mptcp.mp_capable = 0;
rx_opt->mptcp.mp_join = 0;
@@ -157,6 +152,16 @@ static inline void tcp_clear_options(struct tcp_options_received
*rx_opt)
#endif
}
+static inline void tcp_clear_options(struct tcp_options_received *rx_opt)
+{
+ rx_opt->tstamp_ok = rx_opt->sack_ok = 0;
+ rx_opt->wscale_ok = rx_opt->snd_wscale = 0;
+#if IS_ENABLED(CONFIG_SMC)
+ rx_opt->smc_ok = 0;
+#endif
+ mptcp_init_options(rx_opt);
+}
+
/* This is the max number of SACKS that we'll generate and process. It's safe
* to increase this, although since:
* size = TCPOLEN_SACK_BASE_ALIGNED (4) + n * TCPOLEN_SACK_PERBLOCK (8)
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 4a7c467b99db..e8a7c141836e 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -72,7 +72,7 @@ void mptcp_parse_option(const struct sk_buff *skb, const unsigned char
*ptr,
if (flags & MPTCP_CAP_CHECKSUM_REQD)
break;
- mp_opt->mp_capable = 1;
+ mptcp_option_set_mp_capable(opt_rx);
if (opsize >= TCPOLEN_MPTCP_MPC_SYNACK) {
mp_opt->sndr_key = get_unaligned_be64(ptr);
ptr += 8;
@@ -87,7 +87,7 @@ void mptcp_parse_option(const struct sk_buff *skb, const unsigned char
*ptr,
* equivalent to those in a DSS option and can be used
* interchangeably."
*/
- mp_opt->dss = 1;
+ mptcp_option_set_dss(opt_rx);
mp_opt->use_map = 1;
mp_opt->mpc_map = 1;
mp_opt->data_len = get_unaligned_be16(ptr);
@@ -99,7 +99,6 @@ void mptcp_parse_option(const struct sk_buff *skb, const unsigned char
*ptr,
break;
case MPTCPOPT_MP_JOIN:
- mp_opt->mp_join = 1;
if (opsize == TCPOLEN_MPTCP_MPJ_SYN) {
mp_opt->backup = *ptr++ & MPTCPOPT_BACKUP;
mp_opt->join_id = *ptr++;
@@ -125,9 +124,10 @@ void mptcp_parse_option(const struct sk_buff *skb, const unsigned
char *ptr,
memcpy(mp_opt->hmac, ptr, MPTCPOPT_HMAC_LEN);
pr_debug("MP_JOIN hmac");
} else {
- pr_warn("MP_JOIN bad option size");
- mp_opt->mp_join = 0;
+ pr_warn_once("MP_JOIN bad option size");
+ break;
}
+ mptcp_option_set_mp_join(opt_rx);
break;
case MPTCPOPT_DSS:
@@ -176,7 +176,7 @@ void mptcp_parse_option(const struct sk_buff *skb, const unsigned char
*ptr,
opsize != expected_opsize + TCPOLEN_MPTCP_DSS_CHECKSUM)
break;
- mp_opt->dss = 1;
+ mptcp_option_set_dss(opt_rx);
if (mp_opt->use_ack) {
if (mp_opt->ack64) {
@@ -238,7 +238,7 @@ void mptcp_parse_option(const struct sk_buff *skb, const unsigned char
*ptr,
break;
}
- mp_opt->add_addr = 1;
+ mptcp_option_set_add_addr(opt_rx);
mp_opt->port = 0;
mp_opt->addr_id = *ptr++;
pr_debug("ADD_ADDR: id=%d", mp_opt->addr_id);
@@ -272,7 +272,7 @@ void mptcp_parse_option(const struct sk_buff *skb, const unsigned char
*ptr,
if (opsize != TCPOLEN_MPTCP_RM_ADDR_BASE)
break;
- mp_opt->rm_addr = 1;
+ mptcp_option_set_rm_addr(opt_rx);
mp_opt->rm_id = *ptr++;
pr_debug("RM_ADDR: id=%d", mp_opt->rm_id);
break;
@@ -349,13 +349,13 @@ void mptcp_rcv_synsent(struct sock *sk)
struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
struct tcp_sock *tp = tcp_sk(sk);
- if (subflow->request_mptcp && tp->rx_opt.mptcp.mp_capable) {
+ if (subflow->request_mptcp && mptcp_option_mp_capable(&tp->rx_opt)) {
subflow->mp_capable = 1;
subflow->can_ack = 1;
subflow->remote_key = tp->rx_opt.mptcp.sndr_key;
pr_debug("subflow=%p, remote_key=%llu", subflow,
subflow->remote_key);
- } else if (subflow->request_join && tp->rx_opt.mptcp.mp_join) {
+ } else if (subflow->request_join && mptcp_option_mp_join(&tp->rx_opt))
{
subflow->mp_join = 1;
subflow->thmac = tp->rx_opt.mptcp.thmac;
subflow->remote_nonce = tp->rx_opt.mptcp.nonce;
@@ -685,7 +685,7 @@ bool mptcp_synack_options(const struct request_sock *req, unsigned int
*size,
static bool check_fully_established(struct mptcp_sock *msk, struct sock *sk,
struct mptcp_subflow_context *subflow,
struct sk_buff *skb,
- struct mptcp_options_received *mp_opt)
+ struct tcp_options_received *rx_opt)
{
/* here we can process OoO, in-window pkts, only in-sequence 4th ack
* will make the subflow fully established
@@ -697,7 +697,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct
sock *sk,
*/
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->mp_join &&
+ subflow->mp_join && mptcp_option_mp_join(rx_opt) &&
READ_ONCE(msk->pm.server_side))
tcp_send_ack(sk);
goto fully_established;
@@ -709,7 +709,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct
sock *sk,
if (TCP_SKB_CB(skb)->seq != subflow->ssn_offset + 1)
return subflow->mp_capable;
- if (mp_opt->use_ack) {
+ if (rx_opt->mptcp.use_ack) {
/* subflows are fully established as soon as we get any
* additional ack.
*/
@@ -722,14 +722,14 @@ static bool check_fully_established(struct mptcp_sock *msk, struct
sock *sk,
/* If the first established packet does not contain MP_CAPABLE + data
* then fallback to TCP
*/
- if (!mp_opt->mp_capable) {
+ if (!mptcp_option_mp_capable(rx_opt)) {
subflow->mp_capable = 0;
tcp_sk(sk)->is_mptcp = 0;
return false;
}
subflow->fully_established = 1;
- subflow->remote_key = mp_opt->sndr_key;
+ subflow->remote_key = rx_opt->mptcp.sndr_key;
subflow->can_ack = 1;
fully_established:
@@ -823,10 +823,10 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb,
struct mptcp_ext *mpext;
mp_opt = &opt_rx->mptcp;
- if (!check_fully_established(msk, sk, subflow, skb, mp_opt))
+ if (!check_fully_established(msk, sk, subflow, skb, opt_rx))
return;
- if (mp_opt->add_addr && add_addr_hmac_valid(msk, mp_opt)) {
+ if (mptcp_option_add_addr(opt_rx) && add_addr_hmac_valid(msk, mp_opt)) {
struct mptcp_addr_info addr;
addr.port = htons(mp_opt->port);
@@ -846,7 +846,7 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb,
mp_opt->add_addr = 0;
}
- if (!mp_opt->dss)
+ if (!mptcp_option_dss(opt_rx))
return;
/* we can't wait for recvmsg() to update the ack_seq, otherwise
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 7104504dc8b7..c08fe7ae58ce 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1381,7 +1381,7 @@ struct sock *mptcp_sk_clone(const struct sock *sk,
msk->write_seq = subflow_req->idsn + 1;
atomic64_set(&msk->snd_una, msk->write_seq);
- if (opt_rx->mptcp.mp_capable) {
+ if (mptcp_option_mp_capable(opt_rx)) {
msk->can_ack = true;
msk->remote_key = opt_rx->mptcp.sndr_key;
mptcp_crypto_key_sha(msk->remote_key, NULL, &ack_seq);
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index a2b3048037d0..451315c0dfe3 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -97,6 +97,66 @@ static inline __be32 mptcp_option(u8 subopt, u8 len, u8 nib, u8 field)
((nib & 0xF) << 8) | field);
}
+static inline void
+mptcp_option_set_mp_capable(struct tcp_options_received *opt_rx)
+{
+ opt_rx->mptcp.mp_capable = 1;
+}
+
+static inline void
+mptcp_option_set_mp_join(struct tcp_options_received *opt_rx)
+{
+ opt_rx->mptcp.mp_join =1;
+}
+
+static inline void
+mptcp_option_set_dss(struct tcp_options_received *opt_rx)
+{
+ opt_rx->mptcp.dss = 1;
+}
+
+static inline void
+mptcp_option_set_add_addr(struct tcp_options_received *opt_rx)
+{
+ opt_rx->mptcp.add_addr = 1;
+}
+
+static inline void
+mptcp_option_set_rm_addr(struct tcp_options_received *opt_rx)
+{
+ opt_rx->mptcp.rm_addr = 1;
+}
+
+static inline bool
+mptcp_option_mp_capable(const struct tcp_options_received *opt_rx)
+{
+ return opt_rx->mptcp.mp_capable;
+}
+
+static inline bool
+mptcp_option_mp_join(const struct tcp_options_received *opt_rx)
+{
+ return opt_rx->mptcp.mp_join;
+}
+
+static inline bool
+mptcp_option_dss(const struct tcp_options_received *opt_rx)
+{
+ return opt_rx->mptcp.dss;
+}
+
+static inline bool
+mptcp_option_add_addr(const struct tcp_options_received *opt_rx)
+{
+ return opt_rx->mptcp.add_addr;
+}
+
+static inline bool
+mptcp_option_rm_add(const struct tcp_options_received *opt_rx)
+{
+ return opt_rx->mptcp.rm_addr;
+}
+
#define MPTCP_PM_MAX_ADDR 4
struct mptcp_addr_info {
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 2488e011048c..dda6688bb2e4 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -128,7 +128,7 @@ static void subflow_init_req(struct request_sock *req,
pr_debug("subflow_req=%p, listener=%p", subflow_req, listener);
- memset(&rx_opt.mptcp, 0, sizeof(rx_opt.mptcp));
+ mptcp_init_options(&rx_opt);
mptcp_get_options(skb, &rx_opt);
subflow_req->mp_capable = 0;
@@ -142,16 +142,16 @@ static void subflow_init_req(struct request_sock *req,
return;
#endif
- if (rx_opt.mptcp.mp_capable) {
+ if (mptcp_option_mp_capable(&rx_opt)) {
SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MPCAPABLEPASSIVE);
- if (rx_opt.mptcp.mp_join)
+ if (mptcp_option_mp_join(&rx_opt))
return;
- } else if (rx_opt.mptcp.mp_join) {
+ } else if (mptcp_option_mp_join(&rx_opt)) {
SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINSYNRX);
}
- if (rx_opt.mptcp.mp_capable && listener->request_mptcp) {
+ if (mptcp_option_mp_capable(&rx_opt) && listener->request_mptcp) {
int err;
err = mptcp_token_new_request(req);
@@ -159,7 +159,7 @@ static void subflow_init_req(struct request_sock *req,
subflow_req->mp_capable = 1;
subflow_req->ssn_offset = TCP_SKB_CB(skb)->seq;
- } else if (rx_opt.mptcp.mp_join && listener->request_mptcp) {
+ } else if (mptcp_option_mp_join(&rx_opt) && listener->request_mptcp) {
subflow_req->ssn_offset = TCP_SKB_CB(skb)->seq;
subflow_req->mp_join = 1;
subflow_req->backup = rx_opt.mptcp.backup;
@@ -403,7 +403,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
pr_debug("listener=%p, req=%p, conn=%p", listener, req, listener->conn);
- opt_rx.mptcp.mp_capable = 0;
+ mptcp_init_options(&opt_rx);
if (tcp_rsk(req)->is_mptcp == 0)
goto create_child;
@@ -419,7 +419,7 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
}
mptcp_get_options(skb, &opt_rx);
- if (!opt_rx.mptcp.mp_capable) {
+ if (!mptcp_option_mp_capable(&opt_rx)) {
fallback = true;
goto create_child;
}
@@ -430,9 +430,8 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
fallback = true;
} else if (subflow_req->mp_join) {
fallback_is_fatal = true;
- opt_rx.mptcp.mp_join = 0;
mptcp_get_options(skb, &opt_rx);
- if (!opt_rx.mptcp.mp_join ||
+ if (!mptcp_option_mp_join(&opt_rx) ||
!subflow_hmac_valid(req, &opt_rx)) {
SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINACKMAC);
return NULL;
@@ -474,8 +473,8 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
* mpc option
*/
ctx->remote_key = opt_rx.mptcp.sndr_key;
- ctx->fully_established = opt_rx.mptcp.mp_capable;
- ctx->can_ack = opt_rx.mptcp.mp_capable;
+ ctx->can_ack = mptcp_option_mp_capable(&opt_rx);
+ ctx->fully_established = ctx->can_ack;
} else if (ctx->mp_join) {
struct mptcp_sock *owner;
--
2.21.1
7:14 p.m.
New subject: [RFC PATCH 2/5] mptcp: use a single sub_option field in mptcp_incoming_option
So that we can clear all the flags with a single assignment.
No functional change intended, this will just make next patch
more readable.
Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
---
include/linux/tcp.h | 19 ++++++-------------
net/mptcp/options.c | 2 +-
net/mptcp/protocol.h | 20 ++++++++++----------
3 files changed, 17 insertions(+), 24 deletions(-)
diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index f24b25e50b30..b2b34db4f424 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -86,14 +86,11 @@ struct mptcp_options_received {
u64 data_seq;
u32 subflow_seq;
u16 data_len;
- u16 mp_capable : 1,
- mp_join : 1,
- dss : 1,
- add_addr : 1,
- rm_addr : 1,
- family : 4,
+ u8 sub_options;
+ u8 family: 4,
echo : 1,
- backup : 1;
+ backup : 1,
+ __unused : 2;
u32 token;
u32 nonce;
u64 thmac;
@@ -105,7 +102,7 @@ struct mptcp_options_received {
use_ack:1,
ack64:1,
mpc_map:1,
- __unused:2;
+ __unused2:2;
u8 addr_id;
u8 rm_id;
union {
@@ -144,11 +141,7 @@ struct tcp_options_received {
static inline void mptcp_init_options(struct tcp_options_received *rx_opt)
{
#if IS_ENABLED(CONFIG_MPTCP)
- rx_opt->mptcp.mp_capable = 0;
- rx_opt->mptcp.mp_join = 0;
- rx_opt->mptcp.add_addr = 0;
- rx_opt->mptcp.rm_addr = 0;
- rx_opt->mptcp.dss = 0;
+ rx_opt->mptcp.sub_options = 0;
#endif
}
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index e8a7c141836e..27e2ef63edcb 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -843,7 +843,7 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb,
#endif
if (!mp_opt->echo)
mptcp_pm_add_addr_received(msk, &addr);
- mp_opt->add_addr = 0;
+ mp_opt->sub_options &= ~BIT(MPTCPOPT_ADD_ADDR);
}
if (!mptcp_option_dss(opt_rx))
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 451315c0dfe3..b61373933972 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -100,61 +100,61 @@ static inline __be32 mptcp_option(u8 subopt, u8 len, u8 nib, u8
field)
static inline void
mptcp_option_set_mp_capable(struct tcp_options_received *opt_rx)
{
- opt_rx->mptcp.mp_capable = 1;
+ opt_rx->mptcp.sub_options |= BIT(MPTCPOPT_MP_CAPABLE);
}
static inline void
mptcp_option_set_mp_join(struct tcp_options_received *opt_rx)
{
- opt_rx->mptcp.mp_join =1;
+ opt_rx->mptcp.sub_options |= BIT(MPTCPOPT_MP_JOIN);
}
static inline void
mptcp_option_set_dss(struct tcp_options_received *opt_rx)
{
- opt_rx->mptcp.dss = 1;
+ opt_rx->mptcp.sub_options |= BIT(MPTCPOPT_DSS);
}
static inline void
mptcp_option_set_add_addr(struct tcp_options_received *opt_rx)
{
- opt_rx->mptcp.add_addr = 1;
+ opt_rx->mptcp.sub_options |= BIT(MPTCPOPT_ADD_ADDR);
}
static inline void
mptcp_option_set_rm_addr(struct tcp_options_received *opt_rx)
{
- opt_rx->mptcp.rm_addr = 1;
+ opt_rx->mptcp.sub_options |= BIT(MPTCPOPT_RM_ADDR);
}
static inline bool
mptcp_option_mp_capable(const struct tcp_options_received *opt_rx)
{
- return opt_rx->mptcp.mp_capable;
+ return !!(opt_rx->mptcp.sub_options & BIT(MPTCPOPT_MP_CAPABLE));
}
static inline bool
mptcp_option_mp_join(const struct tcp_options_received *opt_rx)
{
- return opt_rx->mptcp.mp_join;
+ return !!(opt_rx->mptcp.sub_options & BIT(MPTCPOPT_MP_JOIN));
}
static inline bool
mptcp_option_dss(const struct tcp_options_received *opt_rx)
{
- return opt_rx->mptcp.dss;
+ return !!(opt_rx->mptcp.sub_options & BIT(MPTCPOPT_DSS));
}
static inline bool
mptcp_option_add_addr(const struct tcp_options_received *opt_rx)
{
- return opt_rx->mptcp.add_addr;
+ return !!(opt_rx->mptcp.sub_options & BIT(MPTCPOPT_ADD_ADDR));
}
static inline bool
mptcp_option_rm_add(const struct tcp_options_received *opt_rx)
{
- return opt_rx->mptcp.rm_addr;
+ return !!(opt_rx->mptcp.sub_options & BIT(MPTCPOPT_RM_ADDR));
}
#define MPTCP_PM_MAX_ADDR 4
--
2.21.1
7:14 p.m.
New subject: [RFC PATCH 3/5] tcp: init MPTCP option before parsing.
The sub_options field carries per packet information,
The MPTCP option parsing will set the relevant bits according
to the ingress option.
Currently the sub_option field is cleared only on option
reset, so that a sub_option set by e.g. a packet later dropped
due to seq checks, will remain visible on next good packet.
The above allows out-of-windows packets to corrupt the mptcp
stream.
Fix the above clearing the sub_options field before options
parsing.
Additionally avoid explicitly clearing the add_addr sub-option
after processing it - no more needed, as it's now always
cleared before parsing.
Fixes: 648ef4b88673 ("mptcp: Implement MPTCP receive path")
Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
---
include/linux/tcp.h | 1 -
net/ipv4/tcp_input.c | 1 +
net/mptcp/options.c | 1 -
3 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index b2b34db4f424..bc16bb3bcbd1 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -152,7 +152,6 @@ static inline void tcp_clear_options(struct tcp_options_received
*rx_opt)
#if IS_ENABLED(CONFIG_SMC)
rx_opt->smc_ok = 0;
#endif
- mptcp_init_options(rx_opt);
}
/* This is the max number of SACKS that we'll generate and process. It's safe
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index bf4ced9273e8..738a0db29f77 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -3848,6 +3848,7 @@ void tcp_parse_options(const struct net *net,
ptr = (const unsigned char *)(th + 1);
opt_rx->saw_tstamp = 0;
+ mptcp_init_options(opt_rx);
while (length > 0) {
int opcode = *ptr++;
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 27e2ef63edcb..282f12772fe2 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -843,7 +843,6 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb,
#endif
if (!mp_opt->echo)
mptcp_pm_add_addr_received(msk, &addr);
- mp_opt->sub_options &= ~BIT(MPTCPOPT_ADD_ADDR);
}
if (!mptcp_option_dss(opt_rx))
--
2.21.1
7:43 p.m.
New subject: [RFC PATCH 3/5] tcp: init MPTCP option before parsing.
On Fri, 2020-04-24 at 19:14 +0200, Paolo Abeni wrote:
The sub_options field carries per packet information,
The MPTCP option parsing will set the relevant bits according
to the ingress option.
Currently the sub_option field is cleared only on option
reset, so that a sub_option set by e.g. a packet later dropped
due to seq checks, will remain visible on next good packet.
The above allows out-of-windows packets to corrupt the mptcp
stream.
Fix the above clearing the sub_options field before options
parsing.
Additionally avoid explicitly clearing the add_addr sub-option
after processing it - no more needed, as it's now always
cleared before parsing.
Fixes: 648ef4b88673 ("mptcp: Implement MPTCP receive path")
Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
---
include/linux/tcp.h | 1 -
net/ipv4/tcp_input.c | 1 +
net/mptcp/options.c | 1 -
3 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index b2b34db4f424..bc16bb3bcbd1 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -152,7 +152,6 @@ static inline void tcp_clear_options(struct tcp_options_received
*rx_opt)
#if IS_ENABLED(CONFIG_SMC)
rx_opt->smc_ok = 0;
#endif
- mptcp_init_options(rx_opt);
}
/* This is the max number of SACKS that we'll generate and process. It's safe
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index bf4ced9273e8..738a0db29f77 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -3848,6 +3848,7 @@ void tcp_parse_options(const struct net *net,
ptr = (const unsigned char *)(th + 1);
opt_rx->saw_tstamp = 0;
+ mptcp_init_options(opt_rx);
while (length > 0) {
int opcode = *ptr++;
I just noticed the above is not enough: we will get stray mptcp option
values when hitting the tcp_fast_parse_options() code path (no opt in
the rx packet, or only tstamp).
Usually the export branch always set mptcp option, so we do not hit
tcp_fast_parse_options(), but due to another bug - still quite obscure
- the sender does that on some race.
More importantly, a generic MPTCP endpoint may chose to omit the DSS in
some packets.
A possible fix would be adding more mptcp_init_options() in
tcp_fast_parse_options(), but that starts looking very wrong...
As an alternative, I was looking at booking an 'unused' bit in
tcp_skb_cb() but even that will need explicit initialization.
Another option would be calling skb_ext_add() in mptcp_parse_option(),
clear the mptcp option only when the extension is added and using
mptcp_get_ext() in mptcp_incoming_options() to validate the
mptcp_options_received sub options flags.
The last one is probably the more robust alternative, but has the
downside to force ext allocation on syn packet (we currently don't do
that) and possibly try multiple allocation on memory pressure - if
skb_ext_add() fails at option parsing time and the ingress packet has
multiple mptcp options.
Any better option?
Thanks,
Paolo
7:14 p.m.
New subject: [RFC PATCH 4/5] mptcp: avoid a WARN on bad input.
Syzcaller has found a way to trigger the WARN_ON_ONCE condition
in check_fully_established().
The root cause is actually a legit fallback to TCP scenario,
so replace the WARN with a plain message on a more strict
condition.
Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
---
net/mptcp/options.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 282f12772fe2..5a37dbfc8b4e 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -717,8 +717,6 @@ static bool check_fully_established(struct mptcp_sock *msk, struct
sock *sk,
goto fully_established;
}
- WARN_ON_ONCE(subflow->can_ack);
-
/* If the first established packet does not contain MP_CAPABLE + data
* then fallback to TCP
*/
@@ -728,6 +726,8 @@ static bool check_fully_established(struct mptcp_sock *msk, struct
sock *sk,
return false;
}
+ if (unlikely(!READ_ONCE(msk->pm.server_side)))
+ pr_warn_once("bogus mpc option on established client sk");
subflow->fully_established = 1;
subflow->remote_key = rx_opt->mptcp.sndr_key;
subflow->can_ack = 1;
--
2.21.1
7:14 p.m.
New subject: [RFC PATCH 5/5] mptcp: fix 'use_ack' option access.
The mentioned RX option field is initialized only for DSS
packet, we must access it only if 'dss' is set too, or
the subflow will end-up in a bad status, leading to
RFC violations.
Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
---
net/mptcp/options.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 5a37dbfc8b4e..a35aa5c76f90 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -709,7 +709,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct
sock *sk,
if (TCP_SKB_CB(skb)->seq != subflow->ssn_offset + 1)
return subflow->mp_capable;
- if (rx_opt->mptcp.use_ack) {
+ if (mptcp_option_dss(rx_opt) && rx_opt->mptcp.use_ack) {
/* subflows are fully established as soon as we get any
* additional ack.
*/
--
2.21.1
2:03 a.m.
On Fri, 24 Apr 2020, Paolo Abeni wrote:
This is a very early RFC showcasing the fix discussed in yday mtg -
the
first 3 patches. It contains also fix/warkaround for the last warn() I'm
able to hit with syzbot repro provided by Christoph.
With this series and https://marc.info/?l=linux-netdev&m=158772695009150&w=2
on top of -net I haven't hit any of the 'known' oops/warn so far, but
I'm hitting a WARN_ONCE("Bad mapping: "...) I'm unsure it's
introduced here or not.
+1 on the suboption approach and adding mptcp_init_options().
While the accessors are more verbose, I do agree that it makes things more
readable. In a way I think checking/setting the bits directly might make
it more obvious that the changes are minor, but it seems like it would get
merged either way.
--
Mat Martineau
Intel
267
days inactive
270
days old
7 comments
2 participants
participants (2)
-
Mat Martineau -
Paolo Abeni