From 7f937ecece6b75bd39199f2d572e4de325d02297 Mon Sep 17 00:00:00 2001 From: Beniamino Galvani Date: Mon, 17 Jun 2019 15:31:59 +0200 Subject: [PATCH 1/5] 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) --- 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 3f5c15a5a3..cd6d5bc181 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -2802,6 +2802,7 @@ concheck_update_state (NMDevice *self, NM_CONNECTIVITY_PORTAL, NM_CONNECTIVITY_FULL, NM_CONNECTIVITY_FAKE, + NM_CONNECTIVITY_NONE, NM_CONNECTIVITY_ERROR)); if (state == NM_CONNECTIVITY_ERROR) { @@ -3128,6 +3129,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 2816e76afc..8305b00923 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 28540a8eb8c952dc35d9894486a79a4116fdaf82 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 18 Jun 2019 09:25:32 +0200 Subject: [PATCH 2/5] connectivity: remove unused error varialbe in _idle_cb() (cherry picked from commit 4001aee3703ddc72e5f358c74478396603dfac0e) --- 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 8305b00923..5165c39413 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 3c2886f447efdcb22d1bf638373441eb36aee2b2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 18 Jun 2019 09:30:35 +0200 Subject: [PATCH 3/5] connectivity: simplify passing result to idle handler (cherry picked from commit 19c957f091334dd12c5b99800c2d150a42586600) --- 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 5165c39413..07e70fc433 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; } @@ -950,7 +941,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; } @@ -986,7 +978,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 a842280dbe43a3987f955ce4308d668069dd300b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 18 Jun 2019 09:36:41 +0200 Subject: [PATCH 4/5] 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) --- 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 07e70fc433..aee6c577bd 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. From aa055239a226252b6a74028b29fbc142220daffa Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 18 Jun 2019 09:38:38 +0200 Subject: [PATCH 5/5] Revert "Coerce connectivity "LIMITED" to "NONE" when device is disconnected" NMConnectivity can now distinguish between LIMITED and NONE connectivity and it does so based on whether IP addresses and routes are configured. Previously, NMConnectivity would not differenciate between limited and no connectivity, which is why NMDevice added some additional logic on top to coerce LIMITED to NONE (if the device is not logically connected). But note that the connectivity state (whether a network is reachable on an interface) depends on what is configured in kernel and whether the internet is reachable on that interface. It does not depend on the logical device state. On the other hand, whether the device is configured in a manner to have connectivity depends on the logical state of the device (as NetworkManager is configuring the device). So, in many cases, the logical state and the connectivity state agree now, but for the right reasons. This reverts commit 4c4dbcb78d5b9427434c46c6952d6518b2e66562. (cherry picked from commit 5a416a9da1ac72241933fb52e2a361a87a6cfe66) --- src/devices/nm-device.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index cd6d5bc181..e9128a25cf 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -2825,14 +2825,6 @@ concheck_update_state (NMDevice *self, state = NM_CONNECTIVITY_LIMITED; } else state = NM_CONNECTIVITY_NONE; - } else if (state == NM_CONNECTIVITY_LIMITED) { - /* NMConnectivity cannot distinguish between NONE and LIMITED connectivity. In both - * cases, it just failed to fetch the URL. - * - * NMDevice coerces a LIMITED state to NONE here, if the logical state of the device - * is disconnected. */ - if (priv->state <= NM_DEVICE_STATE_DISCONNECTED) - state = NM_CONNECTIVITY_NONE; } if (priv->concheck_x[IS_IPv4].state == state) {