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
Path MTU discovery for IPv6 #3141
Conversation
- 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
- 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
Let me know if I have more work to do; as far as I know everything works fine. |
Hell @vanrein |
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
WIth Had it been 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!
|
- also set pmtu_discovery core parameter for IPv6 - based on a patch from Rick van Rein <rick@openfortress.nl> - probably to be extended further
@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. I have kept default behaviour for now, so its disabled by default. 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 See |
Thank you. I am trying to summarize it
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.
1. 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.
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
Yes. But I wonder if there could ever be a reason to use _DO.
4. 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 :)
|
…PV6_PMTUDISC_WANT (GH #3141) - add pmtu_discovery=2 for IPv4 and IPv6 - set IP_PMTUDISC_WANT/IPV6_PMTUDISC_WANT - related to GH #3141 - for IPv4: will fragment a datagram if needed according to the path MTU, or will set the don't-fragment flag otherwise - for IPv6: will fragment a datagram if needed according to the path MTU for IPv6
@vanrein Thanks for the correction. New option has been added and documented in devel cookbook. Close PR. |
Pre-Submission Checklist
in
doc/
subfolder, the README file is autogenerated)Type Of Change
Checklist:
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
,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.