8290349: IP_DONTFRAGMENT doesn't set DF bit in IPv4 header #9575

djelinski · 2022-07-20T17:19:50Z

This patch partially fixes the issue where IPv6 sockets were fragmenting outgoing IPv4 datagrams even when IP_DONTFRAGMENT flag was set. Specifically, it fixes the issue on Linux and Windows. As far as I could tell, the issue is unfixable on Mac OS X.

All systems have a separate DONTFRAGMENT flag for IPv4 and IPv6. Each flag only affects packets from its address family; if we want to disable fragmentation of both IPv4 and IPv6 packets sent by an IPv6 socket, we need to set both IPv4 and IPv6 flags. This is similar to other already existing options like IP_TOS or IP_MULTICAST_*.

On Mac OS X it's impossible to set an IPv4 socket option on an IPv6 socket; attempting to do so results in an error. This is a known issue with Mac OS X; on that system we return false from Net#shouldSetBothIPv4AndIPv6Options to avoid setting IPv4 options.

As far as I can tell, non-privileged users have no way to check if the DF flag was set or if the packet was fragmented. I implemented a test that attempted to send a large packet over a physical interface and expected SocketException / EMSGSIZE; the test frequently failed for unrelated reasons, so I decided against including it. Loopback interface has infinite MTU on some systems, so can not be used for this test.

Testing performed (with IP_DONTFRAGMENT flag):
Windows 10:
With this patch, IPv4 packets sent from IPv6 socket have the DF flag; sending an IPv4 packet larger than the interface MTU results in EMSGSIZE and no packet is sent. Without this patch, the packet is fragmented and sent without DF flag.
Sending IPv6 packets larger than the interface MTU usually results in EMSGSIZE. It may succeed if the destination address is non-routable.

For other systems I could not capture packets, so I can only report the observed behavior of sending packets.
Windows 2012 and 2016 (IP_MTU_DISCOVER not supported):
With this patch, sending any packet exceeding MTU size fails with EMSGSIZE. Without this patch sending a large IPv6 packet succeeds.
If a packet is sent to a non-routable address, send succeeds, no error is reported.

Linux:
With this patch, sending any packet exceeding MTU size fails with EMSGSIZE. Without this patch, sending large IPv4 packets from IPv6 sockets succeeds.

Mac OS X 12:
Sending an IPv6 packet exceeding MTU size fails with EMSGSIZE. Sending large IPv4 packets from IPv4 sockets also fails with EMSGSIZE. Sending large IPv4 packets from IPv6 sockets succeeds. The patch does not change the observed behavior.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8290349: IP_DONTFRAGMENT doesn't set DF bit in IPv4 header

Reviewers

Michael McMahon (@Michael-Mc-Mahon - Reviewer)
Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9575/head:pull/9575
$ git checkout pull/9575

Update a local copy of the PR:
$ git checkout pull/9575
$ git pull https://git.openjdk.org/jdk pull/9575/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9575

View PR using the GUI difftool:
$ git pr show -t 9575

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9575.diff

bridgekeeper · 2022-07-20T17:20:18Z

👋 Welcome back djelinski! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-07-20T17:24:16Z

@djelinski The following labels will be automatically applied to this pull request:

net
nio

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2022-07-20T19:49:27Z

Webrevs

jaikiran · 2022-07-21T14:46:58Z

src/jdk.net/macosx/native/libextnet/MacOSXSocketOptions.c

+        close(fd);
+        if (rv == -1) {
+            return JNI_FALSE;
+        }
    }
    return JNI_TRUE;


Hello Daniel, if I'm reading this diff correctly, then we have inverted the check on fd. So I suspect this final return now needs to be changed to return JNI_FALSE to account for fd being -1?

Hi Jaikiran, the point of this change was to better support systems where either IPv4 or IPv6 was disabled. I'm not sure if that can even happen on Mac.
The change here is that if we can't create one socket, we don't immediately return false, but instead return the result for the other socket. For example, if creating IPv4 socket fails, we only check if DF is supported on IPv6 socket, and return that as a result. If both socket calls fail, we don't know if the DF flag is supported, and return true (before the patch we would return false in that case). I can modify the patch to return false in that case, or roll back this change entirely if you think it makes no sense.

I think this change makes sense so long as we are sure that if IPv4 or IPv6 is disabled then the right call happens for the other option.

