Added default gateway support to the routing table reachability check #11000
Conversation
WalkthroughAdds allow_default_gw control to netlink route lookups and propagates rtm_dst_len into route parsing; fixes memset sizing and conditional route-rule initialization with rollback; adds ucs_netif_is_ipoib(); introduces a TCP device flag to allow default-gateway matching; updates callers to pass allow_default_gw. Changes
Sequence Diagram(s)sequenceDiagram
participant TCP as TCP iface init
participant Sys as ucs_netif_is_ipoib
participant Netlink as ucs_netlink_route_exists
participant Parser as route parser
TCP->>Sys: ucs_netif_is_ipoib(if_name)
alt IPoIB
Sys-->>TCP: true
TCP->>Netlink: ucs_netlink_route_exists(..., allow_default_gw=0)
else not IPoIB
Sys-->>TCP: false
TCP->>Netlink: ucs_netlink_route_exists(..., allow_default_gw=1)
end
Netlink->>Parser: parse route attrs (rtm_dst_len)
alt route is default GW and allow_default_gw==0
Parser-->>Netlink: skip route (trace)
else
Parser-->>Netlink: include route
end
Netlink-->>TCP: route exists result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)src/ucs/sys/netlink.c (1)
🔇 Additional comments (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/ucs/sys/netlink.c (1)
285-319: API compatibility shim for header change (if adopted).If you keep the header refactor suggested earlier, add these impl shims:
-int ucs_netlink_route_exists(int if_index, const struct sockaddr *sa_remote, - int allow_default_gw) +int ucs_netlink_route_exists_ex(int if_index, const struct sockaddr *sa_remote, + int allow_default_gw) { ... return info.found; } +int ucs_netlink_route_exists(int if_index, const struct sockaddr *sa_remote) +{ + return ucs_netlink_route_exists_ex(if_index, sa_remote, 1); +}Also update in-tree call sites that pass the 3rd argument to use
_ex.src/uct/tcp/tcp_iface.c (1)
213-279: Don’t base allow_default_gw solely on the remote flag; include local link layer.If the local NIC is IPoIB but the remote peer is older and didn’t set the flag, we’ll incorrectly allow default GW and may accept unroutable peers. Compute the flag using both local interface and remote flags.
Apply this diff:
- /* Default gateway is not relevant for IPoIB interfaces */ - allow_default_gw = !(tcp_dev_addr->flags & - UCT_TCP_DEVICE_ADDR_FLAG_LINK_LAYER_IB); + /* Default gateway is not relevant for IPoIB on either side */ + { + int local_is_ipoib = ucs_netif_is_ipoib(iface->if_name); + int remote_is_ipoib = !!(tcp_dev_addr->flags & + UCT_TCP_DEVICE_ADDR_FLAG_LINK_LAYER_IB); + allow_default_gw = !(local_is_ipoib || remote_is_ipoib); + } - if (!ucs_netlink_route_exists(ndev_index, - (const struct sockaddr *)&remote_addr, - allow_default_gw)) { + if (!ucs_netlink_route_exists_ex(ndev_index, + (const struct sockaddr *)&remote_addr, + allow_default_gw)) { ... }If you keep the existing 3‑arg API name, drop the
_exrename in the snippet accordingly.
🧹 Nitpick comments (2)
src/ucs/sys/netlink.c (1)
177-197: Unused parameter in ucs_netlink_get_route_info.
rtm_dst_lenis not used in this function; either drop it from the signature or add a comment explaining why it’s threaded through.src/ucs/sys/sys.c (1)
181-194: IPoIB detection implementation LGTM; consider portability comment.Works on Linux via SIOCGIFHWADDR + ARPHRD_INFINIBAND. Add a brief comment noting Linux dependency to align with header guard suggestion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/ucs/sys/netlink.c(8 hunks)src/ucs/sys/netlink.h(1 hunks)src/ucs/sys/sys.c(2 hunks)src/ucs/sys/sys.h(1 hunks)src/uct/ib/base/ib_iface.c(2 hunks)src/uct/tcp/tcp.h(1 hunks)src/uct/tcp/tcp_iface.c(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
src/ucs/sys/sys.h (1)
src/ucs/sys/sys.c (1)
ucs_netif_is_ipoib(181-194)
src/ucs/sys/sys.c (1)
src/ucs/sys/sock.c (1)
ucs_netif_ioctl(75-101)
src/uct/ib/base/ib_iface.c (1)
src/ucs/sys/netlink.c (1)
ucs_netlink_ethernet_device_route_exists(315-319)
src/ucs/sys/netlink.c (1)
src/ucs/sys/sock.c (2)
ucs_sockaddr_set_inet_addr(732-746)ucs_sockaddr_is_same_subnet(969-999)
src/ucs/sys/netlink.h (1)
src/ucs/sys/netlink.c (2)
ucs_netlink_route_exists(285-313)ucs_netlink_ethernet_device_route_exists(315-319)
src/uct/tcp/tcp_iface.c (2)
src/ucs/sys/sys.c (1)
ucs_netif_is_ipoib(181-194)src/ucs/sys/netlink.c (1)
ucs_netlink_route_exists(285-313)
🔇 Additional comments (4)
src/uct/tcp/tcp.h (1)
298-303: New LINK_LAYER_IB flag looks fine; confirm cross-version behavior.Peers running older UCX will ignore unknown bits. Here, reachability depends on this bit in the remote address. If the remote is older (doesn’t set the bit) but either side is actually IPoIB, we may wrongly allow default-GW. See tcp_iface.c fix suggested.
src/uct/ib/base/ib_iface.c (1)
713-726: Switch to ethernet-only route check LGTM.Semantics preserved and clearer intent via
ucs_netlink_ethernet_device_route_exists.src/uct/tcp/tcp_iface.c (2)
142-145: Marking IPoIB in device address LGTM.Flagging
UCT_TCP_DEVICE_ADDR_FLAG_LINK_LAYER_IBduring packing is correct and minimal.
276-279: Verification complete: no legacy 2-arg calls found.All calls to
ucs_netlink_route_existsin the codebase use the 3-argument version. The function definition atsrc/ucs/sys/netlink.h:60-61confirms the current signature requires three arguments. The two existing call sites—src/uct/tcp/tcp_iface.c:276andsrc/ucs/sys/netlink.c:318—both pass the correct number of arguments.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/ucs/sys/netlink.c (1)
192-194: Consider refining validation for default routes.For default routes (
rtm_dst_len == 0),dst_in_addris extracted but never used (lines 236-242 skip address assignment). The validation could be more precise by only requiringdst_in_addrfor non-default routes.Apply this diff to make validation conditional:
- if ((*if_index_p == -1) || (*dst_in_addr == NULL)) { + if (*if_index_p == -1) { + return UCS_ERR_INVALID_PARAM; + } + + if ((rtm_dst_len != 0) && (*dst_in_addr == NULL)) { return UCS_ERR_INVALID_PARAM; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/ucs/sys/netlink.c(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/ucs/sys/netlink.c (1)
src/ucs/sys/sock.c (2)
ucs_sockaddr_set_inet_addr(732-746)ucs_sockaddr_is_same_subnet(969-999)
🔇 Additional comments (5)
src/ucs/sys/netlink.c (5)
32-33: LGTM! Clear field addition.The
allow_default_gwfield is well-named with a clear comment explaining its purpose for controlling default gateway route matching.
210-212: LGTM! Correct parameter passing.Passing
rt_msg->rtm_dst_lenenables the function to distinguish default routes (dst_len == 0) from normal routes.
234-244: Excellent fixes addressing past review comments!Line 234 now correctly uses
sizeof(new_rule->dest)instead of the previously buggysizeof(sizeof(new_rule->dest)).Line 244 now unconditionally sets
subnet_prefix_len = rt_msg->rtm_dst_len, ensuring it's always initialized (0 for default routes, eliminating the nondeterministic read flagged in past reviews).Lines 236-242 correctly skip address assignment for default routes while preserving the error path for non-default routes.
253-283: LGTM! Default gateway filtering logic is correct.The implementation properly identifies default gateways (
subnet_prefix_len == 0), skips them whenallow_default_gwis false with a clear trace message, and uses correct short-circuit evaluation for matching (default GW matches immediately, otherwise check subnet).
285-319: LGTM! Clean API extension.The signature change adds necessary
allow_default_gwcontrol. The single call site insrc/uct/tcp/tcp_iface.c:276has been properly updated with the new parameter, and the newucs_netlink_ethernet_device_route_existshelper provides a clear ethernet-specific convenience function.
amastbaum
force-pushed
the
add_default_gateway_support
branch
from
960656e to
d2f227e
Compare
…1019) UCS/SYS: Added default gateway support to the routing table reachability check (#11000) Co-authored-by: amastbaum <[email protected]>
4 participants
What?
Added default gateway support to the routing table reachability check
https://redmine.mellanox.com/issues/4659780
Summary by CodeRabbit
New Features
Improvements
Bug Fixes