Bug#369626: [Buildd-tools-devel] Bug#369626: schroot: rm -rf in file chroot cleanup destroys real /home if umount fails

[Roger Leigh]

Andreas Bombe <bombe at informatik.tu-muenchen.de> writes:

> On Wed, May 31, 2006 at 10:53:18AM +0100, Roger Leigh wrote:
>> Andreas Bombe <aeb at debian.org> writes:
>> 
>> > The session cleanup in 10mount ignores failures of umount invocations
>> > and cleanup continues.  In the case of file chroots with a /home bind
>> > mount that failed to umount, the rm -rf in 05file blindly descends into
>> > the system /home with obvious unpretty results.

>> There are a few possibilities here.
>> 
>> 1) 10mount should exit with an error if umount fails.
>> 
>>    Caveat: if the session is ended with the setup scripts having
>>    failed, this would require manual cleanup by the system admin.
>>    This needs additional work in session::setup_chroot() in
>>    sbuild-session.cc, so that the session is not ended if the scripts
>>    fail.  This means not removing the session file from
>>    /var/lib/schroot/session/ on failure.
>> 
>>    Currently, because of the above consideration, the "setup-stop"
>>    phase of the session scripts can not fail.
>
> If a umount fails it will require manual admin intervention anyway so
> that wouldn't make much of a difference.  Making the rm -rf safe is
> still required in any case, I'd say.

OK, I agree here.  In the patch below, any umount failure will abort
the cleanup.

>> 2) 05file must check if any filesystems are mounted under the chroot
>>    root before running rm -rf.  Is there a portable and reliable way
>>    of doing this?  Would
>> 
>>      if mount | grep "$CHROOT_MOUNT_LOCATION"; then
>>        :
>>      else
>>        rm -rf "$CHROOT_MOUNT_LOCATION" || true
>>      fi
>> 
>>    be sufficient?
>
> I don't think that is safe.  It depends on all mounts being recorded in
> /etc/mtab, which is not the case if something *inside* the chroot
> mounted something, for example.

> I thought about rm with a "do not cross filesystems" option, still that
> wouldn't help because binds may well be on the same filesystem.  There
> are no usable criteria for using "find ... -exec rm ..." either.

To work around this, I wrote a program called schroot-listmounts,
which uses /proc/mounts to get a list:

./schroot/schroot-listmounts -m /srv/chroot/sid
/srv/chroot/sid/tmp
/srv/chroot/sid/home
/srv/chroot/sid/dev/shm
/srv/chroot/sid/dev/pts
/srv/chroot/sid

The shell script can read this list and unmount them in turn.

> The only way I know to be sure there are no submounts is to mount --bind
> the chroot to a temporary location and rm -rf there, then unmount the
> temporary bind and rmdir the chroot.
>
> The rmdir will fail safely if the chroot isn't empty then. Even before,
> the rm -rf of the temp bind will fail safely upon trying to remove an
> empty directory used as a mount point in the chroot.

I think I now have this solved.  If the chroot directories are
unmounted, schroot-listmounts will list nothing:

./schroot/schroot-listmounts -m /var/lib/schroot/mount/testsnap-c4e53f96-e507-40e8-8cf4-fcfd31c17c32
[no output]

so in this case we can be sure it's safe to "rm -rf".  If there's any
output at all, we know it's not safe.


I've attached a patch for this.  Note: it's not yet tested; this is
just to give you an idea of what I'm thinking of as a solution.  If
the testing works out, and you are happy with it, I'll include this in
the next release.

 ChangeLog                             |   43 ++++++++
 NEWS                                  |   10 +
 debian/changelog                      |    3
 schroot/Makefile.am                   |   10 +
 schroot/sbuild-chroot-block-device.cc |    3
 schroot/sbuild-chroot-block-device.h  |    9 -
 schroot/sbuild-chroot-file.cc         |    5
 schroot/sbuild-chroot-file.h          |    9 -
 schroot/sbuild-chroot-lvm-snapshot.cc |    5
 schroot/sbuild-chroot-lvm-snapshot.h  |    9 -
 schroot/sbuild-chroot-plain.cc        |    5
 schroot/sbuild-chroot-plain.h         |    9 -
 schroot/sbuild-chroot.cc              |   13 ++
 schroot/sbuild-chroot.h               |   42 +++++++
 schroot/sbuild-session.cc             |    6 -
 schroot/sbuild-util.cc                |   13 ++
 schroot/sbuild-util.h                 |   12 ++
 schroot/schroot-listmounts-options.cc |   83 +++++++++++++++
 schroot/schroot-listmounts-options.h  |   59 +++++++++++
 schroot/schroot-listmounts.cc         |  181 ++++++++++++++++++++++++++++++++++
 schroot/setup/05file                  |   15 ++
 schroot/setup/10mount                 |   31 +----
 test/sbuild-chroot.cc                 |    3
 23 files changed, 522 insertions(+), 56 deletions(-)


Regards,
Roger

-- 
Roger Leigh
                Printing on GNU/Linux?  http://gutenprint.sourceforge.net/
                Debian GNU/Linux        http://www.debian.org/
                GPG Public Key: 0x25BFB848.  Please sign and encrypt your mail.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: schroot-safecleanup.patch
Type: text/x-patch
Size: 25825 bytes
Desc: Safe unmount and cleanup patch
Url : http://lists.alioth.debian.org/pipermail/buildd-tools-devel/attachments/20060609/1568e651/schroot-safecleanup-0001.bin