The method will return true if the process is at the limit of open file descriptors. Before this patch the code returned false in that case.
As soon as Mac OS 10 (Catalina) goes out of support, we can remove this code entirely - AFAICT all newer versions support DF.

Michael-Mc-Mahon

Thanks for taking care of this. One small comment. I think we should edit the comments (such as in src/java.base/unix/native/libnio/ch/Net.c, also the windows version) that say the dual set option call is only for multicast options to also include this option.

AlanBateman · 2022-08-01T07:28:28Z

Have you considered changing ExtendedSocketOptions.xxxOption to take a family parameter rather than a boolean as that would be more general and maybe useful in the future?

The change to the comments in src/java.base/unix/native/libnio/ch/Net.c and src/java.base/windows/native/libnio/ch/Net.c are a bit ugl and maybe confusing as there is no support for "dontfragment" in java.base. If you really need to change the comments then use the same style as the existing comments and use "IP_DONTFRAGMENT" instead.

djelinski · 2022-08-01T08:40:23Z

Have you considered changing ExtendedSocketOptions.xxxOption to take a family parameter rather than a boolean as that would be more general and maybe useful in the future?

Passing a boolean matches what we're doing in setIntOption0 and gets the job done. At this moment I don't see where passing family would be useful. Can we make that change when it's actually needed?

The change to the comments [...] are a bit ugly

Agree. How about a more specific comment? Something like:

/* Mac OS does not support setting IPv4 options on IPv6 sockets. As a result, some/all IP_* options
 are not applied to IPv4 datagrams sent from IPv6 sockets. */

AlanBateman · 2022-08-02T16:59:20Z

src/jdk.net/windows/native/libextnet/WindowsSocketOptions.c


