11 Jun
2022
11 Jun
'22
2:33 p.m.
#### 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(a)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(a)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
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3141
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3141(a)github.com>
11 Jun
11 Jun
4:37 p.m.
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.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3141#issuecomment-1152929521
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3141/c1152929521(a)github.com>
12 Jun
12 Jun
9:25 a.m.
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?
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3141#issuecomment-1153083231
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3141/c1153083231(a)github.com>
7:41 p.m.
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.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3141#issuecomment-1153229221
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3141/c1153229221(a)github.com>
8:29 p.m.
@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.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3141#issuecomment-1153240930
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3141/c1153240930(a)github.com>
3 Aug
3 Aug
1:51 a.m.
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.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3141#issuecomment-1203288622
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3141/c1203288622(a)github.com>
1:37 p.m.
@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.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3141#issuecomment-1203776230
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3141/c1203776230(a)github.com>
4:10 p.m.
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!
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3141#issuecomment-1203929940
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3141/c1203929940(a)github.com>
14 Aug
14 Aug
12:01 p.m.
@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?
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3141#issuecomment-1214315350
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3141/c1214315350(a)github.com>
18 Aug
18 Aug
10:12 a.m.
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)`.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3141#issuecomment-1219119017
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3141/c1219119017(a)github.com>
5:19 p.m.
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.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3141#issuecomment-1219552551
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3141/c1219552551(a)github.com>
21 Aug
21 Aug
12:09 p.m.
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 :)
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3141#issuecomment-1221504222
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3141/c1221504222(a)github.com>
3:55 p.m.
@vanrein Thanks for the correction. New option has been added and documented in devel
cookbook. Close PR.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3141#issuecomment-1221540656
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3141/c1221540656(a)github.com>
3:55 p.m.
Closed #3141.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/3141#event-7227673414
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/3141/issue_event/7227673414(a)github.com>
828
days inactive
899
days old
13 comments
3 participants
participants (3)
-
Henning Westerholt -
sergey-safarov
-
vanrein