Comment 4 for bug 473615

Revision history for this message
Martin Pitt (pitti) wrote :

Taking some notes while I review this, thinking aloud.

- Code-wise, the init script(s) and upstart job differ in the sense that the ones marked as "noearly" are started after the other ones with the init script, whereas the order can potentially change with the upstart job (which only has INITSTATE="init", not two phases). If I read the upstart jobs correctly, then it could even happen that both the init script and the upstart job run at the same time?

If the order of cryptdisks-enable.conf vs. rcS.conf isn't enforced by some other dependency/magic/coincidence, then it already seems bad to run both. So this speaks in favor of removing the init scripts, even though it means that users might suddenly see a change in the order of which encrypted disks are mounted at startup.

- Boot order: /etc/init.d/cryptdisks LSB header says to start after "raid2 mdadm lvm2 evms". The intention is certainly to catch encrypted disks on RAID/LVM volumes. "raid2" and "lvm2" aren't provided by any init script, "evms" is long gone, so we don't need to care about those. /etc/init.d/mdadm provides "mdadm", but mdadm doesn't have a startup init script (just an udev rule). Thus, the init script dependencies intended by the LSB header have never been implemented by our init.d priorities.

I assume this is what you meant with "So both before and after, this is racy".

The actual bzr change looks correct to me wrt. its intention. Two small nitpicks: changelog has an "ad" typo (meaning "and"), and in the dpkg version the "lt-nl" comparison would suffice (this would fix the corner case where these symlinks were deliberately created manually before installation; very pathological, though).

> alternative approach would be to also hook the upstart job into a udev rule at the same time, to completely eliminate the raciness.

This would mean to run all the parsing, modprobe, dmsetup mknodes, etc. code each time an ecrypted device gets added (which could be on an USB key). Semantically this would probably be correct, since these could be handled in crypttab as well. I'm just not sure about how much overhead this would introduce for a running session.

So I propose we do this small SRU for now, and test it with various common setups (encrypted LVM, encrypted root, encrypted root+home, encrypted home, encrypted swap). In lucid we ought to replace the upstart job with an udev rule based solution and also clean up the scriptery around it (much of it looks obsolete, and we still have gems like "CRYPTDISKS_CHECK=vol_id" in /etc/default/cryptdisks, which is doomed to fail).