[lxc-devel] lxc-destroy erased my hard disc

[Dan Fandrich]

On Tue, Jan 14, 2014 at 04:48:57PM -0600, Serge Hallyn wrote:
> Quoting Dan Fandrich (dan at coneharvesters.com):
> > On Mon, Jan 13, 2014 at 06:23:24PM -0600, Serge Hallyn wrote:
> > > Gah, that really does suck, sorry.  The need to run as root has always
> > > been annoying.  We're finally at the point that some containers can be
> > > created and run entirely as non-root, so that your host should be a lot
> > > safer.
> > 
> > That's definitely a positive development!
> > 
> > > > This was with lxc 0.8.0 on x86 Linux (the relevant 0.9.0 code looks
> > > > substantially similar). It happened shortly after installing lxc for the first
> > > > time to try it out. I believe what must have happened was that the container I
> > > > created wasn't shut down properly and still had its bind mounts for /lib etc.
> > > 
> > > lxc-start would not have done any mounting in the host's mount
> > > namespace, so that scenario shouldn't be possible.
> > > 
> > > So I would actually guess that lxc-create (actually the template) was to
> > > blame for leaving the bind mounts.  Since it looks like you're not using
> > > a stock template I'm not sure how you created the container.
> > 
> > That's quite possible; I'm not completely sure yet how all the lxc
> > moving parts fit together yet. I was using the stock busybox template, but
> > there were a couple of configuration problems with lxc, so it wasn't a single
> > perfectly clean create/start/stop.
> > 
> > > > in place. The container must have been down enough to be considered not running
> > > > according to lxc-info, so lxc-destroy helpfully rm -rf'ed it, including the
> > > > contents of the bind mounts which pointed into my host system.
> > > > 
> > > > rm was given the --one-file-system option, but it didn't prevent this from
> > > > occurring, presumably because the device ID of the container location was the
> > > > same as the device ID of the /lib of the host. After looking at the source to
> > > > lxc-destroy, I'm surprised this kind of problem doesn't happen more often--it
> > > 
> > > We do discourage bind-mounting from the host fs into the container, but
> > > historcally it was considered useful (and may again) for checkpoint/restart.
> > > 
> > > > is not at all defensively written (error codes ignored, arguments not properly
> > > > quoted, this problem I'm writing about, etc.), so I was glad to see that
> > > > lxc-destroy has since been reimplemented. However, on briefly looking at the
> > > > new source, it seems that the same problem can still occur. There's a
> > > > comparison of device IDs before deleting files (presumably, to try to
> > > > accomplish the same thing as rm's --one-file-system option), but my situation
> > > > of bind mounts on the same filesystem device means once again that wouldn't be
> > > > effective in preventing this erroneous deletion.
> > > > 
> > > > I strongly suggest improving the checks before deletion to avoid this kind of
> > > > problem in the future. This may be as simple as checking the bind mount
> > > > points against /proc/mounts before starting the destroy process.
> > > 
> > > I can see no reason not to add a check against /proc/mounts for any
> > > entries under $rootfs (if it is a directory).  That should definately be
> > > added.
> > 
> > I'm attaching the patch I added to my system to try to stop this from happening
> > again. The ugly sed to create the grep regex is because rootdev ends up with
> > two leading slashes instead of one, which wouldn't otherwise match. I suspect
> > that's simply a bug in the sed two lines before, but I wasn't sure if there
> > were implications for lvdisplay or btrfs in fixing it, so I left it.
> 
> The patch doesn't actually seem to be attached?

Whoops--It's attached now.

> Note though that lxc-destroy is now a c program.  I don't recall whether
> someone actually stepped up to Stéphane's call for maintainers for older
> versions - it would be a good thing to have.

>>> Dan
-------------- next part --------------
Check that there are no bind mounts set up before destroying a container.
Otherwise, the contents of that bind will be destroyed if a container
was not properly shut down.

--- a/src/lxc/lxc-destroy.in    2014-01-13 22:45:22.000000000 +0100
+++ b/src/lxc/lxc-destroy.in    2014-01-13 23:13:40.258095984 +0100
@@ -125,6 +125,11 @@
 # else, ignore it. We'll support deletion of others later.
 rootdev=`grep lxc.rootfs $lxc_path/$lxc_name/config 2>/dev/null | sed -e 's/^[^/]*/\//'`
 if [ -n "$rootdev" ]; then
+    if grep -q "$(sed 's@^/\+/@/@' <<<"$rootdev")" /proc/mounts; then
+      echo "$(basename $0): '$lxc_name' is not properly stopped; aborted" >&2
+      exit 1
+    fi
+
     if [ -b "$rootdev" -o -h "$rootdev" ]; then
         lvdisplay $rootdev > /dev/null 2>&1
         if [ $? -eq 0 ]; then
@@ -137,10 +142,10 @@
             btrfs subvolume delete "$rootdev"
         else
             # In case rootfs is not under $lxc_path/$lxc_name, remove it
-            rm -rf --one-file-system --preserve-root $rootdev
+            rm -rf --one-file-system --preserve-root "$rootdev"
         fi
     fi
 fi
 
 # recursively remove the container to remove old container configuration
-rm -rf --one-file-system --preserve-root $lxc_path/$lxc_name
+rm -rf --one-file-system --preserve-root "$lxc_path/$lxc_name"