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

set the don't fragment (DF) bit on Windows #3155

Merged
merged 2 commits into from
Apr 21, 2021

Conversation

tobyxdd
Copy link
Contributor

@tobyxdd tobyxdd commented Apr 20, 2021

No description provided.

@codecov
Copy link

codecov bot commented Apr 20, 2021

Codecov Report

Merging #3155 (9903019) into master (0a06396) will decrease coverage by 0.05%.
The diff coverage is 53.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3155      +/-   ##
==========================================
- Coverage   85.41%   85.36%   -0.05%     
==========================================
  Files         131      131              
  Lines        9698     9710      +12     
==========================================
+ Hits         8283     8288       +5     
- Misses       1045     1049       +4     
- Partials      370      373       +3     
Impacted Files Coverage Δ
conn_windows.go 57.14% <53.85%> (-5.36%) ⬇️
internal/utils/rand.go 62.50% <0.00%> (-12.50%) ⬇️

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 0a06396...9903019. Read the comment docs.

@marten-seemann
Copy link
Member

Is there any way we can test this? I realize testing that DF is actually set might be difficult, but at least we should be able to pass a net.UDPConn to newConn and make sure it doesn’t return an error.

@tobyxdd
Copy link
Contributor Author

tobyxdd commented Apr 20, 2021

Not sure what happened to the failed test here but it doesn't look related to my changes

Copy link
Member

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

I haven't checked Wireshark to see if the DF bit is actually set. @tobyxdd, have you?

@tobyxdd
Copy link
Contributor Author

tobyxdd commented Apr 20, 2021

Yes I did

@tobyxdd
Copy link
Contributor Author

tobyxdd commented Apr 20, 2021

image

image

without vs with on Windows

Copy link
Member

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

Thank you @tobyxdd!

@marten-seemann marten-seemann merged commit 84e03e5 into quic-go:master Apr 21, 2021
@marten-seemann
Copy link
Member

@tobyxdd Have you checked if this also works on IPv6? I was just working on an equivalent PR for OSX, and it looks like there are different syscalls for IPv4 and IPv6.

@marten-seemann
Copy link
Member

Or am I misunderstanding something here? IPv6 shouldn't allow any packet fragmentation anyway, but then why is there a unix.IPV6_DONTFRAG (see https://pkg.go.dev/golang.org/x/sys/unix).

@tobyxdd
Copy link
Contributor Author

tobyxdd commented Apr 21, 2021

Just realized that IPv6 does have fragmentation (as an extension header), but only at the source, not routers.

https://en.wikipedia.org/wiki/IPv6_packet#Fragment
https://tools.ietf.org/html/rfc3542
https://blog.apnic.net/2016/05/19/fragmenting-ipv6/

@tobyxdd
Copy link
Contributor Author

tobyxdd commented Apr 21, 2021

So yes I guess we do need to set DF for IPv6 as well (or get local MTU from the system?)

@marten-seemann
Copy link
Member

So yes I guess we do need to set DF for IPv6 as well (or get local MTU from the system?)

I'm not sure. It depends on what happens when we try to send a too large packet. We'd need some way to feed that information back into the MTU discovery code. Getting the local MTU from the system and initializing the MTU discoverer with that seems like the safer option, but I'm not sure it's needed in practice, as our maximum packet size is below 1500 bytes anyway.

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.

2 participants