Use the correct source IP when binding multiple IPs #3067

rs · 2021-03-04T17:07:14Z

When the server is listening on multiple interfaces or interfaces with multiple IPs, the outgoing datagrams are sometime delivered with the wrong source IP address.

In order to fix that, each quic connection needs to extract the destination IP (and optionally interface id) of the received datagrams, and set it as source IP (and interface) on the sent datagrams.

On most platforms, this can be done using ancillary data with recvmsg() and sendmsg(). Some of the machinery for this is already there for ECN, this change extends it to read the destination IP info and write it to the outgoing packets.

Fix #1736

Note that this change is highly untested at this point.

codecov · 2021-03-04T17:14:40Z

Codecov Report

Merging #3067 (eb6bdfd) into master (3bce408) will decrease coverage by 0.43%.
The diff coverage is 54.60%.

@@            Coverage Diff             @@
##           master    #3067      +/-   ##
==========================================
- Coverage   85.72%   85.29%   -0.43%     
==========================================
  Files         131      131              
  Lines        9520     9607      +87     
==========================================
+ Hits         8161     8194      +33     
- Misses        990     1044      +54     
  Partials      369      369

Impacted Files	Coverage Δ
packet_handler_map.go	`75.10% <0.00%> (ø)`
send_conn.go	`33.33% <38.46%> (-66.67%)`	⬇️
conn_oob.go	`54.89% <54.89%> (ø)`
server.go	`80.06% <58.33%> (-0.41%)`	⬇️
client.go	`78.33% <100.00%> (ø)`
conn.go	`100.00% <100.00%> (ø)`
session.go	`75.94% <100.00%> (+0.02%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3bce408...eb6bdfd. Read the comment docs.

marten-seemann · 2021-03-05T03:47:55Z

Thank you for this PR! Before I do an in-depth review, 3 questions:

Do you see any problems when using batch reads and writes (see use batch read / writes, if available #2607)? Can we just use the serialized pktinfo in the ipv{4,6}.Message.OOB?
Is there anything we can do about the FreeBSD special casing? We can't test FreeBSD on GitHub Actions (and most other CIs), and I've made the painful experience that everything that's not tested regularly will end up broken rather sooner than later.
What's the reason you're not using the functions in https://github.com/golang/net/blob/e18ecbb05110/ipv4/control_pktinfo.go for encoding / decoding the pktinfo? Is it because they're unexported?

rs · 2021-03-05T08:59:48Z

I see no issue as OOB seems to be supported with this API
I guess we could use x/net/ipv{4,6}
I did not want to add a dep and I had hard time following the code of the lib regarding control messages.

marten-seemann · 2021-03-09T06:26:51Z

Great!
/ 3. Does that allow us to get rid of the special case for FreeBSD? If so, that would be a big win in terms of testing of this change.

rs · 2021-03-09T10:58:29Z

/ 3. Does that allow us to get rid of the special case for FreeBSD? If so, that would be a big win in terms of testing of this change.

Yes but x/net does not support ECN so we will have to use two different ways to deal with OOB. It means we will be parsing OOB twice.

marten-seemann

This looks pretty good to me. I left a few comments on the PR, please have a look.

Test-wise, it would be nice if we could have a unit test, along the lines of the ECN test: https://github.com/lucas-clemente/quic-go/blob/9dcb56b76f21241216e90009f3ed26d82c6c163f/conn_ecn_test.go#L56-L74
Making the unit test platform independent is a good way to test that all the platform-dependent code behaves as expected.

marten-seemann · 2021-03-10T08:54:51Z

conn_oob.go

+	// 	int cmsg_level;
+	// 	int cmsg_type;
+	// }
+	hdrLen := 16


Suggested change

hdrLen := 16

const hdrLen = 16

marten-seemann · 2021-03-10T09:09:51Z

conn_oob.go

+		// 	struct in_addr ipi_addr;     /* Header Destination
+		// 									address */
+		// };
+		oob := make([]byte, hdrLen+12)


What happens to the 4 leftover bytes on FreeBSD?

marten-seemann · 2021-03-10T11:51:28Z

send_conn.go

 }

 func (c *sconn) Write(p []byte) error {
-	_, err := c.PacketConn.WriteTo(p, c.remoteAddr)
+	_, err := c.WritePacket(p, c.remoteAddr, c.info)


Would it make sense to serialize the packetInfo once, instead of doing it for every WritePacket call? This would save a lot of garbage, wouldn't it?

Yes, that's a good point.

We also probably don't need all this machinery in case the server does not listen on ANY.

On freebsd, there is also the limitation that it does not seem to work if ANY v4+v6 is bound, you need to create v4 and v6 listeners separately.

marten-seemann · 2021-03-10T11:55:31Z

send_conn.go

+	return addr
+}
+
+type spconn struct {


I'm not sure I understand the logic here. As far as I can see, the spconn is used by the client, whereas the server would use the sconn. That means we assume that the client's kernel will choose the right interface to send the packet from. Is my reasoning correct?

Can we achieve this by using the same struct though? WritePacket also works if the packetInfo is nil, doesn't it?

Your reasoning is correct, although, using the same struct for client would require the client code to wrap the net.PacketConn to make it a connection in all the client code too.

conn_oob.go

marten-seemann · 2021-03-11T08:58:14Z

Looks like the cross-compilation test is failing on some architectures. Can you please check?
As far as I can see, we're also still missing a unit test, right?

rs · 2021-03-11T10:40:01Z

Yes that’s next on my list. I need to learn this testing framework first.

marten-seemann · 2021-03-13T03:33:33Z

@rs I'm happy to help if you run into any problem, please let me know.
This is a really valuable PR, and I'd like to get it merged as soon as possible.

rs · 2021-03-14T01:05:51Z

@marten-seemann I'm not sure why the keepalive test fails since I rebased.

marten-seemann · 2021-03-14T03:23:56Z

Unit tests LGTM. Don't worry about the timeout test, it's been flaky recently. Will investigate it soon.

Can you please rebase & squash your PR?

rs · 2021-03-15T23:46:25Z

Done

When the server is listening on multiple interfaces or interfaces with multiple IPs, the outgoing datagrams are sometime delivered with the wrong source IP address. In order to fix that, each quic connection needs to extract the destination IP (and optionally interface id) of the received datagrams, and set it as source IP (and interface) on the sent datagrams. On most platforms, this can be done using ancillary data with recvmsg() and sendmsg(). Some of the machinery for this is already there for ECN, this change extends it to read the destination IP info and write it to the outgoing packets. Fix quic-go#1736

marten-seemann

@rs LGTM now. Thank you for your contribution!

rs force-pushed the packetinfo branch 2 times, most recently from efd277f to 8707051 Compare March 4, 2021 18:43

rs force-pushed the packetinfo branch from 8707051 to aac453d Compare March 8, 2021 22:59

marten-seemann reviewed Mar 10, 2021

View reviewed changes

marten-seemann reviewed Mar 11, 2021

View reviewed changes

conn_oob.go Show resolved Hide resolved

rs force-pushed the packetinfo branch from 23c16bf to fed5bb6 Compare March 15, 2021 23:45

rs force-pushed the packetinfo branch from fed5bb6 to eb6bdfd Compare March 15, 2021 23:50

marten-seemann approved these changes Mar 16, 2021

View reviewed changes

marten-seemann merged commit e9ea484 into quic-go:master Mar 16, 2021

aschmahmann mentioned this pull request May 14, 2021

Release v0.9.0 ipfs/kubo#8058

Closed

71 tasks

ydnar mentioned this pull request Jul 12, 2021

wrapConn called twice in server #3228

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the correct source IP when binding multiple IPs #3067

Use the correct source IP when binding multiple IPs #3067

rs commented Mar 4, 2021 •

edited

Loading

codecov bot commented Mar 4, 2021 •

edited

Loading

marten-seemann commented Mar 5, 2021

rs commented Mar 5, 2021

marten-seemann commented Mar 9, 2021 •

edited

Loading

rs commented Mar 9, 2021

marten-seemann left a comment •

edited

Loading

marten-seemann Mar 10, 2021

marten-seemann Mar 10, 2021

rs Mar 11, 2021

marten-seemann Mar 10, 2021

rs Mar 11, 2021

rs Mar 11, 2021

marten-seemann Mar 10, 2021

rs Mar 11, 2021

marten-seemann commented Mar 11, 2021

rs commented Mar 11, 2021

marten-seemann commented Mar 13, 2021

rs commented Mar 14, 2021

marten-seemann commented Mar 14, 2021

rs commented Mar 15, 2021

marten-seemann left a comment

Use the correct source IP when binding multiple IPs #3067

Use the correct source IP when binding multiple IPs #3067

Conversation

rs commented Mar 4, 2021 • edited Loading

codecov bot commented Mar 4, 2021 • edited Loading

Codecov Report

marten-seemann commented Mar 5, 2021

rs commented Mar 5, 2021

marten-seemann commented Mar 9, 2021 • edited Loading

rs commented Mar 9, 2021

marten-seemann left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marten-seemann commented Mar 11, 2021

rs commented Mar 11, 2021

marten-seemann commented Mar 13, 2021

rs commented Mar 14, 2021

marten-seemann commented Mar 14, 2021

rs commented Mar 15, 2021

marten-seemann left a comment

Choose a reason for hiding this comment

rs commented Mar 4, 2021 •

edited

Loading

codecov bot commented Mar 4, 2021 •

edited

Loading

marten-seemann commented Mar 9, 2021 •

edited

Loading

marten-seemann left a comment •

edited

Loading