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

connect: add support for new TCP Fast Open API on Linux #2056

Closed
wants to merge 1 commit into from
Closed

connect: add support for new TCP Fast Open API on Linux #2056

wants to merge 1 commit into from

Conversation

ghedo
Copy link
Contributor

@ghedo ghedo commented Nov 6, 2017

The new API added in Linux 4.11 only requires setting a socket option
before connecting, without the whole sento() machinery.

Notably, this makes it possible to use TFO with SSL connections on Linux
as well, without the need to mess around with OpenSSL (or whatever other
SSL library) internals.

@ghedo ghedo requested review from bagder and jay November 6, 2017 00:53
Copy link
Member

@jay jay left a comment

Choose a reason for hiding this comment

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

Overall I'm fine with this with the exception of the changes I think should be addressed. @bagder iirc is using sid so he can try this out and will probably have more to add.

lib/connect.c Outdated
@@ -1078,7 +1080,17 @@ static CURLcode singleipconnect(struct connectdata *conn,
rc = connect(sockfd, &addr.sa_addr, addr.addrlen);
}
#endif /* HAVE_BUILTIN_AVAILABLE */
#elif defined(MSG_FASTOPEN) /* Linux */
#elif defined(TCP_FASTOPEN_CONNECT) /* Linux >= 4.11 */
int optval = 1;
Copy link
Member

Choose a reason for hiding this comment

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

we can't have mixed declarations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

lib/connect.c Outdated
int optval = 1;

if(setsockopt(sockfd, IPPROTO_TCP, TCP_FASTOPEN_CONNECT,
(void *)&optval, sizeof(optval)) < 0)
Copy link
Member

Choose a reason for hiding this comment

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

column alignment should be used on continuation lines when possible

