[PATCH 0/4] Provide guestmount --pid-file and document possible race when unmounting FUSE filesystems.
10:13 a.m.
The full description of this bug is here:
https://bugzilla.redhat.com/show_bug.cgi?id=838592
and the effect it has on OpenStack is described here:
https://bugzilla.redhat.com/show_bug.cgi?id=835466#c9
Rich.
10:13 a.m.
New subject: [PATCH 1/4] fuse: Document race condition possible with fusermount.
From: "Richard W.M. Jones" <rjones(a)redhat.com>
See also:
https://bugzilla.redhat.com/show_bug.cgi?id=835466#c9
---
fuse/guestmount.pod | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/fuse/guestmount.pod b/fuse/guestmount.pod
index a0bc6f1..c37301c 100644
--- a/fuse/guestmount.pod
+++ b/fuse/guestmount.pod
@@ -106,6 +106,22 @@ namespace using the Linux-specific
L<clone(2)>/L<unshare(2)> flag
C<CLONE_NEWNS>. Unfortunately at the moment this requires root and we
would also probably need to add it as a feature to guestmount.
+=head2 Race conditions possible when shutting down the connection
+
+When C<fusermount -u> exits, guestmount may still be running and
+cleaning up the mountpoint. The disk image will not be fully
+finalized.
+
+This means that scripts like the following have a nasty race
+condition:
+
+ guestmount -a disk.img -i /mnt
+ # copy things into /mnt
+ fusermount -u /mnt
+ # immediately try to use 'disk.img' <-- UNSAFE
+
+The solution is to spin waiting for the guestmount process to exit.
+
=head1 OPTIONS
=over 4
--
1.7.10.4
10:13 a.m.
New subject: [PATCH 2/4] fuse: Link to mount-local documentation in guestmount man page.
From: "Richard W.M. Jones" <rjones(a)redhat.com>
---
fuse/guestmount.pod | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/fuse/guestmount.pod b/fuse/guestmount.pod
index c37301c..d459045 100644
--- a/fuse/guestmount.pod
+++ b/fuse/guestmount.pod
@@ -122,6 +122,10 @@ condition:
The solution is to spin waiting for the guestmount process to exit.
+Note that if you use the C<guestfs_mount_local> API directly (see
+L<guestfs(3)/MOUNT LOCAL>) then it is much easier to write a safe,
+race-free program.
+
=head1 OPTIONS
=over 4
@@ -354,7 +358,7 @@ L<virt-inspector(1)>,
L<virt-cat(1)>,
L<virt-edit(1)>,
L<virt-tar(1)>,
-L<guestfs(3)>,
+L<guestfs(3)/MOUNT LOCAL>,
L<http://libguestfs.org/>;,
L<http://fuse.sf.net/>;.
--
1.7.10.4
10:13 a.m.
New subject: [PATCH 3/4] fuse: Add guestmount --pid-file option (RHBZ#838592).
From: "Richard W.M. Jones" <rjones(a)redhat.com>
---
fuse/guestmount.c | 46 +++++++++++++++++++++++++++++++++++++++++++++-
fuse/guestmount.pod | 8 +++++++-
2 files changed, 52 insertions(+), 2 deletions(-)
diff --git a/fuse/guestmount.c b/fuse/guestmount.c
index 0c2e2dc..fd31726 100644
--- a/fuse/guestmount.c
+++ b/fuse/guestmount.c
@@ -42,6 +42,8 @@
#include "guestmount.h"
#include "options.h"
+static int write_pid_file (const char *pid_file, pid_t pid);
+
#ifndef HAVE_FUSE_OPT_ADD_OPT_ESCAPED
/* Copied from lib/fuse_opt.c and modified.
* Copyright (C) 2001-2007 Miklos Szeredi <miklos(a)szeredi.hu>
@@ -119,6 +121,7 @@ usage (int status)
" -m|--mount dev[:mnt[:opts]] Mount dev on mnt (if omitted,
/)\n"
" -n|--no-sync Don't autosync\n"
" -o|--option opt Pass extra option to FUSE\n"
+ " --pid-file filename Write PID to filename\n"
" -r|--ro Mount read-only\n"
" --selinux Enable SELinux support\n"
" -v|--verbose Verbose messages\n"
@@ -161,6 +164,7 @@ main (int argc, char *argv[])
{ "mount", 1, 0, 'm' },
{ "no-sync", 0, 0, 'n' },
{ "option", 1, 0, 'o' },
+ { "pid-file", 1, 0, 0 },
{ "ro", 0, 0, 'r' },
{ "rw", 0, 0, 'w' },
{ "selinux", 0, 0, 0 },
@@ -184,6 +188,7 @@ main (int argc, char *argv[])
int dir_cache_timeout = -1;
int do_fork = 1;
char *fuse_options = NULL;
+ char *pid_file = NULL;
struct guestfs_mount_local_argv optargs;
@@ -227,6 +232,8 @@ main (int argc, char *argv[])
echo_keys = 1;
} else if (STREQ (long_options[option_index].name, "live")) {
live = 1;
+ } else if (STREQ (long_options[option_index].name, "pid-file")) {
+ pid_file = optarg;
} else {
fprintf (stderr, _("%s: unknown long option: %s (%d)\n"),
program_name, long_options[option_index].name, option_index);
@@ -406,8 +413,12 @@ main (int argc, char *argv[])
exit (EXIT_FAILURE);
}
- if (pid != 0) /* parent */
+ if (pid != 0) { /* parent */
+ if (write_pid_file (pid_file, pid) == -1)
+ _exit (EXIT_FAILURE);
+
_exit (EXIT_SUCCESS);
+ }
/* Emulate what old fuse_daemonize used to do. */
if (setsid () == -1) {
@@ -426,6 +437,11 @@ main (int argc, char *argv[])
close (fd);
}
}
+ else {
+ /* not forking, write PID file anyway */
+ if (write_pid_file (pid_file, getpid ()) == -1)
+ exit (EXIT_FAILURE);
+ }
/* Main loop. */
r = guestfs_mount_local_run (g);
@@ -435,5 +451,33 @@ main (int argc, char *argv[])
r = -1;
guestfs_close (g);
+ /* Don't delete PID file until the cleanup has been completed. */
+ if (pid_file)
+ unlink (pid_file);
+
exit (r == 0 ? EXIT_SUCCESS : EXIT_FAILURE);
}
+
+static int
+write_pid_file (const char *pid_file, pid_t pid)
+{
+ FILE *fp;
+
+ if (pid_file == NULL)
+ return 0;
+
+ fp = fopen (pid_file, "w");
+ if (fp == NULL) {
+ error:
+ perror (pid_file);
+ return -1;
+ }
+
+ if (fprintf (fp, "%d\n", pid) == -1)
+ goto error;
+
+ if (fclose (fp) == -1)
+ goto error;
+
+ return 0;
+}
diff --git a/fuse/guestmount.pod b/fuse/guestmount.pod
index d459045..fdcf133 100644
--- a/fuse/guestmount.pod
+++ b/fuse/guestmount.pod
@@ -120,7 +120,9 @@ condition:
fusermount -u /mnt
# immediately try to use 'disk.img' <-- UNSAFE
-The solution is to spin waiting for the guestmount process to exit.
+The solution is to use the I<--pid-file> option to write the
+guestmount PID to a file, then after fusermount spin waiting for this
+PID to exit.
Note that if you use the C<guestfs_mount_local> API directly (see
L<guestfs(3)/MOUNT LOCAL>) then it is much easier to write a safe,
@@ -282,6 +284,10 @@ to the chosen values.
=back
+=item B<--pid-file filename>
+
+Write the PID of the guestmount worker process to C<filename>.
+
=item B<-r>
=item B<--ro>
--
1.7.10.4
10:13 a.m.
New subject: [PATCH 4/4] fuse: Add regression test for RHBZ#838592.
From: "Richard W.M. Jones" <rjones(a)redhat.com>
---
fuse/Makefile.am | 2 +-
fuse/test-fuse-umount-race.sh | 77 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 78 insertions(+), 1 deletion(-)
create mode 100755 fuse/test-fuse-umount-race.sh
diff --git a/fuse/Makefile.am b/fuse/Makefile.am
index 1ab8c7b..8176221 100644
--- a/fuse/Makefile.am
+++ b/fuse/Makefile.am
@@ -73,7 +73,7 @@ stamp-guestmount.pod: guestmount.pod
# Tests.
if ENABLE_APPLIANCE
-TESTS = test-fuse.sh
+TESTS = test-fuse.sh test-fuse-umount-race.sh
endif ENABLE_APPLIANCE
TESTS_ENVIRONMENT = \
top_builddir=.. \
diff --git a/fuse/test-fuse-umount-race.sh b/fuse/test-fuse-umount-race.sh
new file mode 100755
index 0000000..a5a6e21
--- /dev/null
+++ b/fuse/test-fuse-umount-race.sh
@@ -0,0 +1,77 @@
+#!/bin/bash -
+# libguestfs
+# Copyright (C) 2012 Red Hat Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+# https://bugzilla.redhat.com/show_bug.cgi?id=838592
+# This tests that the --pid-file option can be used to fix the race.
+
+unset CDPATH
+set -e
+#set -v
+
+if [ -n "$SKIP_TEST_FUSE_SH" ]; then
+ echo "$0: test skipped because environment variable is set."
+ exit 0
+fi
+
+if [ ! -w /dev/fuse ]; then
+ echo "SKIPPING guestmount test, because there is no /dev/fuse."
+ exit 0
+fi
+
+rm -f test.qcow2 test-copy.qcow2 test.pid
+rm -rf mp
+
+# Make a copy of the Fedora image so we can write to it then discard it.
+qemu-img create -F raw -b ../tests/guests/fedora.img -f qcow2 test.qcow2
+
+mkdir mp
+./guestmount -a test.qcow2 -m /dev/VG/Root --pid-file test.pid mp
+cp $0 mp/test-umount
+
+count=10
+while ! fusermount -u mp && [ $count -gt 0 ]; do
+ sleep 1
+ ((count--))
+done
+if [ $count -eq 0 ]; then
+ echo "$0: fusermount failed after 10 attempts"
+ exit 1
+fi
+
+# Wait for guestmount to exit.
+count=10
+while kill -0 `cat test.pid` 2>/dev/null && [ $count -gt 0 ]; do
+ sleep 1
+ ((count--))
+done
+if [ $count -eq 0 ]; then
+ echo "$0: wait for guestmount to exit failed after 10 seconds"
+ exit 1
+fi
+
+# It should now be safe to copy and read the disk image.
+cp test.qcow2 test-copy.qcow2
+
+if [ "$(../fish/guestfish -a test-copy.qcow2 --ro -i is-file /test-umount)" !=
"true" ]; then
+ echo "$0: test failed"
+ exit 1
+fi
+
+rm test.qcow2 test-copy.qcow2
+rm -f test.pid
+rm -r mp
--
1.7.10.4
4400
days inactive
4400
days old
4 comments
1 participants
participants (1)
-
Richard W.M. Jones