Re: [libvirt] [RFC PATCH 5/5] qemu: snapshot: Improve approach to deal with snapshot metadata

[warning - long email ahead. tl;dr: this patch is not ready yet, because I found a problem in the design] On 06/12/2014 09:02 AM, Peter Krempa wrote: It's also a nice diffstat :) Yay - moving the code in the direction that the comments hinted we should be moving. One thing this code was trying to do is hoist all allocation up front, so that if we hit OOM, we have not made any permanent changes and can therefore safely roll back. But here, you've delayed the allocation until after we've already made some changes. You'll need to hoist the call to virStorageSourceCopy() to occur in the same place as the code it is replacing. This line looks like it is in the correct location - we are merely manipulating the linked list in place to insert the newly-created wrapper. I do see a potential problem, though. Lots of graphics below, in three major demo sections: ================== In the _existing_ code, we are doing in-place modification of _portions_ of the storage source, then calling the security/audit/lease manager on the modified source. Why? Because the portions of the disk that we do NOT modify include the labeling information, and that's in part because historically we had multiple pieces of labeling information stored only once at the disk level. Graphically, our current behavior has a lifecycle something like this, when booting a domain with a chain of 2 and snapshotting it into a chain of 3: at parse time, we only learn the top level, and the fact that we need to generate multiple labels: disk: dev = vda src = +-------------------+ + path = mid + + rwlabel = NULL + + rolabel = NULL + + backing = NULL + + mid is unlabeled + +-------------------+ at domain start time, we then probe the whole chain, which learns more file names, but does not populate label information in the backing files; we also perform labeling: disk: dev = vda src = +-------------------+ +--------------------+ + path = mid + + path = base + + rwlabel = foo + + rwlabel = NULL + + rolabel = bar + + rolabel = NULL + + backing = ---------->+ backing = NULL + + mid has label foo + + base has label bar + +-------------------+ +--------------------+ Now the snapshot comes along, and we are adding a new layer: snapshot: dev = vda src = +-------------------+ + path = top + + rwlabel = NULL + + rolabel = NULL + + backing = NULL + + top is unlabeled + +-------------------+ so we label it - but with what label? The label was tied to 'mid'. Hence, the in-place swapping, so that we have: disk: dev = vda src = +-------------------+ + path = top + + rwlabel = foo + + rolabel = bar + + backing = NULL + + top has label foo + +-------------------+ long enough to give us a label. If successful, the code takes a shortcut at this point, and just reprobes the chain, finally leaving us with the desired chain (if unsuccessful, we undo the temporary changes by restoring src to mid, and backing to point to base). But note that the reprobe does NOT know what label backing elements actually have; what's more, mid is still labeled 'foo' for read-write privileges, even though after the snapshot we could have safely downgraded it to label 'bar' for read-only privileges: disk: dev = vda src = +-------------------+ +-------------------+ +--------------------+ + src = top + + src = mid + + src = base + + rwlabel = foo + + rwlabel = NULL + + rwlabel = NULL + + rolabel = bar + + rolabel = NULL + + rolabel = NULL + + backing = ---------->+ backing = ---------->+ backing = NULL + + top has label foo + + mid has label ?? + + base has label ?? + +-------------------+ +-------------------+ +--------------------+ =============== But with your code, merely inserting (a copy of) the snapshot source at the front of the chain would leave us with: disk: dev = vda src = +-------------------+ +-------------------+ +--------------------+ + src = top + + src = mid + + src = base + + rwlabel = NULL + + rwlabel = foo + + rwlabel = NULL + + rolabel = NULL + + rolabel = bar + + rolabel = NULL + + backing = ---------->+ backing = ---------->+ backing = NULL + + top has label foo + + mid has label foo + + base has label bar + +-------------------+ +-------------------+ +--------------------+ it has the benefit of remembering what we have previously labeled things, but has the absolute failure of forgetting what labels we have generated for the <disk> (since those labels are buried in the backing chain instead of the top element of the chain). For your code to work, you have to ALSO transfer the security information into the top of the chain, probably best done via a new helper function. =============== Here's what I envision that we should be doing instead. The existing code is tracking two different labels at the top level source, and none at any other layer. Instead, we should be tracking both read-write and read-only label templates at the <disk> level, then for any given virStorageSourcePtr, have a notion only of a current label. The security manager only needs access to the disk label templates (the generated read-write and read-only labels don't change over the life of the disk), as well as the current label per disk, along with the ability to swap that label (we basically have three label states: unlabeled, read-only, and read-write - and use the security manager to switch between those states). So the same lifecycle now looks more like: original parse: disk: dev = vda rwlabel = NULL rolabel = NULL src = +-------------------+ + path = mid + + backing = NULL + + mid is unlabeled + +-------------------+ probe the backing chain, and generate labels: disk: dev = vda rwlabel = foo rolabel = bar src = +-------------------+ +--------------------+ + path = mid + + path = base + + backing = ---------->+ backing = NULL + + mid has label foo + + base has label bar + +-------------------+ +--------------------+ Parse the snapshot, and pass THAT storage source pointer to the security code, but reusing the label template from the disk, so that we get the correct label in-place: snapshot: dev = vda src = +-------------------+ + path = top + + backing = NULL + + top has label foo + +-------------------+ Wire up the chain: disk: dev = vda rwlabel = foo rolabel = bar src = +-------------------+ +-------------------+ +--------------------+ + src = top + + src = mid + + src = base + + backing = ---------->+ backing = ---------->+ backing = NULL + + top has label foo + + mid has label foo + + base has label bar + +-------------------+ +-------------------+ +--------------------+ and finally revoke the read-write label on mid: disk: dev = vda rwlabel = foo rolabel = bar src = +-------------------+ +-------------------+ +--------------------+ + src = top + + src = mid + + src = base + + backing = ---------->+ backing = ---------->+ backing = NULL + + top has label foo + + mid has label bar + + base has label bar + +-------------------+ +-------------------+ +--------------------+ and if a read-only backing file is shared by more than one chain (or even across more than one <domain>), now you can see why reference counting becomes important, for knowing when it is safe to revoke read-write or even to forbid granting read-write. Furthermore, once we start allowing backing chain as user input instead of crawling the chain ourselves, the per-source label can be overridden for any element of the chain, instead of the current limitation of only having an override label at the top level. I don't think we are going to get to the idea setup of security management overnight, so in the meantime, we need some crutches to make sure that your code is not botching things. ================= So, with that explanation out of the way, I'm resuming the review. I think we can get away with your method in the short term (the 2nd of the 3 sections above) IF you also manage to transfer labeling information to the active layer of the chain, but this patch needs a respin because in its current state it does not do that. Again, this is a late allocation; we risk OOM after making changes that can't be undone. I'd rather have two temporary virStorageSourcePtr objects allocated up front, and then plugged in at the end on success, rather than risk late allocation failure. Arggh - I ran out of time to review the rest. But hopefully this gives you some ideas to think about. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org