if(setsockopt(sockfd, IPPROTO_TCP, TCP_FASTOPEN_CONNECT,
              (void *)&optval, sizeof(optval)) < 0)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@@ -360,7 +364,7 @@ ssize_t Curl_send_plain(struct connectdata *conn, int num,
available. */
pre_receive_plain(conn, num);

#ifdef MSG_FASTOPEN /* Linux */
#if defined(MSG_FASTOPEN) && !defined(TCP_FASTOPEN_CONNECT) /* Linux */
if(conn->bits.tcp_fastopen) {
bytes_written = sendto(sockfd, mem, len, MSG_FASTOPEN,
conn->ip_addr->ai_addr, conn->ip_addr->ai_addrlen);
Copy link
Member

Choose a reason for hiding this comment

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

although not specifically related to your changes i'd like to address how depending on which fastopen is used tcp_fastopen may be set false. For example conn->bits.tcp_fastopen = FALSE; in the line below this line (I can't select it in the reviewer because it's too out of context). Yet Mac TFO and now this new TFO it says true. What concerns me is other code seem to expect it one way or the other such as
https://github.com/curl/curl/blob/curl-7_56_1/lib/connect.c#L675
https://github.com/curl/curl/blob/curl-7_56_1/lib/connect.c#L780
was the intention for tcp_fastopen to always be set false or only set false in certain circumstances?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case tcp_fastopen is set to false because the sendto() logic only needs to be run once per connection. In the other cases (macOS and TCP_FASTOPEN_CONNECT) there is no sendto() logic.

We could set it to false for the other cases immediately after connect(), but I don't think it's necessary.

@jay
Copy link
Member

jay commented Nov 6, 2017

i notice this is failing on test230 "HTTP GET multiply compressed content". I don't have that test, where did that come from?

@monnerat
Copy link
Contributor

monnerat commented Nov 6, 2017

@jay : test230 is from commit dbcced8#diff-c3eb2d696b714d609a3f191d0b60bf94. It checks multiple content encoding. I already had the same problem once while developing this feature, but resubmitting the job was al right. I have tried to make it fail locally (outside travis) without success. I've just found a problem with data length (that can be random) and fixed it. Next travis runs will show if the error occurs again.

@bagder
Copy link
Member

bagder commented Nov 23, 2017

ping @ghedo, would you mind rebasing this and fix the conflicts?

The new API added in Linux 4.11 only requires setting a socket option
before connecting, without the whole sento() machinery.

Notably, this makes it possible to use TFO with SSL connections on Linux
as well, without the need to mess around with OpenSSL (or whatever other
SSL library) internals.
@ghedo
Copy link
Contributor Author

ghedo commented Nov 23, 2017

Done.

@ghedo
Copy link
Contributor Author

ghedo commented Nov 23, 2017

Btw, you can use https://www.ghedini.me to test this, e.g. using tcpdump on the first curl run I see:

21:31:20.761410 IP 192.168.1.170.41002 > marceline.ghedini.me.https: Flags [S], seq 3297935574, win 29200, options [mss 1460,sackOK,TS val 3330159306 ecr 0,nop,wscale 7,tfo  cookiereq,nop,nop], length 0
	0x0000:  4500 0040 382b 4000 4006 b2ab c0a8 01aa  E..@8+@.@.......
	0x0010:  2d4d 6042 a02a 01bb c492 80d6 0000 0000  -M`B.*..........
	0x0020:  b002 7210 726f 0000 0204 05b4 0402 080a  ..r.ro..........
	0x0030:  c67e 32ca 0000 0000 0103 0307 2202 0101  .~2........."...

and then running curl again (once we have received and cached the TFO cookie from the previous connection):

21:31:30.659627 IP 192.168.1.170.41004 > marceline.ghedini.me.https: Flags [S], seq 3177591783:3177592300, win 29200, options [mss 1460,sackOK,TS val 3330169204 ecr 0,nop,wscale 7,tfo  cookie b9ddd27bf21eebcc,nop,nop], length 517
	0x0000:  4500 024d f142 4000 4006 f786 c0a8 01aa  E..M.B@.@.......
	0x0010:  2d4d 6042 a02c 01bb bd66 33e7 0000 0000  -M`B.,...f3.....
	0x0020:  d002 7210 91e9 0000 0204 05b4 0402 080a  ..r.............
	0x0030:  c67e 5974 0000 0000 0103 0307 220a b9dd  .~Yt........"...
	0x0040:  d27b f21e ebcc 0101 1603 0102 0001 0001  .{..............
	0x0050:  fc03 0337 b84d b7e2 ea3d 253e a1b9 4b73  ...7.M...=%>..Ks
	0x0060:  f221 878c 4ce3 7747 9fd2 9f1b 2e12 b0f0  .!..L.wG........
	0x0070:  09c1 9b00 008c c030 c02c c028 c024 c014  .......0.,.(.$..
	0x0080:  c00a 00a5 00a3 00a1 009f 006b 006a 0069  ...........k.j.i
	0x0090:  0068 0039 0038 0037 0036 0088 0087 0086  .h.9.8.7.6......
	0x00a0:  0085 c032 c02e c02a c026 c00f c005 009d  ...2...*.&......
	0x00b0:  003d 0035 0084 c02f c02b c027 c023 c013  .=.5.../.+.'.#..
	0x00c0:  c009 00a4 00a2 00a0 009e 0067 0040 003f  ...........g.@.?
	0x00d0:  003e 0033 0032 0031 0030 009a 0099 0098  .>.3.2.1.0......
	0x00e0:  0097 0045 0044 0043 0042 c031 c02d c029  ...E.D.C.B.1.-.)
	0x00f0:  c025 c00e c004 009c 003c 002f 0096 0041  .%.......<./...A
	0x0100:  00ff 0100 0147 0000 0013 0011 0000 0e77  .....G.........w
	0x0110:  7777 2e67 6865 6469 6e69 2e6d 6500 0b00  ww.ghedini.me...
	0x0120:  0403 0001 0200 0a00 1c00 1a00 1700 1900  ................
	0x0130:  1c00 1b00 1800 1a00 1600 0e00 0d00 0b00  ................
	0x0140:  0c00 0900 0a00 0d00 2000 1e06 0106 0206  ................
	0x0150:  0305 0105 0205 0304 0104 0204 0303 0103  ................
	0x0160:  0203 0302 0102 0202 0300 0f00 0101 3374  ..............3t
	0x0170:  0000 0010 000e 000c 0268 3208 6874 7470  .........h2.http
	0x0180:  2f31 2e31 0015 00c5 0000 0000 0000 0000  /1.1............
	0x0190:  0000 0000 0000 0000 0000 0000 0000 0000  ................
	0x01a0:  0000 0000 0000 0000 0000 0000 0000 0000  ................
	0x01b0:  0000 0000 0000 0000 0000 0000 0000 0000  ................
	0x01c0:  0000 0000 0000 0000 0000 0000 0000 0000  ................
	0x01d0:  0000 0000 0000 0000 0000 0000 0000 0000  ................
	0x01e0:  0000 0000 0000 0000 0000 0000 0000 0000  ................
	0x01f0:  0000 0000 0000 0000 0000 0000 0000 0000  ................
	0x0200:  0000 0000 0000 0000 0000 0000 0000 0000  ................
	0x0210:  0000 0000 0000 0000 0000 0000 0000 0000  ................
	0x0220:  0000 0000 0000 0000 0000 0000 0000 0000  ................
	0x0230:  0000 0000 0000 0000 0000 0000 0000 0000  ................
	0x0240:  0000 0000 0000 0000 0000 0000 00

@bagder bagder closed this in 979b012 Nov 24, 2017
@bagder
Copy link
Member

bagder commented Nov 24, 2017

Thanks!

@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants