selftests: simult_flows: hit WARN_ON_ONCE(!mpext)
#227
Assignees
Labels
Comments
|
yep, that is caused by the leak fix attempt. Either: diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index cd690794e22b..0341e89d9634 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1320,7 +1320,7 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
}
}
- if (!can_collapse && !ssk->sk_tx_skb_cache &&
+ if (!ssk->sk_tx_skb_cache &&
!mptcp_alloc_tx_skb(sk, ssk, info->data_lock_held))
return 0;or: diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index cd690794e22b..736b49328c44 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1310,16 +1310,18 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
* SSN association set here
*/
mpext = skb_ext_find(skb, SKB_EXT_MPTCP);
- can_collapse = (info->size_goal - skb->len > 0) &&
- mptcp_skb_can_collapse_to(data_seq, skb, mpext);
- if (!can_collapse) {
+ if (!mptcp_skb_can_collapse_to(data_seq, skb, mpext)) {
TCP_SKB_CB(skb)->eor = 1;
- } else {
- size_bias = skb->len;
- avail_size = info->size_goal - skb->len;
+ goto alloc_skb;
}
+
+ can_collapse = (info->size_goal - skb->len > 0) &&
+ (skb_shinfo(skb)->nr_frags <= sysctl_max_skb_frags);
+ if (can_collapse)
+ avail_size = info->size_goal - skb->len;
}
+alloc_skb:
if (!can_collapse && !ssk->sk_tx_skb_cache &&
!mptcp_alloc_tx_skb(sk, ssk, info->data_lock_held))
return 0;should address the issue. The first option is safer, but removes the allocation benefit brought by the culprit commit. |
|
@pabeni: Thank you for the very quick reply! I can try to reproduce the issue locally and check if your patches address the issue. |
|
@pabeni I have some issues with the second patch: |
|
Please note that the tests are running in a loop for a bit of time on my side with the first patch without issue. |
|
uhm... the 2nd patch was a bit rough. It needs a little more changes. This version: --- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -1290,7 +1290,7 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
bool zero_window_probe = false;
struct mptcp_ext *mpext = NULL;
struct sk_buff *skb, *tail;
- bool can_collapse = false;
+ bool must_collapse = false;
int size_bias = 0;
int avail_size;
size_t ret = 0;
@@ -1310,17 +1310,21 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
* SSN association set here
*/
mpext = skb_ext_find(skb, SKB_EXT_MPTCP);
- can_collapse = (info->size_goal - skb->len > 0) &&
- mptcp_skb_can_collapse_to(data_seq, skb, mpext);
- if (!can_collapse) {
+ if (!mptcp_skb_can_collapse_to(data_seq, skb, mpext)) {
TCP_SKB_CB(skb)->eor = 1;
- } else {
+ goto alloc_skb;
+ }
+
+ must_collapse = (info->size_goal - skb->len > 0) &&
+ (skb_shinfo(skb)->nr_frags < sysctl_max_skb_frags);
+ if (must_collapse) {
size_bias = skb->len;
avail_size = info->size_goal - skb->len;
}
}
- if (!can_collapse && !ssk->sk_tx_skb_cache &&
+alloc_skb:
+ if (!must_collapse && !ssk->sk_tx_skb_cache &&
!mptcp_alloc_tx_skb(sk, ssk, info->data_lock_held))
return 0;
@@ -1353,7 +1357,6 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
if (skb == tail) {
TCP_SKB_CB(tail)->tcp_flags &= ~TCPHDR_PSH;
mpext->data_len += ret;
- WARN_ON_ONCE(!can_collapse);
WARN_ON_ONCE(zero_window_probe);
goto out;
}should be better - at least does not splat here after a few iterations. |
|
@pabeni: thank you for this new version. It looks better indeed! After 18 attempts, one test failed but I guess I was just unlucky: |
|
I did 85 more iterations without issues so I guess I was just unlucky here. |
This is really an instance of issue #137, and a quite unlucky one, since we are off by only 2ms ;) |
|
Fixed thanks to Paolo's patches:
Builds and tests have ran: https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20210824T100358 |
jenkins-tessares
pushed a commit
that referenced
this issue
Jul 20, 2023
Add a big batch of test coverage to assert all aspects of the tcx link API: # ./vmtest.sh -- ./test_progs -t tc_links [...] #225 tc_links_after:OK #226 tc_links_append:OK #227 tc_links_basic:OK #228 tc_links_before:OK #229 tc_links_chain_classic:OK #230 tc_links_dev_cleanup:OK #231 tc_links_invalid:OK #232 tc_links_prepend:OK #233 tc_links_replace:OK #234 tc_links_revision:OK Summary: 10/0 PASSED, 0 SKIPPED, 0 FAILED Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> Link: https://lore.kernel.org/r/20230719140858.13224-9-daniel@iogearbox.net Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Probably caused by 5d8855f ("mptcp: fix possible divide by zero", cc @pabeni ), my CI reported a
WARNwas emitted when runningsimult_flowswith a non debug kernel:We hit this
WARNinmptcp_sendmsg_frag():mptcp_net-next/net/mptcp/protocol.c
Lines 1361 to 1365 in 1c685f6
The text was updated successfully, but these errors were encountered: