dns: change behavior for "rc-manager=symlink" to preserve "/etc/resolv.conf" as fileth/dns-unsymlink-resolv-conf-rh1367551-v2

The purpose of "rc-manager=symlink" is so that the administrator can point
the "/etc/resolv.conf" as a symlink to a certain file, and thus indicating
that a certain component is responsible to manage resolv.conf, while others
should stay away from it.
For example, systemd-resolved never touches "/etc/resolv.conf", but
expects the admin to setup the symlink appropriately. It also recognizes
whether the symlink points to it's own resolv.conf in /run or to another
component.

Previously, "rc-manager=symlink" would always replace a regular file
with a symlink to "/var/run/NetworkManager/resolv.conf". Only if
"/etc/resolv.conf" is already a symlink somewhere else, NM would not
touch it. This with the exception that if "/etc/resolv.conf" points to
"/var/run/NetworkManager/resolv.conf", it would replace the symlink
with the same link to raise inotify events.

Change behavior so if "/etc/resolv.conf" is already a regular file, keep
it as file.
This means, if you have multiple components that don't care, everybody
can write the "/etc/resolv.conf" (as file) and there is no clear
expressed responsibility.
It was wrong that NetworkManager would convert the file to a symlink,
this should be reserved to the admin. Instead, NetworkManager should
accept that the intent is unspecified and preserve the regular file.
It's up to the admin to replace the symlink to somewhere else (to keep
NM off), or to point it to "/var/run/NetworkManager/resolv.conf", to show
the explicit intent.

The wrong behavior causes dangling symlinks when somebody disables
NetworkManager for good.

https://bugzilla.redhat.com/show_bug.cgi?id=1367551

4 files changed, 50 insertions, 65 deletions

diff --git a/configure.ac b/configure.ac
index b5ca3b4162..611ef582ed 100644
--- a/configure.ac
+++ b/configure.ac
@@ -89,6 +89,7 @@ AC_SUBST(nmlibdir, '${prefix}'/lib/$PACKAGE, [NetworkManager library directory])
 AC_SUBST(nmdatadir, '${datadir}'/$PACKAGE, [NetworkManager shared data directory])
 AC_SUBST(nmstatedir, '${localstatedir}'/lib/$PACKAGE, [NetworkManager persistent state directory])
 AC_SUBST(nmrundir, '${runstatedir}'/$PACKAGE, [NetworkManager runtime state directory])
+AC_SUBST(nmrundir_, "${runstatedir}/$PACKAGE", [NetworkManager runtime state directory (expanded)])
 
 AC_GNU_SOURCE
 AC_CHECK_FUNCS([__secure_getenv secure_getenv])
diff --git a/man/NetworkManager.conf.xml b/man/NetworkManager.conf.xml
index 8c93ac4c06..7ca496bfb0 100644
--- a/man/NetworkManager.conf.xml
+++ b/man/NetworkManager.conf.xml
@@ -321,15 +321,17 @@ no-auto-default=*
         options, and this version of NetworkManager was build with a default of
         "<literal>&NM_CONFIG_DEFAULT_MAIN_RC_MANAGER;</literal>".
         Regardless of this setting, NetworkManager will
-        always write resolv.conf to its runtime state directory.</para>
-        <para><literal>symlink</literal>: NetworkManager will symlink
-        <filename>/etc/resolv.conf</filename> to its private
-        resolv.conf file in the runtime state directory. If
-        <filename>/etc/resolv.conf</filename>
-        already is a symlink pointing to a different location, the file
-        will not be modified. This allows the user to disable managing
-        by pointing the link <filename>/etc/resolv.conf</filename> to
-        somewhere else.</para>
+        always write resolv.conf to its runtime state directory
+        <filename>&nmrundir;/resolv.conf</filename>.</para>
+        <para><literal>symlink</literal>: If <filename>/etc/resolv.conf</filename> is
+        a regular file, NetworkManager will replace the file on update. If
+        <filename>/etc/resolv.conf</filename> is instead a symlink, NetworkManager
+        will leave it alone. Unless the symlink points to the internal file
+        <filename>&nmrundir;/resolv.conf</filename>,
+        in which case the symlink will be updated to emit an inotify notification.
+        This allows the user to conveniently instruct NetworkManager not
+        to manage <filename>/etc/resolv.conf</filename> by replacing it with
+        a symlink.</para>
         <para><literal>file</literal>: NetworkManager will write
         <filename>/etc/resolv.conf</filename> as file. If it finds
         a symlink, it will follow the symlink and update the target
diff --git a/man/common.ent.in b/man/common.ent.in
index 4c3310c44b..b3a4bca8fe 100644
--- a/man/common.ent.in
+++ b/man/common.ent.in
@@ -1,6 +1,7 @@
 <?xml version="1.0" encoding="utf-8" ?>
 <!ENTITY NM_VERSION "@NM_VERSION@">
 <!ENTITY sysconfdir "@sysconfdir@">
+<!ENTITY nmrundir "@nmrundir_@">
 <!ENTITY NM_CONFIG_DEFAULT_MAIN_AUTH_POLKIT_TEXT "@NM_CONFIG_DEFAULT_MAIN_AUTH_POLKIT_TEXT@">
 <!ENTITY NM_CONFIG_DEFAULT_LOGGING_BACKEND_TEXT "@NM_CONFIG_DEFAULT_LOGGING_BACKEND_TEXT@">
 <!ENTITY NM_CONFIG_DEFAULT_LOGGING_AUDIT_TEXT "@NM_CONFIG_DEFAULT_LOGGING_AUDIT_TEXT@">
diff --git a/src/dns/nm-dns-manager.c b/src/dns/nm-dns-manager.c
index c990243ec7..ab41af9496 100644
--- a/src/dns/nm-dns-manager.c
+++ b/src/dns/nm-dns-manager.c
@@ -668,6 +668,20 @@ dispatch_resolvconf (NMDnsManager *self,
 	return success ? SR_SUCCESS : SR_ERROR;
 }
 
+static const char *
+_read_link_cached (const char *path, gboolean *is_cached, char **cached)
+{
+	nm_assert (is_cached);
+	nm_assert (cached);
+
+	if (*is_cached)
+		return *cached;
+
+	nm_assert (!*cached);
+	*is_cached = TRUE;
+	return (*cached = g_file_read_link (path, NULL));
+}
+
 #define MY_RESOLV_CONF NMRUNDIR "/resolv.conf"
 #define MY_RESOLV_CONF_TMP MY_RESOLV_CONF ".tmp"
 #define RESOLV_CONF_TMP "/etc/.resolv.conf.NetworkManager"
