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

macos: add TCP_FASTOPEN_FORCE_ENABLE #3135

Closed

Conversation

database64128
Copy link

@rustbot
Copy link
Collaborator

rustbot commented Mar 3, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @JohnTitor (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@database64128
Copy link
Author

Did I miss anything? Not sure why the test failed.

@JohnTitor
Copy link
Member

The source you linked seems old, there's no update since 2021. For example, https://github.com/apple-oss-distributions/xnu/blob/5c2921b07a2480ab43ec66f5b9e41cb872bc554f/bsd/netinet/tcp_private.h#L141 shows it's now a private type. But it may be inaccurate as I cannot make sure it's official. Could you check the source directly? I don't have macOS env.

@database64128
Copy link
Author

Could you check the source directly? I don't have macOS env.

I also don't have macOS env. This only came up because I was helping test an iOS app that uses my tfo-go module and it does not reliably utilize TFO. I was able to make TFO consistently work on my iOS 15 & 16 devices by setting TCP_FASTOPEN_FORCE_ENABLE to 1. Tests in my tfo-go repo suggest that this also works on macOS.

@zonyitoo Can you help with this?

@JohnTitor If it's indeed a private type, does that mean it's no longer eligible for inclusion in this project?

@zonyitoo
Copy link
Contributor

zonyitoo commented Mar 4, 2023

I could verify that setsockopt with 0x218 returns 0.

setsockopt(0x6, 0x6, 0x218)		 = 0 0

Client will always try to send TFO cookie even if the remote endpoint doesn't support TFO (python -m SimpleHTTPServer 80):
client_tfo.pcapng.zip

@database64128
Copy link
Author

@zonyitoo Thanks for the pcap file. Can you check the header file on your system to see if it's indeed part of the private API? I think that's also what they meant in their comment:

Could you check the source directly?

@zonyitoo
Copy link
Contributor

zonyitoo commented Mar 4, 2023

Source Path:

/Library/Developer/CommandLineTools/SDKs/MacOSX13.1.sdk/System/Library/Frameworks/Kernel.framework/Versions/A/Headers/netinet/tcp_private.h
#define MPTCP_ALTERNATE_PORT            0x216
#define MPTCP_FORCE_ENABLE              0x217
#define TCP_FASTOPEN_FORCE_ENABLE       0x218
#define MPTCP_EXPECTED_PROGRESS_TARGET  0x219
#define MPTCP_FORCE_VERSION             0x21a

Well, yes, it is part of the private API. Because it doesn't included in:

$ ls /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/netinet/
bootp.h      icmp_var.h   igmp.h       in.h         in_systm.h   ip.h         ip_icmp.h    tcp.h        tcp_seq.h    tcp_var.h    udp.h
icmp6.h      if_ether.h   igmp_var.h   in_pcb.h     in_var.h     ip6.h        ip_var.h     tcp_fsm.h    tcp_timer.h  tcpip.h      udp_var.h

So it is impossible to use it directly from C without adding the Kernel.framework into search path.

@JohnTitor
Copy link
Member

There's no solid policy about the private items, but if it's subject to change in the future or needs additional setup, this crate's stability policy doesn't work well, I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants