Repository navigation
In mirrored mode, handle route update as add instead of replace - #41391
Conversation
FetoiuCatalin
changed the title
There was a problem hiding this comment.
Pull request overview
This draft PR changes Linux route “Update” handling in netlinkutil to avoid inadvertently replacing another interface’s route when multiple routes share the same destination/metric key, and adds a Windows networking test to validate the new behavior.
Changes:
- Update route modification logic to omit
NLM_F_REPLACEfor Update operations (treat Update as “ensure exists” / add-only). - Add a regression test that creates two same-metric default routes on different interfaces and verifies both persist after an Update.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/linux/netlinkutil/RoutingTable.cpp |
Changes Update route flags to avoid NLM_F_REPLACE overwriting another interface’s route. |
test/windows/NetworkTests.cpp |
Adds a WSL2 test validating that updating eth0’s default route does not remove another interface’s same-metric default route. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/linux/netlinkutil/RoutingTable.cpp:103
- Remove the leftover commented-out assignment (
// flags = NLM_F_CREATE;). It’s redundant with the next line and makes it look like the code is mid-edit.
// Note: The EEXIST error is already treated as non-fatal.
// flags = NLM_F_CREATE;
flags = NLM_F_CREATE;
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/windows/service/exe/WslMirroredNetworking.cpp:1415
- In the refresh-all-routes retry path, the code sets
SyncStatus = PendingUpdate, but this PR now treatsPendingUpdateidentically toPendingAdd(both sendModifyRequestType::Add). SettingPendingAddhere would better reflect the actual retry operation and avoid future confusion when debugging route-sync state transitions.
hr = SendRouteRequestToGns(endpoint, trackedRoute, hns::ModifyRequestType::Add);
if (FAILED(hr.value()))
{
trackedRoute.SyncStatus = PendingUpdate;
trackedRoute.SyncRetryCount = TrackedRoute::MaxSyncRetryCount;
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/windows/service/exe/WslMirroredNetworking.cpp:1416
- In the refresh-all-routes retry path, the code now only ever sends
ModifyRequestType::Add, but on failure it setsSyncStatus = PendingUpdate. SincePendingUpdateno longer results in an Update request (it’s handled identically toPendingAdd), keeping the status asPendingUpdateis misleading and makes future debugging harder. Consider usingPendingAddhere (or renaming the enum more broadly) to match the actual behavior.
hr = SendRouteRequestToGns(endpoint, trackedRoute, hns::ModifyRequestType::Add);
if (FAILED(hr.value()))
{
trackedRoute.SyncStatus = PendingUpdate;
trackedRoute.SyncRetryCount = TrackedRoute::MaxSyncRetryCount;
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/windows/service/exe/WslMirroredNetworking.cpp:1403
- Grammar: use "A" (not "An") before "ModifyRequestType::Add".
// An ModifyRequestType::Add plumbs a new route or leaves an existing one in place (EEXIST is ignored),
|
FetoiuCatalin - is this ready to be merged? |
There was a problem hiding this comment.
🟡 Changes recommended
Fix the test gateways so routes are valid on-link, and correct the comment typo.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
src/windows/service/exe/WslMirroredNetworking.cpp:1403
- The newly added comment has a grammatical typo: use “A ModifyRequestType::Add” rather than “An ModifyRequestType::Add.”
// An ModifyRequestType::Add plumbs a new route or leaves an existing one in place (EEXIST is ignored),
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
Summary of the Pull Request
GNS is handling a route update operation using the NLM_F_REPLACE flag (equivalent to ip route replace), which collapses multiple routes that have the same destination, metric, tos, even if they have different next hops or interfaces. E.g. replacing one of the 2 routes below will collapse them into 1.
default via 10.130.174.1 dev eth1 proto kernel metric 25
default via 10.120.186.1 dev eth0 proto kernel metric 25
This was found when adding and removing and adapter on a test windows host VM - if the new adapter happens to have the same default route with same metric as existing one, after removing the new one connectivity will be lost in WSL as the route of the original one will be deleted if an update happens for the route of the new one.
Detailed Description of the Pull Request / Additional comments
The mirrored mode route synchronization logic never attempts an in-place update of a route. Whenever a route change occurs on the host (ProcessRouteChange), we mark for removal the routes that are known to have been synced in Linux but are no longer part of the latest set of host routes, we do not do a diff of the route properties to determine if an in-place update is needed.
Handling an update as an Add (ignoring already exists error, which already happens) is the intended behavior for route updates.
Note: NAT and virtio modes already use just add and remove operations for Routes
Validation Steps Performed
Scenario that found the bug: Add-VMNetworkAdapter/Remove-VMNetworkAdapter on a windows host VM and verifying connectivity
Connecting/disconnecting OpenVPN multiple times and verifying connectivity, which leads to routes being added/deleted multiple times
Existing automated NetworkTests + new tests that validate host route updates are correctly mirrored