@@ -681,13 +695,14 @@ update_resolv_conf (NMDnsManager *self,
                     NMDnsManagerResolvConfManager rc_manager)
 {
 	FILE *f;
-	struct stat st;
 	gboolean success;
 	gs_free char *content = NULL;
 	SpawnResult write_file_result = SR_SUCCESS;
 	int errsv;
 	const char *rc_path = _PATH_RESCONF;
 	nm_auto_free char *rc_path_real = NULL;
+	gboolean resconf_link_cached = FALSE;
+	gs_free char *resconf_link = NULL;
 
 	/* If we are not managing /etc/resolv.conf and it points to
 	 * MY_RESOLV_CONF, don't write the private DNS configuration to
@@ -697,9 +712,8 @@ update_resolv_conf (NMDnsManager *self,
 	 * This is the only situation, where we don't try to update our
 	 * internal resolv.conf file. */
 	if (rc_manager == NM_DNS_MANAGER_RESOLV_CONF_MAN_UNMANAGED) {
-		gs_free char *path = g_file_read_link (_PATH_RESCONF, NULL);
-
-		if (g_strcmp0 (path, MY_RESOLV_CONF) == 0) {
+		if (nm_streq0 (_read_link_cached (_PATH_RESCONF, &resconf_link_cached, &resconf_link),
+		               MY_RESOLV_CONF)) {
 			_LOGD ("update-resolv-conf: not updating " _PATH_RESCONF
 			       " since it points to " MY_RESOLV_CONF);
 			return SR_SUCCESS;
@@ -708,12 +722,16 @@ update_resolv_conf (NMDnsManager *self,
 
 	content = create_resolv_conf (searches, nameservers, options);
 
-	if (rc_manager == NM_DNS_MANAGER_RESOLV_CONF_MAN_FILE) {
+	if (   rc_manager == NM_DNS_MANAGER_RESOLV_CONF_MAN_FILE
+	    || (   rc_manager == NM_DNS_MANAGER_RESOLV_CONF_MAN_SYMLINK
+	        && !_read_link_cached (_PATH_RESCONF, &resconf_link_cached, &resconf_link))) {
 		GError *local = NULL;
 
-		rc_path_real = realpath (rc_path, NULL);
-		if (rc_path_real)
-			rc_path = rc_path_real;
+		if (rc_manager == NM_DNS_MANAGER_RESOLV_CONF_MAN_FILE) {
+			rc_path_real = realpath (rc_path, NULL);
+			if (rc_path_real)
+				rc_path = rc_path_real;
+		}
 
 		/* we first write to /etc/resolv.conf directly. If that fails,
 		 * we still continue to write to runstatedir but remember the
@@ -788,60 +806,23 @@ update_resolv_conf (NMDnsManager *self,
 		return write_file_result;
 	}
 
-	if (rc_manager != NM_DNS_MANAGER_RESOLV_CONF_MAN_SYMLINK) {
+	if (   rc_manager != NM_DNS_MANAGER_RESOLV_CONF_MAN_SYMLINK
+	    || !_read_link_cached (_PATH_RESCONF, &resconf_link_cached, &resconf_link)) {
 		_LOGT ("update-resolv-conf: write internal file %s succeeded", MY_RESOLV_CONF);
 		return SR_SUCCESS;
 	}
 
-	/* A symlink pointing to NM's own resolv.conf (MY_RESOLV_CONF) is always
-	 * overwritten to ensure that changes are indicated with inotify.  Symlinks
-	 * pointing to any other file are never overwritten.
-	 */
-	if (lstat (_PATH_RESCONF, &st) != 0) {
-		errsv = errno;
-		if (errsv != ENOENT) {
-			/* NM cannot read /etc/resolv.conf */
-			_LOGT ("update-resolv-conf: write internal file %s succeeded but lstat(%s) failed (%s)",
-			       MY_RESOLV_CONF, _PATH_RESCONF, g_strerror (errsv));
-			g_set_error (error,
-			             NM_MANAGER_ERROR,
-			             NM_MANAGER_ERROR_FAILED,
-			             "Could not lstat %s: %s",
-			             _PATH_RESCONF,
-			             g_strerror (errsv));
-			return SR_ERROR;
-		}
-	} else {
-		if (S_ISLNK (st.st_mode)) {
-			if (stat (_PATH_RESCONF, &st) != -1) {
-				gs_free char *path = g_file_read_link (_PATH_RESCONF, NULL);
-
-				if (!path || !nm_streq (path, MY_RESOLV_CONF)) {
-					/* It's not NM's symlink; do nothing */
-					_LOGT ("update-resolv-conf: write internal file %s succeeded "
-					       "but don't update %s as it points to %s",
-					       MY_RESOLV_CONF, _PATH_RESCONF, path ?: "");
-					return SR_SUCCESS;
-				}
-
-				/* resolv.conf is a symlink owned by NM and the target is accessible
-				 */
-			} else {
-				/* resolv.conf is a symlink but the target is not accessible;
-				 * some other program is probably managing resolv.conf and
-				 * NM should not touch it.
-				 */
-				_LOGT ("update-resolv-conf: write internal file %s succeeded "
-				       "but don't update %s as the symlinks points somewhere else",
-				       MY_RESOLV_CONF, _PATH_RESCONF);
-				return SR_SUCCESS;
-			}
-		}
+	if (!nm_streq0 (_read_link_cached (_PATH_RESCONF, &resconf_link_cached, &resconf_link),
+	                MY_RESOLV_CONF)) {
+		_LOGT ("update-resolv-conf: write internal file %s succeeded (don't touch symlink %s linking to %s)",
+		       MY_RESOLV_CONF, _PATH_RESCONF,
+		       _read_link_cached (_PATH_RESCONF, &resconf_link_cached, &resconf_link));
+		return SR_SUCCESS;
 	}
 
-	/* By this point, either /etc/resolv.conf does not exist, is a regular
-	 * file, or is a symlink already owned by NM.  In all cases /etc/resolv.conf
-	 * is replaced with a symlink pointing to NM's resolv.conf in /var/run/.
+	/* By this point, /etc/resolv.conf exists and is a symlink to our internal
+	 * resolv.conf. We update the symlink so that applications get an inotify
+	 * notification.
 	 */
 	if (   unlink (RESOLV_CONF_TMP) != 0
 	    && ((errsv = errno) != ENOENT)) {