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

Only send InitAck with Zero Checksum if enabled #702

Closed
wants to merge 1 commit into from

Conversation

Sean-Der
Copy link

@Sean-Der Sean-Der commented Feb 9, 2024

If a Init without a Zero Checksum was received
sctp_send_initiate_ack would respond that Zero Checksum was enabled. This PR updates so we check snd_edmid instead of rcv_edmid when building the InitAck

If a Init without a Zero Checksum was received
sctp_send_initiate_ack would respond that Zero Checksum
was enabled. This PR updates so we check snd_edmid instead of
rcv_edmid when building the InitAck
@Sean-Der
Copy link
Author

Sean-Der commented Feb 9, 2024

@tuexen I believe this is the right thing to do! Currently Pion is failing to negotiate because even if we didn't request Zero Checksum in the Init we are getting the InitAck with one.

@tuexen
Copy link
Member

tuexen commented Apr 14, 2024

@tuexen I believe this is the right thing to do! Currently Pion is failing to negotiate because even if we didn't request Zero Checksum in the Init we are getting the InitAck with one.

Isn't this scenario covered by the following testcase? The received INIT chunk does not contain the ZERO_CHECKSUM_ACCEPTABLE parameter. However, since the feature is locally enabled, the INIT ACK chunk does contain the the ZERO_CHECKSUM_ACCEPTABLE parameter, but the packet has a correct CRC32c, since the peer did not announce that the reception of incorrect zero checksum are acceptable.
I double checked and ran the above test. Here is a dump from tshark:

    1 0.000000000    192.0.2.1 → 192.168.0.1  SCTP 64 0xeb70aeeb INIT 
    2 0.000051291  192.168.0.1 → 192.0.2.1    SCTP 256 0x844f67b9 INIT_ACK 
    3 0.000249500    192.0.2.1 → 192.168.0.1  SCTP 228 0x225b9d19 COOKIE_ECHO 
    4 0.000766916  192.168.0.1 → 192.0.2.1    SCTP 40 0xb992e2c9 COOKIE_ACK 
    5 0.000846833  192.168.0.1 → 192.0.2.1    SCTP 44 0x5b974c1a SHUTDOWN 
    6 0.000889625    192.0.2.1 → 192.168.0.1  SCTP 40 0x00000000 SHUTDOWN_ACK 
    7 0.000895958  192.168.0.1 → 192.0.2.1    SCTP 40 0xf209856f SHUTDOWN_COMPLETE 
    8 0.001061958    192.0.2.1 → 192.168.0.1  SCTP 68 0xb327113b ABORT 

The but-last column is the checksum. So you can see, that the packet containing the INIT ACK chunk has the correct CRC32c, not an incorrect one of zero.

From my point of view, the implementation behaves as expected.

Wouldn't your patch result in sending an ZERO_CHECKSUM_ACCEPTABLE parameter in the INIT ACK chunk only if there was a ZERO_CHECKSUM_ACCEPTABLE in the received INIT chunk? This behavior is not intended. Where do you think it is described in draft-ietf-tsvwg-sctp-zero-checksum-09?

@Sean-Der Sean-Der closed this Apr 16, 2024
@Sean-Der Sean-Der deleted the init-ack-zero-checksum branch April 16, 2024 01:58
@Sean-Der
Copy link
Author

Yep you are right @tuexen! Thank you for taking the time to explain it to me :)

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.

None yet

2 participants