[buildd-tools-devel] Bug#593516: Bug#593516: schroot: race condition in /proc/mounts can break cleanup

[Greg Price]

On Thu, Aug 19, 2010 at 3:21 PM, Roger Leigh <rleigh at codelibre.net> wrote:
>> Currently I'm working around the issue with a flock around the body of
>> do_umount_all().  That's easy and is enough to address the problem
>> when no other umounts are happening on the system.
>
> If you would like this fix including, I'll be happy to accept a
> patch implementing this.  Are you doing this in the shell script?
> If it's possible to directly lock /proc/mounts with fcntl this could
> be addressed directly in schroot-listmounts using sbuild::file_lock,
> which wraps fcntl.

Sure, patch copied below.  It's not possible for userspace to directly
lock /proc/mounts; the best we can do is an advisory lock that we
ourselves respect.  So I placed a flock around the invocations of both
schroot-listmounts and umount, which are the only such invocations in
the schroot source tree.


> Has a bug report been filed against the Linux kernel or on the
> kernel mailing list?  A fix in the kernel would be ideal, and they
> should probably also be notified.

I haven't filed such a bug.  I'm debating whether to send a message to
the LKML.  Honestly, I suspect this will follow many other well-loved
quirks in that it is difficult to fix, and the best solution to hope
for may be for more people to be aware of it and handle it with
strategies like the separate-namespace approach you suggest.  Maybe
I'll mail the LKML with a strawman patch, and see if someone can prove
me wrong. =)


>> A real fix would
>> probably have to be for schroot to record for itself the list of
>> mounts it makes inside a session's tree, and rely on that list instead
>> of on /proc/mounts.
>
> This wouldn't be robust enough, I'm afraid.  Processes inside the
> chroot (or outside, for that matter) could mount (and umount)
> filesystems after initial setup.  Mounts might also occur in
> additional custom setup scripts.  As a result, we really do need
> to get a definitive list of mounts at the session end in order to
> clean up correctly.

Oof.  Yes, I see.


> At least on Linux, one other approach would be to make use of
> per-process namespaces which would TTBOMK allow us to skip the
> umounts completely since it's automatically cleaned up when the
> last process in the namespace exits.  This has only recently
> become viable without significant rearchitecting of schroot
> though (IIRC it's now possible to connect to an existing namespace
> without a parent-child relationship, though I'm not familiar with
> the details).

That sounds like a potential solution.  If you could give a separate
mount namespace to each schroot session, that would definitely fix the
problem -- the list that /proc/mounts reads is per-namespace, so there
is no race between unmounting in one namespace and reading
/proc/mounts in another.

Greg



--- schroot-1.3.2.orig/etc/setup.d/10mount
+++ schroot-1.3.2/etc/setup.d/10mount
@@ -53,6 +53,7 @@
 do_umount_all()
 {
     if [ -d "$1" ]; then
+       ( flock 9
        mounts="$("$LIBEXEC_DIR/schroot-listmounts" -m "$1")"
        if [ "x$mounts" != 'x' ]; then
             echo "$mounts" |
@@ -63,6 +64,7 @@
                umount "$mountloc" || exit 1
             done || exit 1
        fi
+       ) 9>/var/lock/umount
     fi
 }