Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/linux/netlinkutil/RoutingTable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,11 @@ void RoutingTable::ModifyRouteImpl(const Route& route, Operation action)
int operation = 0;
if (action == Update)
{
// Note: NLM_F_REPLACE matches an existing route by
// (destination prefix, tos, metric) alone - ignoring the next hop / output interface -
// so replacing a route can silently overwrite a *different* interface's route that happens
// to share the same key (e.g. two interfaces with a default route at the same metric),
// deleting the second route.
flags = NLM_F_CREATE | NLM_F_REPLACE;
operation = RTM_NEWROUTE;
}
Expand Down
33 changes: 13 additions & 20 deletions src/windows/service/exe/WslMirroredNetworking.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1396,24 +1396,26 @@ _Check_return_ bool wsl::core::networking::WslMirroredNetworkManager::SyncIpStat
std::optional<HRESULT> hr{};
switch (trackedRoute.SyncStatus)
{
// Use ModifyRequestType::Add rather than ModifyRequestType::Update for PendingUpdate.
// An Update in GNS uses NLM_F_REPLACE and replaces by
// (destination prefix, tos, metric) key alone and would silently overwrite a same-key route
// belonging to a *different* interface (e.g. another interface's default at the same metric).
// An ModifyRequestType::Add plumbs a new route or leaves an existing one in place (EEXIST is ignored),
// which is the desired behavior for PendingUpdate.
//
// The 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.
case PendingAdd:
case PendingUpdate:
hr = SendRouteRequestToGns(endpoint, trackedRoute, hns::ModifyRequestType::Add);
if (FAILED(hr.value()))
{
// try to update it instead if it already exists
hr = SendRouteRequestToGns(endpoint, trackedRoute, hns::ModifyRequestType::Update);
}
break;

case Synced:
if (refreshAllRoutes)
{
hr = SendRouteRequestToGns(endpoint, trackedRoute, hns::ModifyRequestType::Update);
if (FAILED(hr.value()))
{
// try to add it
hr = SendRouteRequestToGns(endpoint, trackedRoute, hns::ModifyRequestType::Add);
}
hr = SendRouteRequestToGns(endpoint, trackedRoute, hns::ModifyRequestType::Add);
if (FAILED(hr.value()))
{
trackedRoute.SyncStatus = PendingUpdate;
Expand All @@ -1422,15 +1424,6 @@ _Check_return_ bool wsl::core::networking::WslMirroredNetworkManager::SyncIpStat
}
break;

case PendingUpdate:
hr = SendRouteRequestToGns(endpoint, trackedRoute, hns::ModifyRequestType::Update);
if (FAILED(hr.value()))
{
// try to add it
hr = SendRouteRequestToGns(endpoint, trackedRoute, hns::ModifyRequestType::Add);
}
break;

case PendingRemoval:
// This route is still slated for removal, which we'll try again later.
continue;
Expand Down