-    if (family == AF_INET) {
+    if (!isIPv6) {


In passing, it might be easier to read if we change this code to use if (isIPv6) { ... } then { ...}. Not important here to do as part of this PR, just an observation.

AlanBateman · 2022-08-02T17:12:26Z

Passing a boolean matches what we're doing in setIntOption0 and gets the job done. At this moment I don't see where passing family would be useful. Can we make that change when it's actually needed?

Okay, we can change it when we need it.

The change to the comments [...] are a bit ugly

Agree. How about a more specific comment? Something like:
/* Mac OS does not support setting IPv4 options on IPv6 sockets. As a result, some/all IP_* options
 are not applied to IPv4 datagrams sent from IPv6 sockets. */

The first part isn't quite right because socket level options do work, it's just IP level that are problematic. Maybe just drop the changes to Net.c rather than trying to document the macOS bugs.

openjdk · 2022-08-03T12:47:51Z

@djelinski This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8290349: IP_DONTFRAGMENT doesn't set DF bit in IPv4 header

Reviewed-by: michaelm, alanb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 157 new commits pushed to the master branch:

26e5c11: 4890041: Remove TAB and Shift TAB from Popup Menu in Motif Look & Feel
0bc804d: 8291762: Backout JDK-8291757 from jdk/jdk
3493973: Merge
43bb399: 8291757: Remove EA from JDK 19 version string starting with Initial RC promotion B35 on August 11, 2022
4772354: 8291825: java/time/nontestng/java/time/zone/CustomZoneNameTest.java fails if defaultLocale and defaultFormatLocale are different
1a6c83b: 8291798: VMRegImpl::print_on is broken after JDK-8289060
b6b0317: 8290840: Refactor the "os" class
4cfbb3b: Merge
e265b2a: 8290867: Race freeing remembered set segments
13f0f12: 8290370: Convert SymbolPropertyTable to ResourceHashtable
... and 147 more: https://git.openjdk.org/jdk/compare/43c47b1ad7453b4be5ad949d49866de1d911973e...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

Michael-Mc-Mahon

The change is fine. The Net.c comment is not worth arguing over.

djelinski · 2022-08-04T10:51:05Z

/integrate

openjdk · 2022-08-04T10:52:35Z

Going to push as commit ce61eb6.
Since your change was applied there have been 157 commits pushed to the master branch:

26e5c11: 4890041: Remove TAB and Shift TAB from Popup Menu in Motif Look & Feel
0bc804d: 8291762: Backout JDK-8291757 from jdk/jdk
3493973: Merge
43bb399: 8291757: Remove EA from JDK 19 version string starting with Initial RC promotion B35 on August 11, 2022
4772354: 8291825: java/time/nontestng/java/time/zone/CustomZoneNameTest.java fails if defaultLocale and defaultFormatLocale are different
1a6c83b: 8291798: VMRegImpl::print_on is broken after JDK-8289060
b6b0317: 8290840: Refactor the "os" class
4cfbb3b: Merge
e265b2a: 8290867: Race freeing remembered set segments
13f0f12: 8290370: Convert SymbolPropertyTable to ResourceHashtable
... and 147 more: https://git.openjdk.org/jdk/compare/43c47b1ad7453b4be5ad949d49866de1d911973e...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-08-04T10:52:56Z

@djelinski Pushed as commit ce61eb6.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

djelinski added 3 commits July 20, 2022 13:31

Improve dontfragment handling on dual stack sockets

76c4af4

Fix retrieval of dontfragment flag

892ad68

Revert test

e89c87b

openjdk bot changed the title ~~8290349~~ 8290349: IP_DONTFRAGMENT doesn't set DF bit in IPv4 header Jul 20, 2022

openjdk bot added nio nio-dev@openjdk.org net net-dev@openjdk.org labels Jul 20, 2022

djelinski marked this pull request as ready for review July 20, 2022 19:44

openjdk bot added the rfr Pull request is ready for review label Jul 20, 2022

jaikiran reviewed Jul 21, 2022

View reviewed changes

Michael-Mc-Mahon reviewed Jul 25, 2022

View reviewed changes

Add comment about dontfragment

7af4473

Update option description

0789286

AlanBateman reviewed Aug 2, 2022

View reviewed changes

Revert Net.c comment changes

339d6be

AlanBateman approved these changes Aug 3, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 3, 2022

Michael-Mc-Mahon approved these changes Aug 4, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Aug 4, 2022

openjdk bot closed this Aug 4, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 4, 2022

djelinski deleted the dontfragment-v4v6 branch August 4, 2022 10:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8290349: IP_DONTFRAGMENT doesn't set DF bit in IPv4 header #9575

8290349: IP_DONTFRAGMENT doesn't set DF bit in IPv4 header #9575

djelinski commented Jul 20, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jul 20, 2022

openjdk bot commented Jul 20, 2022

mlbridge bot commented Jul 20, 2022 •

edited

Loading

jaikiran Jul 21, 2022

djelinski Jul 21, 2022

Michael-Mc-Mahon Jul 25, 2022

djelinski Aug 1, 2022

Michael-Mc-Mahon left a comment

AlanBateman commented Aug 1, 2022

djelinski commented Aug 1, 2022

AlanBateman Aug 2, 2022

AlanBateman commented Aug 2, 2022

openjdk bot commented Aug 3, 2022 •

edited

Loading

Michael-Mc-Mahon left a comment

djelinski commented Aug 4, 2022

openjdk bot commented Aug 4, 2022

openjdk bot commented Aug 4, 2022

8290349: IP_DONTFRAGMENT doesn't set DF bit in IPv4 header #9575

8290349: IP_DONTFRAGMENT doesn't set DF bit in IPv4 header #9575

Conversation

djelinski commented Jul 20, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jul 20, 2022

openjdk bot commented Jul 20, 2022

mlbridge bot commented Jul 20, 2022 • edited Loading

Webrevs

jaikiran Jul 21, 2022

Choose a reason for hiding this comment

djelinski Jul 21, 2022

Choose a reason for hiding this comment

Michael-Mc-Mahon Jul 25, 2022

Choose a reason for hiding this comment

djelinski Aug 1, 2022

Choose a reason for hiding this comment

Michael-Mc-Mahon left a comment

Choose a reason for hiding this comment

AlanBateman commented Aug 1, 2022

djelinski commented Aug 1, 2022

AlanBateman Aug 2, 2022

Choose a reason for hiding this comment

AlanBateman commented Aug 2, 2022

openjdk bot commented Aug 3, 2022 • edited Loading

Michael-Mc-Mahon left a comment

Choose a reason for hiding this comment

djelinski commented Aug 4, 2022

openjdk bot commented Aug 4, 2022

openjdk bot commented Aug 4, 2022

djelinski commented Jul 20, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Jul 20, 2022 •

edited

Loading

openjdk bot commented Aug 3, 2022 •

edited

Loading