| Message ID | 20241007-mpc-hs-port-v2-1-0c9e7827bd0f@kernel.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | mptcp: prevent MPC handshake on port-based signal endpoints | expand |
| Context | Check | Description |
|---|---|---|
| matttbe/build | success | Build and static analysis OK |
| matttbe/checkpatch | warning | total: 0 errors, 2 warnings, 0 checks, 57 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, (I guess you dropped the MPTCP ML from Cc by mistake, re-adding it) On 08/10/2024 08:40, Paolo Abeni wrote: > On 10/7/24 20:22, Matthieu Baerts (NGI0) wrote: >> From: Paolo Abeni <pabeni@redhat.com> >> >> Syzkaller reported a lockdep splat: (...) >> Fixes: 1729cf186d8a ("mptcp: create the listening socket for new port") >> Reported-by: syzbot+f4aacdfef2c6a6529c3e@syzkaller.appspotmail.com >> Closes: https://syzkaller.appspot.com/bug?extid=f4aacdfef2c6a6529c3e >> Cc: Cong Wang <cong.wang@bytedance.com> >> Signed-off-by: Paolo Abeni <pabeni@redhat.com> >> Reviewed-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> > > Please add your co-dev tag instead here. I only cleaned-up comments, etc. without changing the behaviour, I sent the v2 mainly because I had the suggested changes on my side already (when checking if comments could go on one line, checking shellcheck with the other patch, etc.). I can add it on the other patch, because I might have done more modifications changing a bit the behaviour, but I can add my co-dev tag here as well if you prefer. (...) >> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c >> index >> a1e28e1d8b4e39e14bc8f98164013d302d62595c..5958b63f84469b980bb1af472117643baf740713 100644 >> --- a/net/mptcp/subflow.c >> +++ b/net/mptcp/subflow.c >> @@ -132,6 +132,13 @@ static void subflow_add_reset_reason(struct >> sk_buff *skb, u8 reason) >> } >> } >> +static int subflow_reset_req_endp(struct request_sock *req, struct >> sk_buff *skb) > > I used a different name because I was wondering about to re-using this > helper in future patches for other req reset - passing explicitly reason > and mib. I guess we can rename it as needed later. Good point. But yes, probably better to rename it later on if we want something more generic, taking different parameters. > Side note: Jakub still strongly prefers not exceeding the 80 chars per > column. A couple of comments and the above line are now above such > threshold. Mmh, I think we mostly followed the default 100 chars max per line in our code. We don't abuse it though: we use more than 80 to improve the readability, e.g. when the opening parenthesis is "too close" to the end of the line, when each line needs a comment next to, most multilines comments are under 80 chars, etc. In other words, if we want to enforce the max of 80 chars per line, we might uniform that, but I don't think that's worth it. Or do you think we should? In this patch, only the comments are above 80 chars, but they are aligned with the ones just above and below. I can still reduce them I see: /* Prohibited MPC attempted towards a port-based signal endp */ /* Prohibited MPC towards a port-based signal endp */ /* subflow is a listener managed by the in-kernel PM */ /* a listener managed by the kernel PM? */ No hurry, I guess we can send this patch later on. Cheers, Matt
On 10/8/24 11:29, Matthieu Baerts wrote: >> On 10/7/24 20:22, Matthieu Baerts (NGI0) wrote: >> Side note: Jakub still strongly prefers not exceeding the 80 chars per >> column. A couple of comments and the above line are now above such >> threshold. > > Mmh, I think we mostly followed the default 100 chars max per line in > our code. We don't abuse it though: we use more than 80 to improve the > readability, e.g. when the opening parenthesis is "too close" to the end > of the line, when each line needs a comment next to, most multilines > comments are under 80 chars, etc. In other words, if we want to enforce > the max of 80 chars per line, we might uniform that, but I don't think > that's worth it. Or do you think we should? My take is that we should avoid adding more lines exceeding 80 chars, when possible. > > In this patch, only the comments are above 80 chars, but they are > aligned with the ones just above and below. I can still reduce them I see: > > /* Prohibited MPC attempted towards a port-based signal endp */ > /* Prohibited MPC towards a port-based signal endp */ > > /* subflow is a listener managed by the in-kernel PM */ > /* a listener managed by the kernel PM? */ > > No hurry, I guess we can send this patch later on. I would opt for the multi-line comment. /P
On 08/10/2024 16:24, Paolo Abeni wrote: > On 10/8/24 11:29, Matthieu Baerts wrote: >>> On 10/7/24 20:22, Matthieu Baerts (NGI0) wrote: >>> Side note: Jakub still strongly prefers not exceeding the 80 chars per >>> column. A couple of comments and the above line are now above such >>> threshold. >> >> Mmh, I think we mostly followed the default 100 chars max per line in >> our code. We don't abuse it though: we use more than 80 to improve the >> readability, e.g. when the opening parenthesis is "too close" to the end >> of the line, when each line needs a comment next to, most multilines >> comments are under 80 chars, etc. In other words, if we want to enforce >> the max of 80 chars per line, we might uniform that, but I don't think >> that's worth it. Or do you think we should? > > My take is that we should avoid adding more lines exceeding 80 chars, > when possible. OK, I will try to find something to have the new lines under 80 chars, and keep the consistency with the current code style. >> In this patch, only the comments are above 80 chars, but they are >> aligned with the ones just above and below. I can still reduce them I >> see: >> >> /* Prohibited MPC attempted towards a port-based signal endp */ >> /* Prohibited MPC towards a port-based signal endp */ >> >> /* subflow is a listener managed by the in-kernel PM */ >> /* a listener managed by the kernel PM? */ >> >> No hurry, I guess we can send this patch later on. > > I would opt for the multi-line comment. OK! Cheers, Matt
diff --git a/net/mptcp/mib.c b/net/mptcp/mib.c index ad88bd3c58dffed8335eedb43ca6290418e3c4f4..19eb9292bd6093a760b41f98c1774fd2490c48e3 100644 --- a/net/mptcp/mib.c +++ b/net/mptcp/mib.c @@ -17,6 +17,7 @@ static const struct snmp_mib mptcp_snmp_list[] = { SNMP_MIB_ITEM("MPCapableFallbackSYNACK", MPTCP_MIB_MPCAPABLEACTIVEFALLBACK), SNMP_MIB_ITEM("MPCapableSYNTXDrop", MPTCP_MIB_MPCAPABLEACTIVEDROP), SNMP_MIB_ITEM("MPCapableSYNTXDisabled", MPTCP_MIB_MPCAPABLEACTIVEDISABLED), + SNMP_MIB_ITEM("MPCapableEndpAttempt", MPTCP_MIB_MPCAPABLEENDPATTEMPT), SNMP_MIB_ITEM("MPFallbackTokenInit", MPTCP_MIB_TOKENFALLBACKINIT), SNMP_MIB_ITEM("MPTCPRetrans", MPTCP_MIB_RETRANSSEGS), SNMP_MIB_ITEM("MPJoinNoTokenFound", MPTCP_MIB_JOINNOTOKEN), diff --git a/net/mptcp/mib.h b/net/mptcp/mib.h index 3206cdda8bb1067f9a8354fd45deed86b67ac7da..42e21b23009462b93553473a7b02c5e09e561a66 100644 --- a/net/mptcp/mib.h +++ b/net/mptcp/mib.h @@ -12,6 +12,7 @@ enum linux_mptcp_mib_field { MPTCP_MIB_MPCAPABLEACTIVEFALLBACK, /* Client-side fallback during 3-way handshake */ MPTCP_MIB_MPCAPABLEACTIVEDROP, /* Client-side fallback due to a MPC drop */ MPTCP_MIB_MPCAPABLEACTIVEDISABLED, /* Client-side disabled due to past issues */ + MPTCP_MIB_MPCAPABLEENDPATTEMPT, /* Prohibited MPC attempted towards a port-based signal endp */ MPTCP_MIB_TOKENFALLBACKINIT, /* Could not init/allocate token */ MPTCP_MIB_RETRANSSEGS, /* Segments retransmitted at the MPTCP-level */ MPTCP_MIB_JOINNOTOKEN, /* Received MP_JOIN but the token was not found */ diff --git a/net/mptcp/pm_netlink.c b/net/mptcp/pm_netlink.c index c738330ba403a75dae0a47a34abc3b9b65b36655..d37fefbaf34f50c16f6c36d785e064797d5df7ad 100644 --- a/net/mptcp/pm_netlink.c +++ b/net/mptcp/pm_netlink.c @@ -1129,6 +1129,7 @@ static int mptcp_pm_nl_create_listen_socket(struct sock *sk, */ inet_sk_state_store(newsk, TCP_LISTEN); lock_sock(ssk); + WRITE_ONCE(mptcp_subflow_ctx(ssk)->pm_listener, true); err = __inet_listen_sk(ssk, backlog); if (!err) mptcp_event_pm_listener(ssk, MPTCP_EVENT_LISTENER_CREATED); diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index c3942416fa3ab46a0e72dd4aed851a6e716398fc..de5c3275df1757d387b275fe58e4d36f3a7de84c 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -532,6 +532,7 @@ struct mptcp_subflow_context { close_event_done : 1, /* has done the post-closed part */ mpc_drop : 1, /* the MPC option has been dropped in a rtx */ __unused : 9; + bool pm_listener; /* subflow is a listener managed by the in-kernel PM */ bool data_avail; bool scheduled; bool fully_established; /* path validated */ diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c index a1e28e1d8b4e39e14bc8f98164013d302d62595c..5958b63f84469b980bb1af472117643baf740713 100644 --- a/net/mptcp/subflow.c +++ b/net/mptcp/subflow.c @@ -132,6 +132,13 @@ static void subflow_add_reset_reason(struct sk_buff *skb, u8 reason) } } +static int subflow_reset_req_endp(struct request_sock *req, struct sk_buff *skb) +{ + SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MPCAPABLEENDPATTEMPT); + subflow_add_reset_reason(skb, MPTCP_RST_EPROHIBIT); + return -EPERM; +} + /* Init mptcp request socket. * * Returns an error code if a JOIN has failed and a TCP reset @@ -165,6 +172,8 @@ static int subflow_check_req(struct request_sock *req, if (opt_mp_capable) { SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_MPCAPABLEPASSIVE); + if (unlikely(listener->pm_listener)) + return subflow_reset_req_endp(req, skb); if (opt_mp_join) return 0; } else if (opt_mp_join) { @@ -172,6 +181,8 @@ static int subflow_check_req(struct request_sock *req, if (mp_opt.backup) SUBFLOW_REQ_INC_STATS(req, MPTCP_MIB_JOINSYNBACKUPRX); + } else if (unlikely(listener->pm_listener)) { + return subflow_reset_req_endp(req, skb); } if (opt_mp_capable && listener->request_mptcp) {