issue: 3382805 Fixing incorrect handling of src field of IPv4 routing #126
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Instead of using src address of the routing resolution result to identify interface, especially in IPv4 multicast case, use the if_index of the routing result. Signed-off-by: Alexander Grissik <agrissik@nvidia.com>
In case of IPv4 bind to any ip avoid trying to register dst_entry as observer with routing src field which should be used for outgoing address selection and not for routing rule decisions. What was happening: If we found routing for any IP, we looked if that routing has src-ip field. In case of IPv4, if routing entry has no src-ip (which is common), we forcefully set it to the first address of the device related to this routing entry. And so, we go and look for another routing entry by giving now the src field of the routing we found. That src field affects the routing rule selection. Appart the fact this is not hte goal of the src routing field, in most cases it ends up with the same device we dicovered at first place. Signed-off-by: Alexander Grissik <agrissik@nvidia.com>
Route entry src field should be used for src addr selection algorithm. Forcing src addr based on interface IPs may lead to wrong src addr selection. Signed-off-by: Alexander Grissik <agrissik@nvidia.com>
Registering a route entry per device is not used. The entry does not track device up/down events. Signed-off-by: Alexander Grissik <agrissik@nvidia.com>
The m_src_addr field can be used now. No need for special one. Signed-off-by: Alexander Grissik <agrissik@nvidia.com>
pasis
reviewed
Apr 6, 2024
| } | ||
|
|
||
| si_udp_logdbg("IPPROTO_IP, %s=%d.%d.%d.%d, mc_if:%d.%d.%d.%d mc_if:%d.%d.%d.%d", |
There was a problem hiding this comment.
Inconsistency between format and arguments.
Also, you can use ip_address object to print pretty to_str() which is IP version independent. You already need to wrap mc_grp with ip_address for resolve_if_ip().
| @@ -1127,31 +1127,16 @@ int sockinfo_udp::setsockopt(int __level, int __optname, __const void *__optval, | |||
| break; | |||
| } | |||
|
|
|||
| // Find local interface IP address | |||
| // Find local if for this MC ADD/DROP | |||
There was a problem hiding this comment.
[suggestion] local interface instead of local if.
Comment on lines
438
to
+442
| rt_mgr_logdbg("dst ip '%s' resolved to if_index: %d, src-addr: %s, gw-addr: %s, " | ||
| "route-mtu: %" PRIu32, | ||
| dst_addr.to_str(family).c_str(), res.if_index, | ||
| res.src.to_str(family).c_str(), res.gw.to_str(family).c_str(), res.mtu); | ||
| p_val->get_src_addr().to_str(family).c_str(), | ||
| p_val->get_gw_addr().to_str(family).c_str(), res.mtu); |
There was a problem hiding this comment.
[suggestion] Mix of res and p_val in the arguments can lead to inconvenient changes in the future. I'd suggest to use a single object consistently (p_val in this case). Not critical.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fix usage of src routing field of IPv4
What
The src field should be used for src address selection only (As done for IPv6) and not as identifying interfaces or routing decisions.
This eliminates incorrect behavior and warnings such as "could not figure out source ip for rtv".
Why ?
Functionality
Change type
What kind of change does this PR introduce?
Check list