#### Pre-Submission Checklist - [X] Commit message has the format required by CONTRIBUTING guide - [X] Commits are split per component (core, individual modules, libs, utils, ...) - [ ] Each component has a single commit (if not, squash them into one commit) - [X] No commits to README files for modules (changes must be done to docbook files in `doc/` subfolder, the README file is autogenerated)
#### Type Of Change - [X] Small bug fix (non-breaking change which fixes an issue) - [X] New feature (non-breaking change which adds new functionality) - [ ] Breaking change (fix or feature that would change existing functionality)
#### Checklist: - [ ] PR should be backported to stable branches (no, it is a requirement to use UDP/IPv6 with everyone) - [X] Tested changes locally (same results as before with the Kamailio test framework) - [ ] Related to issue #3119
#### Description
The IPv6 support for Path MTU discovery is absent, but IPv6 has no DF flag to allow downstream fragmentation. See Issue #3119 for my discovery / learning path. In short, unconnected UDP sockets do not learn Path MTU problems, so a message dropped once will be dropped again on resend. Using `IPV6_PMTUDISC_WANT`, any knowledge in the kernel can be used to fragment the message at sending time, as intended for IPv6.
These patches actually correct two IPv6-related things in the `core/udp_server.c`,
1. Benefit from any kernel knowledge about Path MTU for IPv6 2. The same option to learn from UDP_ERRORS is now available for IPv6
Do note that the last has not been implemented for IPv4 or IPv6. It is more useful for IPv6, allowing instant "tm" resends for Path MTU, but not "sl" I think. I cannot do that work, but this brings IPv6 to the same level as IPv4.
``` commit 0f90cff05c1a448eb2f85f83b4c087ab32ede11 Author: Rick van Rein <rick@openfortress.nl> Date: Sat Jun 11 10:57:32 2022 +0000
core: Issue 3119. Handling ICMPv6 Packet too Big
- This was only defined for IPv4 under flag UDP_ERRORS - IPv6 has no toggle DF to clear for en-route fragmentation - Kamailio currently does not collect with recvmsg(...,MSG_ERRQUEUE) - The benefits of adding that can be instant resends for "tm" messages - Note that "sl" messages will have been forgotten at this point
commit 6b404b5f9807174177bee36eaf3543be0794f55e Author: Rick van Rein <rick@openfortress.nl> Date: Sat Jun 11 10:44:27 2022 +0000
core: Issue #3119. Path MTU kernel info for IPv6
- For IPv4, DF is an option; for IPv6 it is always active - This makes pmtu_discover an IPv4-only option - This means that we should set IPV6_MTU_DISCOVER to IPV6_PMTUDISC_WANT - Unconnected UDP sockets can now learn from ICMPv6 "Packet too Big" - As a result, hitting a Path MTU upper bound is a learning process - This should stop consistent SIP packet drops due to Path MTU ```
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/3141
-- Commit Summary --
* core: Issue #3119. Path MTU kernel info for IPv6 * core: Issue 3119. Handling ICMPv6 Packet too Big
-- File Changes --
M doc/misc/NEWS (2) M src/core/udp_server.c (44)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/3141.patch https://github.com/kamailio/kamailio/pull/3141.diff
1. Additionally tested on my own IPv6 PBX, no problems. 2. A bit worried abou tthe long runtime of LGTM on C/C++ and if that is normal 3. I don't think the open ticks above are a problem
Let me know if I have more work to do; as far as I know everything works fine.
Hell @vanrein I have tested PMTU via IPv4 and found it does not work as expected for me. Is PMTU via IPv4 works for you?
Hi Sergey,
I have tested PMTU via IPv4 and found it does not work as expected for me. Is PMTU via IPv4 works for you?
I don't use IPv4, so I can't tell.
The Path MTU code has always been like this for IPv4, but it was also applied to IPv6 sockets with the socket options for IPv4. And the values for IPv6 were absent.
@sergey-safarov You did not say how it "did not work as expected".
With what I've learnt, I was a bit surprised about the IPv4 original code, which says
``` optval= (pmtu_discovery) ? IP_PMTUDISC_DO : IP_PMTUDISC_DONT; ```
WIth `ptmu_discover != 0` this code sets the DF flag on all IPv4 traffic, and makes it behave like in IPv6 where this is implied. But I am not so sure if it picks up any ICMP messages *Fragmentation needed and DF set* are processed. An unconnected socket has no way of using that.
Had it been `IP_PMTUDISC_WANT`, like I used for IPv6, then at least any such messages would have been learnt by the OS for the route and influence the resend.
I did not want to change the existing code, but given that you do not experience a benefit, this might make sense. It is much less clear than in the case for IPv6, though, so I am uncertain.
Please decide on this Pull Request. I did some *serious research* and removed buggy / overlooked cases for IPv6, while not altering the code for IPv4. Nobody seems to be questioning that, and yet it has stalled?
The original code that I changed was not very good to begin with, but nobody appears to be motivated to fix it for IPv4, which apparently is *good enough*. For IPv6 the current code is *unacceptable* and that is fixed by this Pull Request. So, please accept this as **the best option available** -- the alternative is worse, namely doing nothing and continue with broken support for SIP over IPv6.
Also, this is my first bugfix, I invested seriously by studying Kamailio and exploring the options. I also depend on SIP over IPv6, and generally believe it should be a serious option for all. I have other ideas to extend/improve Kamailio with a few neat modules, but this indecisive handling makes me too nervous to seriously undertake further effort.
@vanrein Thank you for the pull request. It is a bit more complicated one, and it was also not clear to me if the work from your side was already done, as there was an extensive discussion in #3119. Also summer time probably plays a role, with several developers out of the office for some time. I will review it this week and if there are not other comments or objections, probably merge it.
Its always good to send a quick ping if you do not get a reply to a PR for some reasons. Looking forward to other contributions from your side.
Yes, my work in this PR is done, hence the gentle nudge.
The remark about it not working for IPv4 sounds to me like it also would not have worked before the patch.
Thanks for a new round of attention!
Its always good to send a quick ping if you do not get a reply to a PR for some reasons. Looking forward to other contributions from your side.
Excellent, thanks for your openness!
@vanrein I have pushed a commit based on your PR to the core. Now the core setting pmtu_discovery is also set for IPv6 sockets.
In the initial PR you used the option IPV6_PMTUDISC_WANT instead of IPV6_PMTUDISC_DO. Was there a specific reason for that?
Thanks Henning!
The `IPV6_PMTUDISC_DO` documentation says that it sets the Don't Fragment flag, which is absent in IPv6 because it always behaves that way. The kernel will then refuse larger-MTU frames with `EMSGSIZE`; using `IPV6_PMTUDISC_WANT` instead will fragment the frame at Path MTU boundaries. It is doubtful if the `_DO` setting works for IPv4, but it is not the intention of my PR to question code that has been around for very long.
See `ip(7)` which is referenced from `ipv6(7)`.
Thank you. I am trying to summarize it
1. core option pmtu_discovery for IPv4 - exists a long time - sets socket option IP_PMTUDISC_DO - sets do not fragment bit in outgoing packets - kernel will implement PMTU discovery, initial UDP packets might be dropped while the kernel tries to figure out the MTU - if MTU is found, UDP will be delivered and kernel will store the discovered MTU internally - further packets will use this value - this is probably not really efficient, but probably works
2. core option pmtu_discovery for IPv6 - new introduced in this PR - set socket option IP_PMTUDISC_DO - will not set do not fragment bit in outgoing packets - the kernel will refuse larger packets with EMSGSIZE - no discovery will take place, Kamailio will probably just stop sending the packet
3. suggestion in this PR for IPv6 - use IPV6_PMTUDISC_WANT as socket option - will fragment a datagram if needed according to the path MTU for IPv6 - probably could be set by adding a new core option
4. suggestion in this PR for IPv4 - use IP_PMTUDISC_WANT as socket option - will fragment a datagram if needed according to the path MTU, or will set the don't-fragment flag otherwise - probably could be set by adding a new core option
5. Additionally Kamailio provide options to manually manage the MTU with usual sockets - core setting udp_mtu and udp_mtu_try_proto - will try another protocol if MTU exceeded internally
6. Additionally Kamailio provide options to manually manage the MTU with RAW sockets - core option udp4_raw_mtu - will fragment the packets internally
My suggestion would be add a new value **2** to the pmtu_discovery core parameter, which sets then the _WANT socket option for IPv4 and IPv6.
Great summary!
I only respond to what I think is incorrect.
- core option pmtu_discovery for IPv4
- if MTU is found, UDP will be delivered and kernel will store the discovered MTU internally
- further packets will use this value
no, ip(7) says for IP_PMTUDISC_DO
It is the user's responsibility to packetize the data in MTU-sized chunks and to do the retransmits if necessary. The kernel will reject (with EMSGSIZE) datagrams that are bigger than the known path MTU.
unlike what it says for IP_PMTUDISC_WANT
IP_PMTUDISC_WANT will fragment a datagram if needed according to the path MTU, or will set the don't-fragment flag otherwise.
You can use _DO *if* you respond to EMSGSIZE, which Kamailio does not seem to be doing. Otherwise, go for _WANT.
- suggestion in this PR for IPv6
- use IPV6_PMTUDISC_WANT as socket option
- will fragment a datagram if needed according to the path MTU for IPv6
- probably could be set by adding a new core option
Yes. But I wonder if there could ever be a reason to use _DO.
- suggestion in this PR for IPv4
- probably could be set by adding a new core option
Yes. And I do understand that deviation from the past needs an option for IPv4.
My suggestion would be add a new value **2** to the pmtu_discovery core parameter, which sets then the _WANT socket option for IPv4 and IPv6.
That sounds like a good idea.
Thanks for helping to chew through this tough matter :)
@vanrein Thanks for the correction. New option has been added and documented in devel cookbook. Close PR.
Closed #3141.
-
Henning Westerholt -
sergey-safarov
-
vanrein