From 97d3d2759129e79c8afa09f44a78a7b8c83ad158 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Mon, 17 Jun 2019 15:31:59 +0200 Subject: [PATCH 1/4] device: don't start connectivity check on unconfigured devices If the interface has no carrier, no addresses or no routes there is no point in starting a connectivity check on it because it will fail. Moreover, doing the check on a device without routes causes the addition of a negative entry in the ARP table for each of the addresses associated with the connectivity check host; this can lead to poor network performances. https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/181 (cherry picked from commit 91d447df19a7f72b0ebb6c7cc05680c5f856f63b) (cherry picked from commit 7f937ecece6b75bd39199f2d572e4de325d02297) --- src/devices/nm-device.c | 2 + src/nm-connectivity.c | 91 ++++++++++++++++++++++++++++++++++++++++- src/nm-connectivity.h | 1 + 3 files changed, 93 insertions(+), 1 deletion(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index 268b679d63..4658752277 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -2799,6 +2799,7 @@ concheck_update_state (NMDevice *self, int addr_family, NM_CONNECTIVITY_PORTAL, NM_CONNECTIVITY_FULL, NM_CONNECTIVITY_FAKE, + NM_CONNECTIVITY_NONE, NM_CONNECTIVITY_ERROR)); if (state == NM_CONNECTIVITY_ERROR) { @@ -3117,6 +3118,7 @@ concheck_start (NMDevice *self, handle->c_handle = nm_connectivity_check_start (concheck_get_mgr (self), handle->addr_family, + nm_device_get_platform (self), nm_device_get_ip_ifindex (self), nm_device_get_ip_iface (self), concheck_cb, diff --git a/src/nm-connectivity.c b/src/nm-connectivity.c index fde2379277..5c88da5759 100644 --- a/src/nm-connectivity.c +++ b/src/nm-connectivity.c @@ -27,6 +27,7 @@ #if WITH_CONCHECK #include #endif +#include #include "c-list/src/c-list.h" #include "nm-core-internal.h" @@ -104,6 +105,7 @@ struct _NMConnectivityCheckHandle { guint timeout_id; NMConnectivityState completed_state; + const char *completed_reason; bool fail_reason_no_dbus_connection:1; }; @@ -672,7 +674,9 @@ _idle_cb (gpointer user_data) g_set_error (&error, NM_UTILS_ERROR, NM_UTILS_ERROR_INVALID_ARGUMENT, "no D-Bus connection"); cb_data_complete (cb_data, NM_CONNECTIVITY_ERROR, "no D-Bus connection"); - } else + } else if (cb_data->completed_reason) + cb_data_complete (cb_data, cb_data->completed_state, cb_data->completed_reason); + else cb_data_complete (cb_data, NM_CONNECTIVITY_FAKE, "fake result"); return G_SOURCE_REMOVE; } @@ -800,9 +804,78 @@ resolve_cb (GObject *object, GAsyncResult *res, gpointer user_data) #define SD_RESOLVED_DNS ((guint64) (1LL << 0)) +static NMConnectivityState +check_platform_config (NMConnectivity *self, + NMPlatform *platform, + int ifindex, + int addr_family, + const char **reason) +{ + const NMDedupMultiHeadEntry *addresses; + const NMDedupMultiHeadEntry *routes; + + if (!nm_platform_link_is_connected (platform, ifindex)) { + NM_SET_OUT (reason, "no carrier"); + return NM_CONNECTIVITY_NONE; + } + + addresses = nm_platform_lookup_object (platform, + addr_family == AF_INET + ? NMP_OBJECT_TYPE_IP4_ADDRESS + : NMP_OBJECT_TYPE_IP6_ADDRESS, + ifindex); + if (!addresses || addresses->len == 0) { + NM_SET_OUT (reason, "no IP address configured"); + return NM_CONNECTIVITY_NONE; + } + + routes = nm_platform_lookup_object (platform, + addr_family == AF_INET + ? NMP_OBJECT_TYPE_IP4_ROUTE + : NMP_OBJECT_TYPE_IP6_ROUTE, + ifindex); + if (!routes || routes->len == 0) { + NM_SET_OUT (reason, "no IP route configured"); + return NM_CONNECTIVITY_NONE; + } + + switch (addr_family) { + case AF_INET: { + const NMPlatformIP4Route *route; + gboolean found_global = FALSE; + NMDedupMultiIter iter; + const NMPObject *plobj; + + /* For IPv4 also require a route with global scope. */ + nmp_cache_iter_for_each (&iter, routes, &plobj) { + route = NMP_OBJECT_CAST_IP4_ROUTE (plobj); + if (nm_platform_route_scope_inv (route->scope_inv) == RT_SCOPE_UNIVERSE) { + found_global = TRUE; + break; + } + } + + if (!found_global) { + NM_SET_OUT (reason, "no global route configured"); + return NM_CONNECTIVITY_LIMITED; + } + break; + } + case AF_INET6: + /* Route scopes aren't meaningful for IPv6 so any route is fine. */ + break; + default: + g_return_val_if_reached (FALSE); + } + + NM_SET_OUT (reason, NULL); + return NM_CONNECTIVITY_UNKNOWN; +} + NMConnectivityCheckHandle * nm_connectivity_check_start (NMConnectivity *self, int addr_family, + NMPlatform *platform, int ifindex, const char *iface, NMConnectivityCheckCallback callback, @@ -837,9 +910,25 @@ nm_connectivity_check_start (NMConnectivity *self, && priv->enabled && priv->uri_valid) { gboolean has_systemd_resolved; + NMConnectivityState state; + const char *reason; cb_data->concheck.ch_ifindex = ifindex; + state = check_platform_config (self, + platform, + ifindex, + addr_family, + &reason); + nm_assert ((state == NM_CONNECTIVITY_UNKNOWN) == !reason); + if (state != NM_CONNECTIVITY_UNKNOWN) { + _LOG2D ("skip connectivity check due to %s", reason); + cb_data->completed_state = state; + cb_data->completed_reason = reason; + cb_data->timeout_id = g_idle_add (_idle_cb, cb_data); + return cb_data; + } + /* note that we pick up support for systemd-resolved right away when we need it. * We don't need to remember the setting, because we can (cheaply) check anew * on each request. diff --git a/src/nm-connectivity.h b/src/nm-connectivity.h index f262298a82..f7ef0109ec 100644 --- a/src/nm-connectivity.h +++ b/src/nm-connectivity.h @@ -74,6 +74,7 @@ typedef void (*NMConnectivityCheckCallback) (NMConnectivity *self, NMConnectivityCheckHandle *nm_connectivity_check_start (NMConnectivity *self, int family, + NMPlatform *platform, int ifindex, const char *iface, NMConnectivityCheckCallback callback, From 597b8da69793e1482778b8bf6e569155038a50c8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 18 Jun 2019 09:25:32 +0200 Subject: [PATCH 2/4] connectivity: remove unused error varialbe in _idle_cb() (cherry picked from commit 4001aee3703ddc72e5f358c74478396603dfac0e) (cherry picked from commit 28540a8eb8c952dc35d9894486a79a4116fdaf82) --- src/nm-connectivity.c | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/src/nm-connectivity.c b/src/nm-connectivity.c index 5c88da5759..078f5854c0 100644 --- a/src/nm-connectivity.c +++ b/src/nm-connectivity.c @@ -662,19 +662,11 @@ _idle_cb (gpointer user_data) cb_data->timeout_id = 0; if (!cb_data->ifspec) { - gs_free_error GError *error = NULL; - /* the invocation was with an invalid ifname. It is a fail. */ - g_set_error (&error, NM_UTILS_ERROR, NM_UTILS_ERROR_INVALID_ARGUMENT, - "no interface specified for connectivity check"); cb_data_complete (cb_data, NM_CONNECTIVITY_ERROR, "missing interface"); - } else if (cb_data->fail_reason_no_dbus_connection) { - gs_free_error GError *error = NULL; - - g_set_error (&error, NM_UTILS_ERROR, NM_UTILS_ERROR_INVALID_ARGUMENT, - "no D-Bus connection"); + } else if (cb_data->fail_reason_no_dbus_connection) cb_data_complete (cb_data, NM_CONNECTIVITY_ERROR, "no D-Bus connection"); - } else if (cb_data->completed_reason) + else if (cb_data->completed_reason) cb_data_complete (cb_data, cb_data->completed_state, cb_data->completed_reason); else cb_data_complete (cb_data, NM_CONNECTIVITY_FAKE, "fake result"); From 4f489ac4504db3cff5d2dee2b2dfff837e213798 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 18 Jun 2019 09:30:35 +0200 Subject: [PATCH 3/4] connectivity: simplify passing result to idle handler (cherry picked from commit 19c957f091334dd12c5b99800c2d150a42586600) (cherry picked from commit 3c2886f447efdcb22d1bf638373441eb36aee2b2) --- src/nm-connectivity.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/src/nm-connectivity.c b/src/nm-connectivity.c index 078f5854c0..1424c906aa 100644 --- a/src/nm-connectivity.c +++ b/src/nm-connectivity.c @@ -106,8 +106,6 @@ struct _NMConnectivityCheckHandle { NMConnectivityState completed_state; const char *completed_reason; - - bool fail_reason_no_dbus_connection:1; }; enum { @@ -659,17 +657,10 @@ _idle_cb (gpointer user_data) nm_assert (NM_IS_CONNECTIVITY (cb_data->self)); nm_assert (c_list_contains (&NM_CONNECTIVITY_GET_PRIVATE (cb_data->self)->handles_lst_head, &cb_data->handles_lst)); + nm_assert (cb_data->completed_reason); cb_data->timeout_id = 0; - if (!cb_data->ifspec) { - /* the invocation was with an invalid ifname. It is a fail. */ - cb_data_complete (cb_data, NM_CONNECTIVITY_ERROR, "missing interface"); - } else if (cb_data->fail_reason_no_dbus_connection) - cb_data_complete (cb_data, NM_CONNECTIVITY_ERROR, "no D-Bus connection"); - else if (cb_data->completed_reason) - cb_data_complete (cb_data, cb_data->completed_state, cb_data->completed_reason); - else - cb_data_complete (cb_data, NM_CONNECTIVITY_FAKE, "fake result"); + cb_data_complete (cb_data, cb_data->completed_state, cb_data->completed_reason); return G_SOURCE_REMOVE; } @@ -939,7 +930,8 @@ nm_connectivity_check_start (NMConnectivity *self, * * Anyway, something is very odd, just fail connectivity check. */ _LOG2D ("start fake request (fail due to no D-Bus connection)"); - cb_data->fail_reason_no_dbus_connection = TRUE; + cb_data->completed_state = NM_CONNECTIVITY_ERROR; + cb_data->completed_reason = "no D-Bus connection"; cb_data->timeout_id = g_idle_add (_idle_cb, cb_data); return cb_data; } @@ -975,7 +967,14 @@ nm_connectivity_check_start (NMConnectivity *self, } #endif - _LOG2D ("start fake request"); + if (!cb_data->ifspec) { + cb_data->completed_state = NM_CONNECTIVITY_ERROR; + cb_data->completed_reason = "missing interface"; + } else { + cb_data->completed_state = NM_CONNECTIVITY_FAKE; + cb_data->completed_reason = "fake result"; + } + _LOG2D ("start fake request (%s)", cb_data->completed_reason); cb_data->timeout_id = g_idle_add (_idle_cb, cb_data); return cb_data; From 36cdfe703fcbe28330c19eab3365d72a59a8c660 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 18 Jun 2019 09:36:41 +0200 Subject: [PATCH 4/4] connectivity: make platform argument to nm_connectivity_check_start() optional The platform is used to detect whether to skip the connectivity check right away. It should be an optional argument, so one could avoid this pre-check. (cherry picked from commit b626baa3136b9309b0dd1d1fad48677413103269) (cherry picked from commit a842280dbe43a3987f955ce4308d668069dd300b) --- src/nm-connectivity.c | 27 +++++++++++++++------------ 1 file changed, 15 insertions(+), 12 deletions(-) diff --git a/src/nm-connectivity.c b/src/nm-connectivity.c index 1424c906aa..dd73d0692a 100644 --- a/src/nm-connectivity.c +++ b/src/nm-connectivity.c @@ -870,6 +870,7 @@ nm_connectivity_check_start (NMConnectivity *self, g_return_val_if_fail (NM_IS_CONNECTIVITY (self), NULL); g_return_val_if_fail (callback, NULL); + nm_assert (!platform || NM_IS_PLATFORM (platform)); priv = NM_CONNECTIVITY_GET_PRIVATE (self); @@ -898,18 +899,20 @@ nm_connectivity_check_start (NMConnectivity *self, cb_data->concheck.ch_ifindex = ifindex; - state = check_platform_config (self, - platform, - ifindex, - addr_family, - &reason); - nm_assert ((state == NM_CONNECTIVITY_UNKNOWN) == !reason); - if (state != NM_CONNECTIVITY_UNKNOWN) { - _LOG2D ("skip connectivity check due to %s", reason); - cb_data->completed_state = state; - cb_data->completed_reason = reason; - cb_data->timeout_id = g_idle_add (_idle_cb, cb_data); - return cb_data; + if (platform) { + state = check_platform_config (self, + platform, + ifindex, + addr_family, + &reason); + nm_assert ((state == NM_CONNECTIVITY_UNKNOWN) == !reason); + if (state != NM_CONNECTIVITY_UNKNOWN) { + _LOG2D ("skip connectivity check due to %s", reason); + cb_data->completed_state = state; + cb_data->completed_reason = reason; + cb_data->timeout_id = g_idle_add (_idle_cb, cb_data); + return cb_data; + } } /* note that we pick up support for systemd-resolved right away when we need it.