fixed incorrect source IP address selection for the SIP messages sending procedure when TCP transport is used or for UDP with the 'mhomed' setting set as 'mhomed=1'.
<!-- Kamailio Pull Request Template -->
<!-- IMPORTANT: - for detailed contributing guidelines, read: https://github.com/kamailio/kamailio/blob/master/.github/CONTRIBUTING.md - pull requests must be done to master branch, unless they are backports of fixes from master branch to a stable branch - backports to stable branches must be done with 'git cherry-pick -x ...' - code is contributed under BSD for core and main components (tm, sl, auth, tls) - code is contributed GPLv2 or a compatible license for the other components - GPL code is contributed with OpenSSL licensing exception -->
#### Pre-Submission Checklist <!-- Go over all points below, and after creating the PR, tick all the checkboxes that apply --> <!-- All points should be verified, otherwise, read the CONTRIBUTING guidelines from above--> <!-- If you're unsure about any of these, don't hesitate to ask on sr-dev mailing list --> - [x] Commit message has the format required by CONTRIBUTING guide - [ ] Commits are split per component (core, individual modules, libs, utils, ...) - [ ] Each component has a single commit (if not, squash them into one commit) - [ ] 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) - [ ] New feature (non-breaking change which adds new functionality) - [ ] Breaking change (fix or feature that would change existing functionality)
#### Checklist: <!-- Go over all points below, and after creating the PR, tick the checkboxes that apply --> - [ ] PR should be backported to stable branches - [x] Tested changes locally - [x] Related to issue #3486(replace XXXX with an open issue number)
#### Description <!-- Describe your changes in detail --> The current service behavior is as follows: if OS contains several network interfaces and the service has a listening TCP socket (or UDP with 'mhomed=1' setting) on the second (third, fourth and so on) interface, then the outbound SIP messages will be always sent from the first (default) interface. These changes make the outbound SIP messages always going through the network_interface/address, which is taken for listening by service.
You can view, comment on, or merge this pull request online at:
https://github.com/kamailio/kamailio/pull/3490
-- Commit Summary --
* core: fixed wrong network interface selection.
-- File Changes --
M src/core/forward.c (7) M src/core/udp_server.c (37) M src/core/udp_server.h (6) M src/main.c (2)
-- Patch Links --
https://github.com/kamailio/kamailio/pull/3490.patch https://github.com/kamailio/kamailio/pull/3490.diff
There is a note that tcp_set_src_addr() should be used only on init:
- https://github.com/kamailio/kamailio/blob/master/src/core/tcp_main.c#L217
Are you sure it does not have side effects, when there is a mix of forcing and not-forcing send socket?
Is it compatible with the old behaviour when the global parameters tcp_source_ipv4/tcp_source_ipv6 were supposed to be global for all traffic?
@ivanuschak pushed 1 commit.
6323eb264c0d9cee7f1a6fc028bd7453b26f5cc8 core: fixed wrong network interface selection.
_tcp_set_src_addr()_/_udp_set_src_addr()_ function calls have been moved to the _main()_ after _fix_all_socket_list()_ invocation. This place is where service is being initiated and not yet entered into the main loop.
_udp_source_ipv6_/_tcp_source_ipv6_/_udp_source_ipv4_/_tcp_source_ipv4_ are global variables and they are initiated on the service init step and then there are no changes for this variables.
Regarding
Is it compatible with the old behaviour when the global parameters tcp_source_ipv4/tcp_source_ipv6 were supposed to be global for all traffic?
Is there some old behavior, which can be enabled by some option? If yes, then the compatibility with this mode should be kept, right?
I didn't find these variables (_tcp_source_ipv4_/_tcp_source_ipv6_) are initialized somewhere in the source code, but they are used, so it looks like its initialization is needed.
The `tcp_source_ipv4` and `tcp_source_ipv6` are set by config parser:
- https://github.com/kamailio/kamailio/blob/master/src/core/cfg.y#L1203-L1222
Your commit likely is overwriting them.
@miconda commented on this pull request.
@@ -2864,6 +2864,16 @@ int main(int argc, char** argv)
fprintf(stderr, "failed to initialize list addresses\n"); goto error; } + for(struct socket_info* si=udp_listen; si; si=si->next){ + udp_set_src_addr(&si->address); + }
This loop results is setting the global variables to the last IPv4 or IPv6 listen addresses in the list, right?
Same happens with the loop for tcp below.
Yes, you're correct the `tcp_source_ipv4` and `tcp_source_ipv6` are set by config parser. My commit replaces this logic. This replacing is not correct, since these vars are configurable from the config file. I'll remove the logic for TCP.
But I don't see similar logic for UDP, i.e. there is no default IP addresses. Is there similar source IP addresses for UDP?
@ivanuschak commented on this pull request.
@@ -2864,6 +2864,16 @@ int main(int argc, char** argv)
fprintf(stderr, "failed to initialize list addresses\n"); goto error; } + for(struct socket_info* si=udp_listen; si; si=si->next){ + udp_set_src_addr(&si->address); + }
Yes, correct. The loop result is setting the global variables to the last IPv4 or IPv6 listen addresses in the list. The same for UDP. But these loops are not correct since these source IP addresses are set up by config file (at least for TCP), these loops just replace the source IP addresses. I will change it.
I haven't added `tcp_source_ipv4` or `tcp_source_ipv6`, neither used them, so I am not sure what was the main purpose for them. I also don't know why there is no equivalent for udp, maybe one should be introduced. But more or less from the beginning of the project, the rule to reuse the incoming socket for outgoing traffic was in place.
Maybe the next steps should be:
- add also for udp the corresponding core parameters to be set from config - add a new parameter like `proto_source_mode` which when set to 0 (the default) the current behaviour is preserved and when set to 1 the one you want is done, if that is solving issues you are facing
In this way we have backward compatibility by default and you have an option to change that behaviour for your specific needs.
@ivanuschak pushed 1 commit.
d0944d1c9a97d7962dc757a28e24e2dfcbd0746d Fixed wrong network interface selection for SIP messages forwarding.
@ivanuschak pushed 1 commit.
f26d2cd25f9d3ef953f61f84ceae5cb2a6c65d48 Fixed wrong network interface selection for SIP messages forwarding.
@ivanuschak pushed 1 commit.
f7cf3be73dab3eaa9648dd274fcff1a302be2208 core: fixed wrong network interface selection.
@ivanuschak pushed 1 commit.
c62fc6ce9b72048cc2a7b3d2883994bd7226bc5d core: fixed wrong network interface selection.
@ivanuschak pushed 1 commit.
37e4196bc816ce5aa588f6f1ff7e6be7fed457ee core: fixed wrong network interface selection.
@ivanuschak pushed 1 commit.
b6eb3a53f826bb3cc9e68b93308c50f1ecae49a4 core: fixed wrong network interface selection.
@ivanuschak pushed 1 commit.
d42f8e68fa396f7daca74fbce1eb256e76baf040 core: fixed wrong network interface selection.
Implementation was re-worked. Now for _tcp_do_connect_ function if _from_ parameter is null then the sending socket is bound on the first listening address/port, which matches the required address family (IPv4/IPv6). The same approach is implemented for UDP sockets when 'mhomed=1'.
@sergey-safarov commented on this pull request.
+inline static int find_listening_sock_info(
+ int s, union sockaddr_union **from, int type) +{ + struct ip_addr ip; + struct socket_info *si = NULL; + su2ip_addr(&ip, *from); + + si = find_si(&ip, 0, type); + + if(unlikely(si == 0)) { + si = find_sock_info_by_address_family(type, ip.af); + if(si) { + int optval = 1; + su2ip_addr(&ip, &si->su); + *from = &si->su; + if(setsockopt(s, SOL_SOCKET, SO_REUSEADDR, (void *)&optval,
Current implementation requires kernel with `SO_REUSEADDR` and `SO_REUSEPORT`. I have checked sources and found several places where this features also required.
I have a question. Can we relay on statement "all kernels provide `SO_REUSEADDR` and `SO_REUSEPORT`" functionality?
Merged #3490 into master.
-
Daniel-Constantin Mierla -
Ivanuschak Nikolay
-
sergey-safarov