[systemd-devel] [correct PATCH v2] dev-root.device is not active, results in an umount spree

[Martin Pitt]

Lennart Poettering [2015-05-18 23:04 +0200]:
> On Mon, 18.05.15 16:08, Martin Pitt (martin.pitt at ubuntu.com) wrote:
> 
> > Martin Pitt [2015-05-17 15:54 +0200]:
> > > This fixes the original "systemd immediately unmounts my mounts" bug,
> > > but not for very long: If you remount or unmount just one mount on a
> > > tentative device, mountinfo changes, and device_found_node() now calls
> > > device_update_found_one() with "add == 0". Then
> > > 
> > >         n = add ? (d->found | found) : (d->found & ~found);
> > > 
> > > would unset the previous DEVICE_FOUND_MOUNT flag, leaving 0 (i. e.
> > > DEVICE_NOT_FOUND). Thus the previously "tentative" device would once
> > > again be set to "dead", and systemd would unmount all other mounts
> > > from it. This must never happen, we simply can't declare tentative
> > > devices as dead and clean up their unmounts, this only works for
> > > "plugged" ones that we know via udev.
> > 
> > Eek, I attached the wrong 0002- patch, sorry about that. The above is
> > fixed by the attached patch, please ignore 0002- from the previous
> > mail. For completeness I also re-attach 0001- again (unchanged).
> 
> Still not getting what the purpose of the 0002 patch is, even in this
> revision...
> 
> Please elaborate!

I'll try to explain step by step:

Say you are booting with plan9 fs, a container, or some other thing
with a mount that references a device "/dev/foo" which isn't actually
available as /dev/foo and in udev. Note that you can't reproduce this
in nspawn, as it forcefully mounts /sys as r/o, which triggers the
unit_type_supported(UNIT_DEVICE) safety check in unit_add_node_link();
this happens in environments with writable /sys.

 - Boot, dev-foo.device becomes DEVICE_FOUND_MOUNT/tentative

 - Do some more mounts from /dev/foo, e. g.

   mkdir /tmp/etc /tmp/boot
   mount -o bind /etc /tmp/etc
   mount -o bind /boot /tmp/boot

   (In practice, you'd probably do such bind mounts with nspawn --bind
   or lxc.mount.entry)

   tmp-etc.mount and tmp-boot.mount will be BindsTo=dev-foo.device,
   and dev-foo.device's status is still unchanged
   (DEVICE_FOUND_MOUNT/tentative)

 - Now do umount /tmp/boot. This *also* umounts /tmp/etc!

   Journal says

     systemd[1]: Requested transaction contradicts existing jobs: Resource deadlock avoided
     systemd[1]: Unmounted /tmp/etc.

  and dev-sda3.device is now inactive/dead, which tears down
  the bound tmp-etc.mount. Boom!

Reason: Unmounting /tmp/boot triggers
device_update_found_one(found==DEVICE_FOUND_MOUNT, add==0).
d->found previously was DEVICE_FOUND_MOUNT, and this

     n = add ? (d->found | found) : (d->found & ~found);

computes the new state to 0 (i. e. DEVICE_NOT_FOUND), and calls
device_set_state(DEVICE_DEAD). Thus here we (1) don't consider that
dev-foo.device is still bound by other units (tmp-etc.mount) and (2)
lose the information that dev-foo.device is tentative.

So the problem is that this tentative → dead transition only works if
a device is referenced once, but causes overzealous unmounts if there
are more references.

We need a reference count, or check if the device is bound by other
device still. Come to think of it now, we actually already have that:

  Id=dev-foo.device
  BoundBy=tmp-boot.mount tmp-etc.mount

But my patch doesn't take that into account yet.

> Devices that show up in /proc/self/mountinfo or /proc/swap, and then
> disappear again, without ever showing up in udev, need to be cleaned
> up.

That's right, I forgot about that, thanks for catching! So current
master is too overzealous, and my current patch never cleans up. So
this needs some more work.

Thanks,

Martin
-- 
Martin Pitt                        | http://www.piware.de
Ubuntu Developer (www.ubuntu.com)  | Debian Developer  (www.debian.org)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.freedesktop.org/archives/systemd-devel/attachments/20150519/be8983bb/attachment.sig>