[PATCH] daemon: Use bind mount to overwrite /etc/resolv.conf
Implement bind mount to overwrite /etc/resolv.conf instead of the current method of rename followed by copy. The bind mount method works even if the file is designated as immutable Signed-off-by: Srihari Parimi <sparimi@redhat.com> --- daemon/sh.c | 86 ++++++++++++++++++++++------------------------------- 1 file changed, 35 insertions(+), 51 deletions(-) diff --git a/daemon/sh.c b/daemon/sh.c index 74e003410..aab9a4534 100644 --- a/daemon/sh.c +++ b/daemon/sh.c @@ -150,8 +150,9 @@ free_bind_state (struct bind_state *bs) static int set_up_etc_resolv_conf (struct resolver_state *rs) { + char *src_etc_resolv = "/etc/resolv.conf"; struct stat statbuf; - CLEANUP_FREE char *buf = NULL; + int r, fd = -1; rs->sysroot_etc_resolv_conf_old = NULL; @@ -161,50 +162,43 @@ set_up_etc_resolv_conf (struct resolver_state *rs) reply_with_perror ("malloc"); goto error; } - - /* If /etc/resolv.conf exists, rename it to the backup file. Note - * that on Ubuntu it's a dangling symlink. + /* If /sysroot/etc/resolv.conf does not exist, create one for mount + * bind to succeed. lstat() call handles dnagling links */ - if (lstat (rs->sysroot_etc_resolv_conf, &statbuf) == 0) { - /* Make a random name for the backup file. */ - if (asprintf (&buf, "%s/etc/XXXXXXXX", sysroot) == -1) { - reply_with_perror ("asprintf"); - goto error; - } - if (random_name (buf) == -1) { - reply_with_perror ("random_name"); - goto error; - } - rs->sysroot_etc_resolv_conf_old = strdup (buf); - if (!rs->sysroot_etc_resolv_conf_old) { - reply_with_perror ("strdup"); - goto error; + if (lstat (rs->sysroot_etc_resolv_conf, &statbuf) != 0) { + if (errno != ENOENT) { + reply_with_perror ("lstat: %s", rs->sysroot_etc_resolv_conf); + return -1; } - - if (verbose) - fprintf (stderr, "renaming %s to %s\n", rs->sysroot_etc_resolv_conf, - rs->sysroot_etc_resolv_conf_old); - - if (rename (rs->sysroot_etc_resolv_conf, - rs->sysroot_etc_resolv_conf_old) == -1) { - reply_with_perror ("rename: %s to %s", rs->sysroot_etc_resolv_conf, - rs->sysroot_etc_resolv_conf_old); - goto error; + /* create empty file */ + fd = open (rs->sysroot_etc_resolv_conf, + O_WRONLY|O_CREAT|O_NOCTTY|O_CLOEXEC, 0644); + if (fd == -1) { + reply_with_perror ("open: %s", rs->sysroot_etc_resolv_conf); + return -1; } + close(fd); } - - /* Now that the guest's <sysroot>/etc/resolv.conf is out the way, we - * can create our own copy of the appliance /etc/resolv.conf. - */ - ignore_value (command (NULL, NULL, "cp", "/etc/resolv.conf", - rs->sysroot_etc_resolv_conf, NULL)); - + if (verbose) + fprintf (stderr, "mount %s to %s\n", src_etc_resolv, + rs->sysroot_etc_resolv_conf); + /* mount bind read only */ + r = command (NULL, NULL, "mount", "--bind", "-o ro", src_etc_resolv, + rs->sysroot_etc_resolv_conf, NULL); + if (r == -1) { + reply_with_perror ("mount: %s to %s failed\n", + src_etc_resolv, rs->sysroot_etc_resolv_conf); + goto error; + } + if (verbose) + fprintf (stderr, "mount %s to %s done\n", src_etc_resolv, + rs->sysroot_etc_resolv_conf); rs->mounted = true; + return 0; - error: +error: free (rs->sysroot_etc_resolv_conf); - free (rs->sysroot_etc_resolv_conf_old); return -1; } @@ -212,20 +206,10 @@ static void free_resolver_state (struct resolver_state *rs) { if (rs->mounted) { - unlink (rs->sysroot_etc_resolv_conf); - - if (rs->sysroot_etc_resolv_conf_old) { - if (verbose) - fprintf (stderr, "renaming %s to %s\n", rs->sysroot_etc_resolv_conf_old, - rs->sysroot_etc_resolv_conf); - - if (rename (rs->sysroot_etc_resolv_conf_old, - rs->sysroot_etc_resolv_conf) == -1) - perror ("error: could not restore /etc/resolv.conf"); - - free (rs->sysroot_etc_resolv_conf_old); - } - + /* umount sysroot_etc_resolv_conf */ + if (verbose) + fprintf (stderr, "umount %s\n", rs->sysroot_etc_resolv_conf); + umount_ignore_fail(rs->sysroot_etc_resolv_conf); free (rs->sysroot_etc_resolv_conf); rs->mounted = false; } -- 2.52.0
On Thu, May 28, 2026 at 08:11:52AM -0400, Srihari Parimi wrote:
Implement bind mount to overwrite /etc/resolv.conf instead of the current method of rename followed by copy. The bind mount method works even if the file is designated as immutable
Signed-off-by: Srihari Parimi <sparimi@redhat.com>
This needs a 'Fixes: ...' line.
--- daemon/sh.c | 86 ++++++++++++++++++++++------------------------------- 1 file changed, 35 insertions(+), 51 deletions(-)
diff --git a/daemon/sh.c b/daemon/sh.c index 74e003410..aab9a4534 100644 --- a/daemon/sh.c +++ b/daemon/sh.c @@ -150,8 +150,9 @@ free_bind_state (struct bind_state *bs) static int set_up_etc_resolv_conf (struct resolver_state *rs) { + char *src_etc_resolv = "/etc/resolv.conf";
const? If the compiler didn't warn about this then you're probably not enabling warnings. You should configure with: ./configure --enable-werror There are similar options on all the other projects which you should always use.
struct stat statbuf; - CLEANUP_FREE char *buf = NULL; + int r, fd = -1;
Why is fd initialized here? This stops GCC from detecting variables that are used when unset.
rs->sysroot_etc_resolv_conf_old = NULL;
@@ -161,50 +162,43 @@ set_up_etc_resolv_conf (struct resolver_state *rs) reply_with_perror ("malloc"); goto error; } - - /* If /etc/resolv.conf exists, rename it to the backup file. Note - * that on Ubuntu it's a dangling symlink. + /* If /sysroot/etc/resolv.conf does not exist, create one for mount + * bind to succeed. lstat() call handles dnagling links */ - if (lstat (rs->sysroot_etc_resolv_conf, &statbuf) == 0) { - /* Make a random name for the backup file. */ - if (asprintf (&buf, "%s/etc/XXXXXXXX", sysroot) == -1) { - reply_with_perror ("asprintf"); - goto error; - } - if (random_name (buf) == -1) { - reply_with_perror ("random_name"); - goto error; - } - rs->sysroot_etc_resolv_conf_old = strdup (buf); - if (!rs->sysroot_etc_resolv_conf_old) { - reply_with_perror ("strdup"); - goto error; + if (lstat (rs->sysroot_etc_resolv_conf, &statbuf) != 0) {
Using '!= 0' is almost always wrong for system calls. The man page for lstat documents -1 as the error code, so check only that value. Also what's the justification for lstat instead of stat?
+ if (errno != ENOENT) { + reply_with_perror ("lstat: %s", rs->sysroot_etc_resolv_conf); + return -1; } - - if (verbose) - fprintf (stderr, "renaming %s to %s\n", rs->sysroot_etc_resolv_conf, - rs->sysroot_etc_resolv_conf_old); - - if (rename (rs->sysroot_etc_resolv_conf, - rs->sysroot_etc_resolv_conf_old) == -1) { - reply_with_perror ("rename: %s to %s", rs->sysroot_etc_resolv_conf, - rs->sysroot_etc_resolv_conf_old); - goto error; + /* create empty file */ + fd = open (rs->sysroot_etc_resolv_conf, + O_WRONLY|O_CREAT|O_NOCTTY|O_CLOEXEC, 0644); + if (fd == -1) { + reply_with_perror ("open: %s", rs->sysroot_etc_resolv_conf); + return -1; } + close(fd);
You must always check the return value from close for errors. This is because Linux can delay certain errors until the close call (particularly EIO). After creating the file in the sysroot, how will it get deleted? The code as written will create /etc/resolv.conf in random guest filesystems.
} - - /* Now that the guest's <sysroot>/etc/resolv.conf is out the way, we - * can create our own copy of the appliance /etc/resolv.conf. - */ - ignore_value (command (NULL, NULL, "cp", "/etc/resolv.conf", - rs->sysroot_etc_resolv_conf, NULL)); - + if (verbose) + fprintf (stderr, "mount %s to %s\n", src_etc_resolv, + rs->sysroot_etc_resolv_conf); + /* mount bind read only */ + r = command (NULL, NULL, "mount", "--bind", "-o ro", src_etc_resolv, + rs->sysroot_etc_resolv_conf, NULL); + if (r == -1) { + reply_with_perror ("mount: %s to %s failed\n", + src_etc_resolv, rs->sysroot_etc_resolv_conf); + goto error; + } + if (verbose) + fprintf (stderr, "mount %s to %s done\n", src_etc_resolv, + rs->sysroot_etc_resolv_conf); rs->mounted = true; + return 0;
- error: +error:
Here we add a blank line and reformat the label. If these changes need to be made then they should be done in a separate commit since they are not part of this logical change. But in this case I'd avoid making whitespace changes altogether.
free (rs->sysroot_etc_resolv_conf); - free (rs->sysroot_etc_resolv_conf_old); return -1; }
@@ -212,20 +206,10 @@ static void free_resolver_state (struct resolver_state *rs) { if (rs->mounted) { - unlink (rs->sysroot_etc_resolv_conf); - - if (rs->sysroot_etc_resolv_conf_old) { - if (verbose) - fprintf (stderr, "renaming %s to %s\n", rs->sysroot_etc_resolv_conf_old, - rs->sysroot_etc_resolv_conf); - - if (rename (rs->sysroot_etc_resolv_conf_old, - rs->sysroot_etc_resolv_conf) == -1) - perror ("error: could not restore /etc/resolv.conf"); - - free (rs->sysroot_etc_resolv_conf_old); - } - + /* umount sysroot_etc_resolv_conf */
This comment says nothing. If you wanted a comment here you could write something useful for the reader, or just leave it out.
+ if (verbose) + fprintf (stderr, "umount %s\n", rs->sysroot_etc_resolv_conf); + umount_ignore_fail(rs->sysroot_etc_resolv_conf);
Space before paren.
free (rs->sysroot_etc_resolv_conf); rs->mounted = false; } -- 2.52.0
The general plan is good and makes a big improvement, but the patch needs a few specific changes. Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
On Thu, May 28, 2026 at 6:31 PM Richard W.M. Jones <rjones@redhat.com> wrote:
On Thu, May 28, 2026 at 08:11:52AM -0400, Srihari Parimi wrote:
Implement bind mount to overwrite /etc/resolv.conf instead of the current method of rename followed by copy. The bind mount method works even if the file is designated as immutable
Signed-off-by: Srihari Parimi <sparimi@redhat.com>
This needs a 'Fixes: ...' line.
Fixed in Patch V2
--- daemon/sh.c | 86 ++++++++++++++++++++++------------------------------- 1 file changed, 35 insertions(+), 51 deletions(-)
diff --git a/daemon/sh.c b/daemon/sh.c index 74e003410..aab9a4534 100644 --- a/daemon/sh.c +++ b/daemon/sh.c @@ -150,8 +150,9 @@ free_bind_state (struct bind_state *bs) static int set_up_etc_resolv_conf (struct resolver_state *rs) { + char *src_etc_resolv = "/etc/resolv.conf";
const? If the compiler didn't warn about this then you're probably not enabling warnings. You should configure with:
./configure --enable-werror
Fixed - we need to add -Wwrite-strings to CFLAGS during configure to trigger the compiler error.
There are similar options on all the other projects which you should always use.
struct stat statbuf; - CLEANUP_FREE char *buf = NULL; + int r, fd = -1;
Why is fd initialized here? This stops GCC from detecting variables that are used when unset.
Fixed in Patch V2
rs->sysroot_etc_resolv_conf_old = NULL;
@@ -161,50 +162,43 @@ set_up_etc_resolv_conf (struct resolver_state *rs) reply_with_perror ("malloc"); goto error; } - - /* If /etc/resolv.conf exists, rename it to the backup file. Note - * that on Ubuntu it's a dangling symlink. + /* If /sysroot/etc/resolv.conf does not exist, create one for mount + * bind to succeed. lstat() call handles dnagling links */ - if (lstat (rs->sysroot_etc_resolv_conf, &statbuf) == 0) { - /* Make a random name for the backup file. */ - if (asprintf (&buf, "%s/etc/XXXXXXXX", sysroot) == -1) { - reply_with_perror ("asprintf"); - goto error; - } - if (random_name (buf) == -1) { - reply_with_perror ("random_name"); - goto error; - } - rs->sysroot_etc_resolv_conf_old = strdup (buf); - if (!rs->sysroot_etc_resolv_conf_old) { - reply_with_perror ("strdup"); - goto error; + if (lstat (rs->sysroot_etc_resolv_conf, &statbuf) != 0) {
Using '!= 0' is almost always wrong for system calls. The man page for lstat documents -1 as the error code, so check only that value.
Also what's the justification for lstat instead of stat?
Fixed in Patch V2 - lstat: if the file argument is a link, regardless of whether it is a dangling or a valid link, it returns success. bind mount succeeds in both cases - stat: if the file argument is a link which is dangling, it returns failure and we end up trying to create a file.
+ if (errno != ENOENT) { + reply_with_perror ("lstat: %s", rs->sysroot_etc_resolv_conf); + return -1; } - - if (verbose) - fprintf (stderr, "renaming %s to %s\n", rs->sysroot_etc_resolv_conf, - rs->sysroot_etc_resolv_conf_old); - - if (rename (rs->sysroot_etc_resolv_conf, - rs->sysroot_etc_resolv_conf_old) == -1) { - reply_with_perror ("rename: %s to %s", rs->sysroot_etc_resolv_conf, - rs->sysroot_etc_resolv_conf_old); - goto error; + /* create empty file */ + fd = open (rs->sysroot_etc_resolv_conf, + O_WRONLY|O_CREAT|O_NOCTTY|O_CLOEXEC, 0644); + if (fd == -1) { + reply_with_perror ("open: %s", rs->sysroot_etc_resolv_conf); + return -1; } + close(fd);
You must always check the return value from close for errors. This is because Linux can delay certain errors until the close call (particularly EIO).
After creating the file in the sysroot, how will it get deleted? The code as written will create /etc/resolv.conf in random guest filesystems.
Handling return value to close() Fixed the file which is created - deleting on errror or post umount operation
} - - /* Now that the guest's <sysroot>/etc/resolv.conf is out the way, we - * can create our own copy of the appliance /etc/resolv.conf. - */ - ignore_value (command (NULL, NULL, "cp", "/etc/resolv.conf", - rs->sysroot_etc_resolv_conf, NULL)); - + if (verbose) + fprintf (stderr, "mount %s to %s\n", src_etc_resolv, + rs->sysroot_etc_resolv_conf); + /* mount bind read only */ + r = command (NULL, NULL, "mount", "--bind", "-o ro", src_etc_resolv, + rs->sysroot_etc_resolv_conf, NULL); + if (r == -1) { + reply_with_perror ("mount: %s to %s failed\n", + src_etc_resolv, rs->sysroot_etc_resolv_conf); + goto error; + } + if (verbose) + fprintf (stderr, "mount %s to %s done\n", src_etc_resolv, + rs->sysroot_etc_resolv_conf); rs->mounted = true; + return 0;
- error: +error:
Here we add a blank line and reformat the label. If these changes need to be made then they should be done in a separate commit since they are not part of this logical change. But in this case I'd avoid making whitespace changes altogether.
Fixed in Patch V2
free (rs->sysroot_etc_resolv_conf); - free (rs->sysroot_etc_resolv_conf_old); return -1; }
@@ -212,20 +206,10 @@ static void free_resolver_state (struct resolver_state *rs) { if (rs->mounted) { - unlink (rs->sysroot_etc_resolv_conf); - - if (rs->sysroot_etc_resolv_conf_old) { - if (verbose) - fprintf (stderr, "renaming %s to %s\n", rs->sysroot_etc_resolv_conf_old, - rs->sysroot_etc_resolv_conf); - - if (rename (rs->sysroot_etc_resolv_conf_old, - rs->sysroot_etc_resolv_conf) == -1) - perror ("error: could not restore /etc/resolv.conf"); - - free (rs->sysroot_etc_resolv_conf_old); - } - + /* umount sysroot_etc_resolv_conf */
This comment says nothing. If you wanted a comment here you could write something useful for the reader, or just leave it out.
Removed
+ if (verbose) + fprintf (stderr, "umount %s\n", rs->sysroot_etc_resolv_conf); + umount_ignore_fail(rs->sysroot_etc_resolv_conf);
Space before paren.
Added the space
free (rs->sysroot_etc_resolv_conf); rs->mounted = false; } -- 2.52.0
The general plan is good and makes a big improvement, but the patch needs a few specific changes.
Thanks,
Rich.
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top
participants (2)
-
Richard W.M. Jones -
Srihari Parimi