On 06/12/2014 09:02 AM, Peter Krempa wrote: I've definitely been wishing for this; as our struct gets more complicated, piece-wise transfers into a temporary will not be robust, while copying into a temporary can be. Did you need to export the new symbol in libvirt_private.syms? Swap this line... ...here, and use src->nsecrets instead of ret->nsecrets in the VIR_ALLOC_N. Why? Because if VIR_ALLOC_N fails, the error label calls virStorageEncryptionFree(ret), but that function blindly assumes that ret->nsecrets is valid and tries to dereference memory. You forgot: ret->format = src->format; add ATTRIBUTE_NONNULL(1), since we blindly dereference src. ACK with problems fixed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

--- src/util/virseclabel.c | 22 ++++++++++++++++++++++ src/util/virseclabel.h | 5 ++++- 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/src/util/virseclabel.c b/src/util/virseclabel.c index 5a4d78e..93c12cc 100644 --- a/src/util/virseclabel.c +++ b/src/util/virseclabel.c @@ -80,3 +80,25 @@ virSecurityDeviceLabelDefNew(const char *model) return seclabel; } + +virSecurityDeviceLabelDefPtr +virSecurityDeviceLabelDefCopy(const virSecurityDeviceLabelDef *src) +{ + virSecurityDeviceLabelDefPtr ret; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + ret->norelabel = src->norelabel; + ret->labelskip = src->labelskip; + + if (VIR_STRDUP(ret->model, src->model) < 0 || + VIR_STRDUP(ret->label, src->label) < 0) + goto error; + + return ret; + + error: + virSecurityDeviceLabelDefFree(ret); + return NULL; +} diff --git a/src/util/virseclabel.h b/src/util/virseclabel.h index 9e970a5..993fd81 100644 --- a/src/util/virseclabel.h +++ b/src/util/virseclabel.h @@ -45,7 +45,7 @@ struct _virSecurityLabelDef { }; -/* Security configuration for domain */ +/* Security configuration for device */ typedef struct _virSecurityDeviceLabelDef virSecurityDeviceLabelDef; typedef virSecurityDeviceLabelDef *virSecurityDeviceLabelDefPtr; struct _virSecurityDeviceLabelDef { @@ -61,6 +61,9 @@ virSecurityLabelDefNew(const char *model); virSecurityDeviceLabelDefPtr virSecurityDeviceLabelDefNew(const char *model); +virSecurityDeviceLabelDefPtr +virSecurityDeviceLabelDefCopy(const virSecurityDeviceLabelDef *src); + void virSecurityLabelDefFree(virSecurityLabelDefPtr def); void virSecurityDeviceLabelDefFree(virSecurityDeviceLabelDefPtr def); -- 1.9.3

It will also be reused later. --- src/util/virstoragefile.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 0792dd8..67c1471 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1444,6 +1444,17 @@ virStorageNetHostDefFree(size_t nhosts, } +static void +virStoragePermsFree(virStoragePermsPtr def) +{ + if (!def) + return; + + VIR_FREE(def->label); + VIR_FREE(def); +} + + virStorageNetHostDefPtr virStorageNetHostDefCopy(size_t nhosts, virStorageNetHostDefPtr hosts) @@ -1557,10 +1568,7 @@ virStorageSourceClear(virStorageSourcePtr def) virSecurityDeviceLabelDefFree(def->seclabels[i]); VIR_FREE(def->seclabels); } - if (def->perms) { - VIR_FREE(def->perms->label); - VIR_FREE(def->perms); - } + virStoragePermsFree(def->perms); VIR_FREE(def->timestamps); virStorageNetHostDefFree(def->nhosts, def->hosts); -- 1.9.3

Now that we have pointers to store disk source information and thus can easily exchange the structs behind we need a function to copy all the data. --- src/libvirt_private.syms | 1 + src/util/virstoragefile.c | 150 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/virstoragefile.h | 2 + 3 files changed, 153 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 122c572..08e67b2 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1889,6 +1889,7 @@ virStorageNetProtocolTypeToString; virStorageSourceAuthClear; virStorageSourceClear; virStorageSourceClearBackingStore; +virStorageSourceCopy; virStorageSourceFree; virStorageSourceGetActualType; virStorageSourceNewFromBacking; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 67c1471..8998807 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1489,6 +1489,156 @@ virStorageNetHostDefCopy(size_t nhosts, } +static virStorageTimestampsPtr +virStorageTimestampsCopy(const virStorageTimestamps *src) +{ + virStorageTimestampsPtr ret; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + memcpy(ret, src, sizeof(*src)); + + return ret; +} + + +static virStoragePermsPtr +virStoragePermsCopy(const virStoragePerms *src) +{ + virStoragePermsPtr ret; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + ret->mode = src->mode; + ret->uid = src->uid; + ret->gid = src->gid; + + if (VIR_STRDUP(ret->label, src->label)) + goto error; + + return ret; + + error: + virStoragePermsFree(ret); + return NULL; +} + + +static virStorageSourcePoolDefPtr +virStorageSourcePoolDefCopy(const virStorageSourcePoolDef *src) +{ + virStorageSourcePoolDefPtr ret; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + ret->voltype = src->voltype; + ret->pooltype = src->pooltype; + ret->actualtype = src->actualtype; + ret->mode = src->mode; + + if (VIR_STRDUP(ret->pool, src->pool) < 0 || + VIR_STRDUP(ret->volume, src->volume) < 0) + goto error; + + return ret; + + error: + virStorageSourcePoolDefFree(ret); + return NULL; +} + + +virStorageSourcePtr +virStorageSourceCopy(const virStorageSource *src, + bool backingChain) +{ + size_t i; + virStorageSourcePtr ret = NULL; + + if (VIR_ALLOC(ret) < 0) + return NULL; + + ret->type = src->type; + ret->protocol = src->protocol; + ret->format = src->format; + ret->allocation = src->allocation; + ret->capacity = src->capacity; + ret->backingRelative = src->backingRelative; + + /* storage driver metadata are not copied */ + ret->drv = NULL; + + if (VIR_STRDUP(ret->path, src->path) < 0 || + VIR_STRDUP(ret->volume, src->volume) < 0 || + VIR_STRDUP(ret->driverName, src->driverName) < 0 || + VIR_STRDUP(ret->relPath, src->relPath) < 0 || + VIR_STRDUP(ret->relDir, src->relDir) < 0 || + VIR_STRDUP(ret->backingStoreRaw, src->backingStoreRaw) < 0 || + VIR_STRDUP(ret->compat, src->compat) < 0 || + VIR_STRDUP(ret->auth.username, src->auth.username) < 0) + goto error; + + if (!(ret->hosts = virStorageNetHostDefCopy(src->nhosts, src->hosts))) + goto error; + ret->nhosts = src->nhosts; + + if (!(ret->srcpool = virStorageSourcePoolDefCopy(src->srcpool))) + goto error; + + if (!(ret->features = virBitmapNewCopy(src->features))) + goto error; + + if (!(ret->encryption = virStorageEncryptionCopy(src->encryption))) + goto error; + + if (!(ret->perms = virStoragePermsCopy(src->perms))) + goto error; + + if (!(ret->timestamps = virStorageTimestampsCopy(src->timestamps))) + goto error; + + ret->auth.secretType = src->auth.secretType; + switch ((virStorageSecretType) src->auth.secretType) { + case VIR_STORAGE_SECRET_TYPE_NONE: + case VIR_STORAGE_SECRET_TYPE_LAST: + break; + + case VIR_STORAGE_SECRET_TYPE_UUID: + memcpy(ret->auth.secret.uuid, src->auth.secret.uuid, VIR_UUID_BUFLEN); + break; + + case VIR_STORAGE_SECRET_TYPE_USAGE: + if (VIR_STRDUP(ret->auth.secret.usage, src->auth.secret.usage) < 0) + goto error; + break; + } + + if (backingChain && src->backingStore) { + if (!(ret->backingStore = virStorageSourceCopy(src->backingStore, + true))) + goto error; + } + + ret->nseclabels = src->nseclabels; + if (VIR_ALLOC_N(ret->seclabels, ret->nseclabels) < 0) + goto error; + + for (i = 0; i < ret->nseclabels; i++) { + if (!(ret->seclabels[i] = virSecurityDeviceLabelDefCopy(src->seclabels[i]))) + goto error; + } + + return ret; + + error: + virStorageSourceFree(ret); + return NULL; +} + + void virStorageSourcePoolDefFree(virStorageSourcePoolDefPtr def) { diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 34b3625..cda9e76 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -324,6 +324,8 @@ int virStorageSourceGetActualType(virStorageSourcePtr def); void virStorageSourceFree(virStorageSourcePtr def); void virStorageSourceClearBackingStore(virStorageSourcePtr def); virStorageSourcePtr virStorageSourceNewFromBacking(virStorageSourcePtr parent); +virStorageSourcePtr virStorageSourceCopy(const virStorageSource *src, + bool backingChain); #endif /* __VIR_STORAGE_FILE_H__ */ -- 1.9.3

[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