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.

Sign up for GitHub

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

Jump to bottom

Add initial coverage for RM_ADDR #52

Open
wants to merge 1 commit into
base: mptcp-net-next
Choose a base branch
from
Open

Add initial coverage for RM_ADDR #52

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions gtests/net/mptcp/remove_addr/remove_addr_client.pkt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// connection initiated by the kernel
--tolerance_usecs=100000
`../common/defaults.sh`

+0 `../common/client.sh`

+0.0 socket(..., SOCK_STREAM, IPPROTO_MPTCP) = 3
+0.0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
+0.0 fcntl(3, F_GETFL) = 0x2 (flags O_RDWR)
+0.0 fcntl(3, F_SETFL, O_RDWR|O_NONBLOCK) = 0

+0.0 connect(3, ..., ...) = -1 EINPROGRESS (Operation now in progress)
+0.0 > S 0:0(0) <mss 1460, sackOK, TS val 100 ecr 0, nop, wscale 8, mpcapable v1 flags[flag_h] nokey>
+0.0 < S. 0:0(0) ack 1 win 65535 <mss 1460, sackOK, TS val 700 ecr 100, nop, wscale 8, mpcapable v1 flags[flag_h] key[skey=2]>
+0.0 > . 1:1(0) ack 1 <nop, nop, TS val 100 ecr 700, mpcapable v1 flags[flag_h] key[ckey, skey]>
Copy link
Member

Choose a reason for hiding this comment

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

we usually align all the ack and win like you did in the other pkt scripts. We can change that if you think it is not interesting to align them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, will do


+0.200 getsockopt(3, SOL_SOCKET, SO_ERROR, [0], [4]) = 0
+0.205 fcntl(3, F_SETFL, O_RDWR) = 0 // set back to blocking

+0.01 write(3, ..., 100) = 100
+0.0 > P. 1:101(100) ack 1 <nop, nop, TS val 305 ecr 700, mpcapable v1 flags[flag_h] key[ckey, skey] mpcdatalen 100, nop, nop>
Copy link
Member

Choose a reason for hiding this comment

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

we usually align the mpcapable part together. If it is OK for you to change :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, will do



// explicit ACK then ADD_ADDR: on some slow env, injecting ADD_ADDR can take
// time, causing the host to retransmit its last data packet. Here we ensure
// this previous packet is acked then we can send the ADD_ADDR
+0.0 < . 1:1(0) ack 101 win 256 <nop, nop, TS val 705 ecr 305, dss dack8=101 dll=0 nocs>
+0.0 < . 1:1(0) ack 101 win 256 <nop, nop, TS val 705 ecr 305, add_address addr[saddr] hmac=auto>

// ADD_ADDR echo (without hmac)
+0.0 > . 101:101(0) ack 1 <nop, nop, TS val 494 ecr 700, add_address addr[saddr] addr_echo>

// remove address
+0.0 < . 1:1(0) ack 101 win 256 <nop, nop, TS val 705 ecr 305, remove_address address_id=[1], dss dack8=101 dll=0 nocs>
Copy link
Member

Choose a reason for hiding this comment

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

should we establish a new SF before and then send the RM_ADDR to see if we close the subflow? (not sure how to check that → counters?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should we establish a new SF before and then send the RM_ADDR to see if we close the subflow? (not sure how to check that → counters?)

@matttbe good point: unless we make sure that the environment does not block the outbound MP_JOIN SYN (resulting from the reception of the ADD_ADDR in line 31), this line potentially races with the creation of a new subflow. Better to test this more common scenario in this case

Copy link
Member

Choose a reason for hiding this comment

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

unless we make sure that the environment does not block the outbound MP_JOIN SYN (resulting from the reception of the ADD_ADDR in line 31), this line potentially races with the creation of a new subflow. Better to test this more common scenario in this case

I'm sorry, I'm not sure to understand :)
If the client is configured with only one IP and can establish new subflows once an ADD_ADDR is received, we can have a SYN+MPJ. Then we can simulate the fact that the server has lost the announced IP (or the original one) and check where data are transferred. We could also check counters or set one subflow as backup.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mattbe, I'm redesigning this on top of the fix for issue #143. re-testing it with a MP_JOIN subflow


+0.0 close(3) = 0
+0.0 > . 101:101(0) ack 1 <nop, nop, TS val 494 ecr 700, dss dack4=1 dsn8=101 ssn=0 dll=1 nocs fin, nop, nop>
28 changes: 28 additions & 0 deletions gtests/net/mptcp/remove_addr/remove_addr_server_v4.pkt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// connection initiated by packetdrill
--tolerance_usecs=100000
`../common/defaults.sh`

