Skip to content

Commit 722fb02

Browse files
FetoiuCatalinCatalin-Emil Fetoiu
FetoiuCatalin
and
Catalin-Emil Fetoiu
authored
Fixes to route mirroring (#40099)
* route fixes from protonvpn testing * add unit test, fix log * ai code review * format with vs 2022 * review * remove optional has_value checks * harden contracts for to, via having or not having values * update IsOnlink check * fix constructor parameters * fix build --------- Co-authored-by: Catalin-Emil Fetoiu <cfetoiu@microsoft.com>
1 parent b61bb85 commit 722fb02

File tree

6 files changed

+123
-22
lines changed

6 files changed

+123
-22
lines changed

‎src/linux/netlinkutil/Route.cpp‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,14 @@
33
#include "Route.h"
44
#include "Utils.h"
55

6-
Route::Route(int family, const std::optional<Address>& via, int dev, bool defaultRoute, const std::optional<Address>& to, int metric) :
7-
family(family), via(via), dev(dev), defaultRoute(defaultRoute), to(to), metric(metric)
6+
Route::Route(int routeFamily, const std::optional<Address>& routeNextHop, int routeInterface, bool isRouteDefault, const std::optional<Address>& routeDestination, int routeMetric) :
7+
family(routeFamily), via(routeNextHop), dev(routeInterface), defaultRoute(isRouteDefault), to(routeDestination), metric(routeMetric)
88
{
9+
// For onlink routes, ensure the via field is empty
10+
if (via.has_value() && ((family == AF_INET && via->Addr() == "0.0.0.0") || (family == AF_INET6 && via->Addr() == "::")))
11+
{
12+
via.reset();
13+
}
914
}
1015

1116
std::ostream& operator<<(std::ostream& out, const Route& route)
@@ -30,7 +35,7 @@ std::ostream& operator<<(std::ostream& out, const Route& route)
3035

3136
bool Route::IsOnlink() const
3237
{
33-
return !via.has_value() || (family == AF_INET && via->Addr() == "0.0.0.0") || (family == AF_INET6 && via->Addr() == "::");
38+
return !via.has_value();
3439
}
3540

3641
bool Route::IsMulticast() const

‎src/linux/netlinkutil/Route.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ struct Route
1414
int metric = 0;
1515
bool isLoopbackRoute = false;
1616

17-
Route(int family, const std::optional<Address>& via, int dev, bool defaultRoute, const std::optional<Address>& to, int metric);
17+
Route(int routeFamily, const std::optional<Address>& routeNextHop, int routeInterface, bool isRouteDefault, const std::optional<Address>& routeDestination, int routeMetric);
1818

1919
bool IsOnlink() const;
2020
bool IsMulticast() const;

‎src/linux/netlinkutil/RoutingTable.cpp‎

Lines changed: 67 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,10 @@ void RoutingTable::ModifyRouteImpl(const Route& route, Operation action)
111111
{
112112
ModifyLoopbackRouteImpl<TAddr>(route, operation, flags);
113113
}
114+
else if (route.defaultRoute && route.IsOnlink())
115+
{
116+
ModifyDefaultLinkLocalRouteImpl<TAddr>(route, operation, flags);
117+
}
114118
else if (route.defaultRoute)
115119
{
116120
ModifyDefaultRouteImpl<TAddr>(route, operation, flags);
@@ -181,7 +185,7 @@ void RoutingTable::ModifyLoopbackRouteImpl(const Route& route, int operation, in
181185
{
182186
if (!route.to.has_value() || !route.via.has_value())
183187
{
184-
throw RuntimeErrorWithSourceLocation(std::format("Loopback route {} missing destination or gateway address", utils::Stringify(route)));
188+
throw RuntimeErrorWithSourceLocation(std::format("Loopback route {} missing destination or next hop", utils::Stringify(route)));
185189
}
186190

187191
struct Message : RouteMessage
@@ -193,8 +197,8 @@ void RoutingTable::ModifyLoopbackRouteImpl(const Route& route, int operation, in
193197

194198
GNS_LOG_INFO(
195199
"SendMessage Route (to {}, via {}), operation ({}), netLinkflags ({})",
196-
route.to.has_value() ? route.to.value().Addr().c_str() : "[empty]",
197-
route.via.has_value() ? route.via.value().Addr().c_str() : "[empty]",
200+
route.to.value().Addr().c_str(),
201+
route.via.value().Addr().c_str(),
198202
RouteOperationToString(operation),
199203
NetLinkFormatFlagsToString(flags).c_str());
200204

@@ -222,20 +226,52 @@ void RoutingTable::ModifyLoopbackRouteImpl(const Route& route, int operation, in
222226

223227
message.route.rtm_flags |= RTNH_F_ONLINK;
224228
GNS_LOG_INFO(
225-
"InitializeAddressAttribute RTA_DST ({}) RTA_GATEWAY ({}) RTA_PRIORITY ([not set])",
229+
"Netlink message configuration: RTA_DST ({}) RTA_GATEWAY ({}) RTA_PRIORITY ([not set])",
226230
route.to.value().Addr().c_str(),
227231
route.via.value().Addr().c_str());
228232
utils::InitializeAddressAttribute<TAddr>(message.to, route.to.value(), RTA_DST);
229233
utils::InitializeAddressAttribute<TAddr>(message.via, route.via.value(), RTA_GATEWAY);
230234
});
231235
}
232236

237+
template <typename TAddr>
238+
void RoutingTable::ModifyDefaultLinkLocalRouteImpl(const Route& route, int operation, int flags)
239+
{
240+
if (route.via.has_value())
241+
{
242+
throw RuntimeErrorWithSourceLocation("Default route has unexpected next hop");
243+
}
244+
if (route.to.has_value())
245+
{
246+
throw RuntimeErrorWithSourceLocation("Default route has unexpected destination address");
247+
}
248+
249+
struct Message : RouteMessage
250+
{
251+
utils::IntegerAttribute metric;
252+
} __attribute__((packed));
253+
254+
GNS_LOG_INFO(
255+
"SendMessage Route (default onlink), operation ({}), netLinkflags ({})",
256+
RouteOperationToString(operation),
257+
NetLinkFormatFlagsToString(flags).c_str());
258+
259+
SendMessage<Message>(route, operation, flags, [&](Message& message) {
260+
GNS_LOG_INFO("Netlink message configuration: RTA_DST ([not set]) RTA_GATEWAY ([not set]), RTA_PRIORITY ({})", route.metric);
261+
utils::InitializeIntegerAttribute(message.metric, route.metric, RTA_PRIORITY);
262+
});
263+
}
264+
233265
template <typename TAddr>
234266
void RoutingTable::ModifyDefaultRouteImpl(const Route& route, int operation, int flags)
235267
{
236268
if (!route.via.has_value())
237269
{
238-
throw RuntimeErrorWithSourceLocation("Default route is missing its gateway address");
270+
throw RuntimeErrorWithSourceLocation("Default route is missing its next hop");
271+
}
272+
if (route.to.has_value())
273+
{
274+
throw RuntimeErrorWithSourceLocation("Default route has unexpected destination address");
239275
}
240276

241277
struct Message : RouteMessage
@@ -246,15 +282,15 @@ void RoutingTable::ModifyDefaultRouteImpl(const Route& route, int operation, int
246282

247283
GNS_LOG_INFO(
248284
"SendMessage Route (to {}, via {}), operation ({}), netLinkflags ({})",
249-
route.to.has_value() ? route.to.value().Addr().c_str() : "[empty]",
250-
route.via.has_value() ? route.via.value().Addr().c_str() : "[empty]",
285+
"[empty]",
286+
route.via.value().Addr().c_str(),
251287
RouteOperationToString(operation),
252288
NetLinkFormatFlagsToString(flags).c_str());
253289

254290
SendMessage<Message>(route, operation, flags, [&](Message& message) {
255291
GNS_LOG_INFO(
256-
"InitializeAddressAttribute RTA_DST ([not set]) RTA_GATEWAY ({}), RTA_PRIORITY ({})",
257-
route.to.has_value() ? route.to.value().Addr().c_str() : "[empty]",
292+
"Netlink message configuration: RTA_DST ([not set]) RTA_GATEWAY ({}), RTA_PRIORITY ({})",
293+
route.via.value().Addr().c_str(),
258294
route.metric);
259295
utils::InitializeAddressAttribute<TAddr>(message.via, route.via.value(), RTA_GATEWAY);
260296
utils::InitializeIntegerAttribute(message.metric, route.metric, RTA_PRIORITY);
@@ -264,6 +300,15 @@ void RoutingTable::ModifyDefaultRouteImpl(const Route& route, int operation, int
264300
template <typename TAddr>
265301
void RoutingTable::ModifyLinkLocalRouteImpl(const Route& route, int operation, int flags)
266302
{
303+
if (!route.to.has_value())
304+
{
305+
throw RuntimeErrorWithSourceLocation("Link-local route is missing its destination address");
306+
}
307+
if (route.via.has_value())
308+
{
309+
throw RuntimeErrorWithSourceLocation("Link-local route has unexpected next hop");
310+
}
311+
267312
struct Message : RouteMessage
268313
{
269314
utils::AddressAttribute<TAddr> to;
@@ -272,15 +317,15 @@ void RoutingTable::ModifyLinkLocalRouteImpl(const Route& route, int operation, i
272317

273318
GNS_LOG_INFO(
274319
"SendMessage Route (to {}, via {}), operation ({}), netLinkflags ({})",
275-
route.to.has_value() ? route.to.value().Addr().c_str() : "[empty]",
276-
route.via.has_value() ? route.via.value().Addr().c_str() : "[empty]",
320+
route.to.value().Addr().c_str(),
321+
"[empty]",
277322
RouteOperationToString(operation),
278323
NetLinkFormatFlagsToString(flags).c_str());
279324

280325
SendMessage<Message>(route, operation, flags, [&](Message& message) {
281326
GNS_LOG_INFO(
282-
"InitializeAddressAttribute RTA_DST ({}) RTA_GATEWAY ([not set]), RTA_PRIORITY ({})",
283-
route.to.has_value() ? route.to.value().Addr().c_str() : "[empty]",
327+
"Netlink message configuration: RTA_DST ({}) RTA_GATEWAY ([not set]), RTA_PRIORITY ({})",
328+
route.to.value().Addr().c_str(),
284329
route.metric);
285330
utils::InitializeAddressAttribute<TAddr>(message.to, route.to.value(), RTA_DST);
286331
utils::InitializeIntegerAttribute(message.metric, route.metric, RTA_PRIORITY);
@@ -294,6 +339,10 @@ void RoutingTable::ModifyOfflinkRouteImpl(const Route& route, int operation, int
294339
{
295340
throw RuntimeErrorWithSourceLocation("Offlink route is missing its next hop");
296341
}
342+
if (!route.to.has_value())
343+
{
344+
throw RuntimeErrorWithSourceLocation("Offlink route is missing its destination address");
345+
}
297346

298347
struct Message : RouteMessage
299348
{
@@ -304,16 +353,16 @@ void RoutingTable::ModifyOfflinkRouteImpl(const Route& route, int operation, int
304353

305354
GNS_LOG_INFO(
306355
"SendMessage Route (to {}, via {}), operation ({}), netLinkflags ({})",
307-
route.to.has_value() ? route.to.value().Addr().c_str() : "[empty]",
308-
route.via.has_value() ? route.via.value().Addr().c_str() : "[empty]",
356+
route.to.value().Addr().c_str(),
357+
route.via.value().Addr().c_str(),
309358
RouteOperationToString(operation),
310359
NetLinkFormatFlagsToString(flags).c_str());
311360

312361
SendMessage<Message>(route, operation, flags, [&](Message& message) {
313362
GNS_LOG_INFO(
314-
"InitializeAddressAttribute RTA_DST ({}) RTA_GATEWAY ({}), RTA_PRIORITY ({})",
315-
route.to.has_value() ? route.to.value().Addr().c_str() : "[empty]",
316-
route.via.has_value() ? route.via.value().Addr().c_str() : "[empty]",
363+
"Netlink message configuration: RTA_DST ({}) RTA_GATEWAY ({}), RTA_PRIORITY ({})",
364+
route.to.value().Addr().c_str(),
365+
route.via.value().Addr().c_str(),
317366
route.metric);
318367
utils::InitializeAddressAttribute<TAddr>(message.to, route.to.value(), RTA_DST);
319368
utils::InitializeAddressAttribute<TAddr>(message.via, route.via.value(), RTA_GATEWAY);

‎src/linux/netlinkutil/RoutingTable.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,9 @@ class RoutingTable
5151
template <typename TAddr>
5252
void ModifyDefaultRouteImpl(const Route& route, int operation, int flags);
5353

54+
template <typename TAddr>
55+
void ModifyDefaultLinkLocalRouteImpl(const Route& route, int operation, int flags);
56+
5457
template <typename TAddr>
5558
void ModifyLinkLocalRouteImpl(const Route& route, int operation, int flags);
5659

‎src/windows/service/exe/WslMirroredNetworking.cpp‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -347,6 +347,15 @@ void wsl::core::networking::WslMirroredNetworkManager::ProcessRouteChange()
347347
{
348348
endpointRoute.Metric = UINT16_MAX;
349349
}
350+
351+
// Some Windows interfaces (like VPNs) can have metric 0 and routes over that interface with metric also 0, adding
352+
// up to 0. Linux treats metric 0 as unspecified and will default to a 1024 metric. The highest priority metric in
353+
// Linux is 1 instead so we need to switch the metric from 0 to 1.
354+
if (endpointRoute.Metric == 0)
355+
{
356+
endpointRoute.Metric = 1;
357+
}
358+
350359
endpoint.Network->Routes.insert(endpointRoute);
351360
}
352361
}

‎test/windows/NetworkTests.cpp‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -345,6 +345,41 @@ class NetworkTests
345345
VERIFY_ARE_EQUAL(v6State.DefaultRoute->Device, L"eth0");
346346
}
347347

348+
WSL2_TEST_METHOD(AddRemoveDefaultOnlinkRoutes)
349+
{
350+
wsl::shared::hns::Route defaultRouteV4;
351+
defaultRouteV4.NextHop = L"0.0.0.0";
352+
defaultRouteV4.DestinationPrefix = LX_INIT_DEFAULT_ROUTE_PREFIX;
353+
defaultRouteV4.Family = AF_INET;
354+
defaultRouteV4.Metric = 1;
355+
SendDeviceSettingsRequest(L"eth0", defaultRouteV4, ModifyRequestType::Add, GuestEndpointResourceType::Route);
356+
357+
wsl::shared::hns::Route defaultRouteV6;
358+
defaultRouteV6.NextHop = L"::";
359+
defaultRouteV6.DestinationPrefix = LX_INIT_DEFAULT_ROUTE_V6_PREFIX;
360+
defaultRouteV6.Family = AF_INET6;
361+
defaultRouteV6.Metric = 1;
362+
SendDeviceSettingsRequest(L"eth0", defaultRouteV6, ModifyRequestType::Add, GuestEndpointResourceType::Route);
363+
364+
const bool defaultV4RouteExists =
365+
LxsstuLaunchWsl(L"ip -4 route show | grep \"default dev eth0\" | grep -w \"metric 1\"") == (DWORD)0;
366+
const bool defaultV6RouteExists =
367+
LxsstuLaunchWsl(L"ip -6 route show | grep \"default dev eth0\" | grep -w \"metric 1\"") == (DWORD)0;
368+
369+
SendDeviceSettingsRequest(L"eth0", defaultRouteV4, ModifyRequestType::Remove, GuestEndpointResourceType::Route);
370+
SendDeviceSettingsRequest(L"eth0", defaultRouteV6, ModifyRequestType::Remove, GuestEndpointResourceType::Route);
371+
372+
const bool defaultV4RouteRemoved =
373+
LxsstuLaunchWsl(L"ip -4 route show | grep \"default dev eth0\" | grep -w \"metric 1\"") != (DWORD)0;
374+
const bool defaultV6RouteRemoved =
375+
LxsstuLaunchWsl(L"ip -6 route show | grep \"default dev eth0\" | grep -w \"metric 1\"") != (DWORD)0;
376+
377+
VERIFY_IS_TRUE(defaultV4RouteExists);
378+
VERIFY_IS_TRUE(defaultV6RouteExists);
379+
VERIFY_IS_TRUE(defaultV4RouteRemoved);
380+
VERIFY_IS_TRUE(defaultV6RouteRemoved);
381+
}
382+
348383
WSL2_TEST_METHOD(SetInterfaceDownAndUp)
349384
{
350385
// Disconnect interface

0 commit comments

Comments
 (0)