On Tue, 2015-05-05 at 23:53 -0400, David Ward wrote:
After DHCP RENEW/REBIND events occur (in particular), do not update
the installed routes if there were no relevant IP config changes.
Aside from being an optimization, this behavior is needed to avoid
unnecessarily interfering with standalone, commerical VPN clients
which follow remote security policy settings. These VPN clients may
force all traffic to go through the tunnel by installing additional
routes in the kernel for each interface, and they may monitor the
routing table and terminate the VPN session if any changes occur.
NetworkManager can coexist with this type of VPN client, as long as
no IP config changes happen on any devices during the VPN session.
Signed-off-by: David Ward <david ward ll mit edu>
---
src/devices/nm-device.c | 11 ++++++-----
src/nm-iface-helper.c | 4 ++--
src/nm-ip4-config.c | 4 ++--
src/nm-ip4-config.h | 2 +-
src/nm-ip6-config.c | 4 ++--
src/nm-ip6-config.h | 2 +-
src/vpn-manager/nm-vpn-connection.c | 4 ++--
7 files changed, 16 insertions(+), 15 deletions(-)
diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index 24ad35f..0387fd1 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -6135,10 +6135,11 @@ nm_device_set_ip4_config (NMDevice *self,
nm_device_set_mtu (self, nm_ip4_config_get_mtu (new_config));
- /* for assumed devices we set the device_route_metric to the default which will
- * stop nm_platform_ip4_address_sync() to replace the device routes. */
- success = nm_ip4_config_commit (new_config, ip_ifindex,
- assumed ? NM_PLATFORM_ROUTE_METRIC_IP4_DEVICE_ROUTE :
default_route_metric);
+ /* If there are no relevant changes, or if the connection is assumed,
+ * pass NM_PLATFORM_ROUTE_METRIC_IP4_DEVICE_ROUTE in order to prevent
+ * nm_platform_ip4_address_sync() from replacing the device routes. */
+ success = nm_ip4_config_commit (new_config, ip_ifindex, has_changes,
+ !has_changes || assumed ?
NM_PLATFORM_ROUTE_METRIC_IP4_DEVICE_ROUTE : default_route_metric);
if (!success)
reason_local = NM_DEVICE_STATE_REASON_CONFIG_FAILED;
}
@@ -6265,7 +6266,7 @@ nm_device_set_ip6_config (NMDevice *self,
/* Always commit to nm-platform to update lifetimes */
if (commit && new_config) {
nm_device_ipv6_set_mtu (self, priv->ip6_mtu);
- success = nm_ip6_config_commit (new_config, ip_ifindex);
+ success = nm_ip6_config_commit (new_config, ip_ifindex, has_changes);
if (!success)
reason_local = NM_DEVICE_STATE_REASON_CONFIG_FAILED;
}
diff --git a/src/nm-iface-helper.c b/src/nm-iface-helper.c
index 09781a8..6d32a86 100644
--- a/src/nm-iface-helper.c
+++ b/src/nm-iface-helper.c
@@ -104,7 +104,7 @@ dhcp4_state_changed (NMDhcpClient *client,
nm_ip4_config_subtract (existing, last_config);
nm_ip4_config_merge (existing, ip4_config);
- if (!nm_ip4_config_commit (existing, ifindex, global_opt.priority_v4))
+ if (!nm_ip4_config_commit (existing, ifindex, TRUE, global_opt.priority_v4))
nm_log_warn (LOGD_DHCP4, "(%s): failed to apply DHCPv4 config", global_opt.ifname);
if (last_config)
@@ -241,7 +241,7 @@ rdisc_config_changed (NMRDisc *rdisc, NMRDiscConfigMap changed, gpointer user_da
nm_ip6_config_subtract (existing, last_config);
nm_ip6_config_merge (existing, ip6_config);
- if (!nm_ip6_config_commit (existing, ifindex))
+ if (!nm_ip6_config_commit (existing, ifindex, TRUE))
nm_log_warn (LOGD_IP6, "(%s): failed to apply IPv6 config", global_opt.ifname);
if (last_config)
diff --git a/src/nm-ip4-config.c b/src/nm-ip4-config.c
index bd99961..80dbbac 100644
--- a/src/nm-ip4-config.c
+++ b/src/nm-ip4-config.c
@@ -261,7 +261,7 @@ nm_ip4_config_capture (int ifindex, gboolean capture_resolv_conf)
}
gboolean
-nm_ip4_config_commit (const NMIP4Config *config, int ifindex, guint32 default_route_metric)
+nm_ip4_config_commit (const NMIP4Config *config, int ifindex, gboolean has_relevant_changes, guint32
default_route_metric)
minor nitpick: a better name at this point would be "@sync_routes", because it should tell nm_ip4_config_commit() how to behave -- and not tell nm_ip4_config_commit() about the current situation, which then causes some internal behavior.
{
NMIP4ConfigPrivate *priv = NM_IP4_CONFIG_GET_PRIVATE (config);
int i;
@@ -276,7 +276,7 @@ nm_ip4_config_commit (const NMIP4Config *config, int ifindex, guint32 default_ro
return FALSE;
/* Routes */
- {
+ if (has_relevant_changes) {
int count = nm_ip4_config_get_num_routes (config);
GArray *routes = g_array_sized_new (FALSE, FALSE, sizeof (NMPlatformIP4Route), count);
const NMPlatformIP4Route *route;
diff --git a/src/nm-ip4-config.h b/src/nm-ip4-config.h
index 3e55195..a6b2d55 100644
--- a/src/nm-ip4-config.h
+++ b/src/nm-ip4-config.h
@@ -67,7 +67,7 @@ const char * nm_ip4_config_get_dbus_path (const NMIP4Config *config);
/* Integration with nm-platform and nm-setting */
NMIP4Config *nm_ip4_config_capture (int ifindex, gboolean capture_resolv_conf);
-gboolean nm_ip4_config_commit (const NMIP4Config *config, int ifindex, guint32 default_route_metric);
+gboolean nm_ip4_config_commit (const NMIP4Config *config, int ifindex, gboolean has_relevant_changes,
guint32 default_route_metric);
void nm_ip4_config_merge_setting (NMIP4Config *config, NMSettingIPConfig *setting, guint32
default_route_metric);
NMSetting *nm_ip4_config_create_setting (const NMIP4Config *config);
diff --git a/src/nm-ip6-config.c b/src/nm-ip6-config.c
index 56ce981..183794e 100644
--- a/src/nm-ip6-config.c
+++ b/src/nm-ip6-config.c
@@ -377,7 +377,7 @@ nm_ip6_config_capture (int ifindex, gboolean capture_resolv_conf, NMSettingIP6Co
}
gboolean
-nm_ip6_config_commit (const NMIP6Config *config, int ifindex)
+nm_ip6_config_commit (const NMIP6Config *config, int ifindex, gboolean has_relevant_changes)
{
NMIP6ConfigPrivate *priv = NM_IP6_CONFIG_GET_PRIVATE (config);
int i;
@@ -392,7 +392,7 @@ nm_ip6_config_commit (const NMIP6Config *config, int ifindex)
return FALSE;
/* Routes */
- {
+ if (has_relevant_changes) {
int count = nm_ip6_config_get_num_routes (config);
GArray *routes = g_array_sized_new (FALSE, FALSE, sizeof (NMPlatformIP6Route), count);
const NMPlatformIP6Route *route;
diff --git a/src/nm-ip6-config.h b/src/nm-ip6-config.h
index e0527e5..f6d4156 100644
--- a/src/nm-ip6-config.h
+++ b/src/nm-ip6-config.h
@@ -67,7 +67,7 @@ const char * nm_ip6_config_get_dbus_path (const NMIP6Config *config);
/* Integration with nm-platform and nm-setting */
NMIP6Config *nm_ip6_config_capture (int ifindex, gboolean capture_resolv_conf, NMSettingIP6ConfigPrivacy
use_temporary);
-gboolean nm_ip6_config_commit (const NMIP6Config *config, int ifindex);
+gboolean nm_ip6_config_commit (const NMIP6Config *config, int ifindex, gboolean has_relevant_changes);
void nm_ip6_config_merge_setting (NMIP6Config *config, NMSettingIPConfig *setting, guint32
default_route_metric);
NMSetting *nm_ip6_config_create_setting (const NMIP6Config *config);
diff --git a/src/vpn-manager/nm-vpn-connection.c b/src/vpn-manager/nm-vpn-connection.c
index 4329cb2..959499e 100644
--- a/src/vpn-manager/nm-vpn-connection.c
+++ b/src/vpn-manager/nm-vpn-connection.c
@@ -924,13 +924,13 @@ nm_vpn_connection_apply_config (NMVpnConnection *connection)
nm_platform_link_set_up (NM_PLATFORM_GET, priv->ip_ifindex);
if (priv->ip4_config) {
- if (!nm_ip4_config_commit (priv->ip4_config, priv->ip_ifindex,
+ if (!nm_ip4_config_commit (priv->ip4_config, priv->ip_ifindex, TRUE,
nm_vpn_connection_get_ip4_route_metric (connection)))
return FALSE;
}
if (priv->ip6_config) {
- if (!nm_ip6_config_commit (priv->ip6_config, priv->ip_ifindex))
+ if (!nm_ip6_config_commit (priv->ip6_config, priv->ip_ifindex, TRUE))
return FALSE;
}
}
In nm_device_set_ip4_config(), @old_config and @new_config are both the representation of "what NM thinks should be configured". @has_changes says, that between before and now, whether "what should be" changed. This allows to safe some internal actions, such as sending D-Bus notifications about the configuration changes. That is distinct from what nm_ip4_config_commit() / nmnm_route_manager_ip6_route_sync() does: That one takes "what NM thinks should be", and applies it to "what actually is". The conclusion from @has_changes -> @do_sync_routes is not necessarily correct... Maybe it is, not sure here... Could you elaborate a bit more on the actual problem you have? It's about these VPN clients, right? thank you!! Thomas
Attachment:
signature.asc
Description: This is a digitally signed message part