+0 `ip mptcp endpoint flush`
+0 `ip mptcp endpoint add 198.51.100.2 id 10 signal`
Copy link
Member

Choose a reason for hiding this comment

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

if we call a script that checks if we are in v4/v6, could we merge both v4 and v6 script?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point. As long as we don't care about matching the value of the address in the ADD_ADDR, that should be do-able. Ideally, it would be nice to to "parametrize" the script so that the number of endpoints is variable: that would allow also writing a script that adds coverage for multi-id RM_ADDR (that seems to be supported by the parser, so it would deserve some coverage as well). @matttbe, WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

yes, I thought about the address but on the other hand, I don't think we check it elsewhere (add_addr). Do we?
If we do, maybe OK to have this small script dedicated to one or the others, at least we check that :-)

If we need to validate more advanced situations, we would use common/server.sh I guess

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if we call a script that checks if we are in v4/v6, could we merge both v4 and v6 script?

with a fix similar done for issue #143 , we can test v4 and v6 using the same script.


+0 socket(..., SOCK_STREAM, IPPROTO_MPTCP) = 3
+0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0

+0 bind(3, ..., ...) = 0
+0 listen(3, 1) = 0
+0 < S 0:0(0) win 32792 <mss 1460, sackOK, nop, nop, nop, wscale 7, mpcapable v1 flags[flag_h] nokey>
+0 > S. 0:0(0) ack 1 <mss 1460, nop, nop, sackOK, nop, wscale 8, mpcapable v1 flags[flag_h] key[skey]>
+0.01 < . 1:1(0) ack 1 win 257 <mpcapable v1 flags[flag_h] key[ckey=2, skey]>
Copy link
Member

Choose a reason for hiding this comment

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

same here for the ack and win alignment with the rest below

+0 accept(3, ..., ...) = 4

// ADD_ADDR is sent directly (>= 5.12), not after first data (< 5.12)
+0 > . 1:1(0) ack 1 <add_address address_id=10 addr[ep=inet_addr("198.51.100.2")] hmac=auto, dss dack4=1 ssn=1 dll=0 nocs>
+0 < . 1:1(0) ack 1 win 257 <add_address address_id=10 addr[ep=inet_addr("198.51.100.2")] addr_echo, dss dack4=1 ssn=1 dll=0 nocs>

// remove address from the server
+0 `ip mptcp endpoint delete id 10`
+0 > . 1:1(0) ack 1 <remove_address address_id=[10], dss dack4=1 nocs>


+0 close(4) = 0
+0 > . 1:1(0) ack 1 <dss dack4=1 dsn8=1 ssn=0 dll=1 nocs fin, nop, nop>
28 changes: 28 additions & 0 deletions gtests/net/mptcp/remove_addr/remove_addr_server_v6.pkt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// connection initiated by packetdrill
--tolerance_usecs=100000
`../common/defaults.sh`

+0 `ip mptcp endpoint flush`
+0 `ip mptcp endpoint add 2001:db8:1::1 id 10 signal`

+0 socket(..., SOCK_STREAM, IPPROTO_MPTCP) = 3
+0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0

+0 bind(3, ..., ...) = 0
+0 listen(3, 1) = 0
+0 < S 0:0(0) win 32792 <mss 1460, sackOK, nop, nop, nop, wscale 7, mpcapable v1 flags[flag_h] nokey>
+0 > S. 0:0(0) ack 1 <mss 1460, nop, nop, sackOK, nop, wscale 8, mpcapable v1 flags[flag_h] key[skey]>
+0.01 < . 1:1(0) ack 1 win 257 <mpcapable v1 flags[flag_h] key[ckey=2, skey]>
Copy link
Member

Choose a reason for hiding this comment

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

same here for the ack and win alignment with the rest below

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, will do

+0 accept(3, ..., ...) = 4

// ADD_ADDR is sent directly (>= 5.12), not after first data (< 5.12)
+0 > . 1:1(0) ack 1 <add_address address_id=10 addr[ep=inet6_addr("2001:db8:1::1")] hmac=auto>
+0 < . 1:1(0) ack 1 win 257 <add_address address_id=10 addr[ep=inet6_addr("2001:db8:1::1")] addr_echo>

// remove address from the server
+0 `ip mptcp endpoint delete id 10`
+0 > . 1:1(0) ack 1 <remove_address address_id=[10], dss dack4=1 nocs>


+0 close(4) = 0
+0 > . 1:1(0) ack 1 <dss dack4=1 dsn8=1 ssn=0 dll=1 nocs fin, nop, nop>