Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mptcp reset option #45

Merged
merged 2 commits into from
Feb 25, 2021

Conversation

fw-strlen
Copy link

This extends packetdrill with support for the mptcp reset option. Also adds test cases for the reset option and updates affected mptcp test cases to expect presence of the mptcp reset option. This needs mptcp-next kernel with mptcp reset option support.

This adds the 'mp_reset' keyword to allow sending packets with TCPRST suboption.

Examples:
1.0 R. 1:1(0) ack 1001 win 264 <mp_reset 1>
1.0 R. 1:1(0) ack 1001 win 264 <mp_reset 1 flags [ flag_t ]>

Signed-off-by: Florian Westphal <fw@strlen.de>
@dcaratti
Copy link
Collaborator

patch LGTM. @matttbe, we might merge this PR first, and then fix mp_reset_multi_v4.pkt to cope with the missing "echoed" ADD_ADDR. WDYT?

@matttbe
Copy link
Member

matttbe commented Feb 23, 2021

@matttbe, we might merge this PR first, and then fix mp_reset_multi_v4.pkt to cope with the missing "echoed" ADD_ADDR. WDYT?

@dcaratti Sounds good to me!
But be careful that for this PR, we need to wait to have the kernel patch in our tree: https://patchwork.ozlabs.org/project/mptcp/patch/20210222124940.23943-1-fw@strlen.de/ (waiting for review)

Note that if you want, we can also rebase this PR 45 on top of 46. You should also have the right to push to the mptcp_reset_option branch on fw-strlen/packetdrill repo ;-)

@matttbe
Copy link
Member

matttbe commented Feb 24, 2021

Hi Florian,

Thank you for these patches!

I hope you don't mind, I just modified the scripts you added to follow the structures of the others: https://github.com/multipath-tcp/packetdrill/compare/26aadab2df2b1ed824f5c0001305158e1c83e069..0d2e6fd606c38c013ffe8bcb28bd4a339611802a

For me, it helps to read and review them but we can of course change that if it is a problem.
(e.g. when we have to deal with multiple src/dst addresses, it starts to be too wide maybe)

mp_reset_multi_v4.pkt - check we can reset a subflow and mptcp
connection is unaffected.

mp_reset_single.pkt - check we can reset single subflow.
Check mp_reset on non-RST packets has no effect.

mp_reset_single_tcp.pkt - check TCP doesn't care about
mp_reset presence.

This also "fixes" existing test cases to expect mp_reset option
in some cases.

Signed-off-by: Florian Westphal <fw@strlen.de>
Copy link
Member

@matttbe matttbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Florian,

Thank you for the patches!

I just did another modification to add --tolerance_usec option missing in one test.
https://github.com/multipath-tcp/packetdrill/compare/0d2e6fd606c38c013ffe8bcb28bd4a339611802a..f24a51364f9c1f0918dde8c050ad9087dbe6e279

The rest looks good to me!

@dcaratti is it also OK for you? Especially the modification of the parser and others :)

Cheers,
Matt

@matttbe matttbe requested review from dcaratti and removed request for dcaratti February 24, 2021 17:50
@matttbe
Copy link
Member

matttbe commented Feb 24, 2021

@dcaratti arf, sorry, I forgot you already said "LGTM" :)

All good then, just have to wait for the kernel patches to be merged first then :)

@dcaratti dcaratti merged commit e762c97 into multipath-tcp:mptcp-net-next Feb 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants