Adds a read-only attribute allowWrite to a disk's backingStore source
element for the duration of a block commit operation, which records
that write access is needed for that backingStore.
This is used by the AppArmor security driver when re-generating profiles.
Partial-Resolves: #692
Bug-Ubuntu: https://bugs.launchpad.net/bugs/2126574
Signed-off-by: Wesley Hershberger <wesley.hershberger@canonical.com>
---
src/conf/domain_conf.c | 7 +++++++
src/conf/storage_source_conf.c | 2 ++
src/conf/storage_source_conf.h | 3 +++
src/qemu/qemu_block.c | 26 ++++++++++++++++++++++++++
src/qemu/qemu_blockjob.c | 8 ++++++++
src/qemu/qemu_security.c | 7 +++++++
src/security/virt-aa-helper.c | 7 ++-----
7 files changed, 55 insertions(+), 5 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index bfeed3dc96..02ff45626e 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -7842,6 +7842,10 @@ virDomainStorageSourceParse(xmlNodePtr node,
return -1;
}
+ if (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) &&
+ g_strcmp0(virXMLPropString(node, "allowWrite"), "yes") == 0)
+ src->secAllowWrite = true;
+
if ((tmp = virXPathNode("./auth", ctxt)) &&
!(src->auth = virStorageAuthDefParse(tmp, ctxt)))
return -1;
@@ -23753,6 +23757,9 @@ virDomainDiskSourceFormat(virBuffer *buf,
if (attrIndex && src->id != 0)
virBufferAsprintf(&attrBuf, " index='%u'", src->id);
+ if ((flags & VIR_DOMAIN_DEF_FORMAT_STATUS) && src->secAllowWrite)
+ virBufferAddLit(&attrBuf, " allowWrite='yes'");
+
if (virDomainDiskDataStoreFormat(&childBuf, src, xmlopt, flags) < 0)
return -1;
diff --git a/src/conf/storage_source_conf.c b/src/conf/storage_source_conf.c
index 087de1eaf2..4550a25500 100644
--- a/src/conf/storage_source_conf.c
+++ b/src/conf/storage_source_conf.c
@@ -911,6 +911,8 @@ virStorageSourceCopy(const virStorageSource *src,
def->nfs_uid = src->nfs_uid;
def->nfs_gid = src->nfs_gid;
+ def->secAllowWrite = src->secAllowWrite;
+
/* 'ioerror_timestamp' and 'ioerror_message' are deliberately not copied */
return g_steal_pointer(&def);
diff --git a/src/conf/storage_source_conf.h b/src/conf/storage_source_conf.h
index fc868b31af..0a54ff8c9c 100644
--- a/src/conf/storage_source_conf.h
+++ b/src/conf/storage_source_conf.h
@@ -449,6 +449,9 @@ struct _virStorageSource {
* to do this decision.
*/
bool seclabelSkipRemember;
+ /* Temporary write access to this source is required (currently only for
+ * QEMU blockcommit) */
+ bool secAllowWrite;
/* Last instance of hypervisor originated I/O error message and timestamp.
* The error stored here is not designed to be stable. The message
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index a7062d3e96..3aed5d5a7f 100644
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -3763,6 +3763,20 @@ qemuBlockCommit(virDomainObj *vm,
* operation succeeds, but doing that requires tracking the
* operation in XML across libvirtd restarts. */
clean_access = true;
+
+ /* Ensure needed perms are present in domstatus XML; this prevents races
+ * in the AppArmor secdriver */
+ baseSource->secAllowWrite = true;
+ if (baseSource->dataFileStore)
+ baseSource->dataFileStore->secAllowWrite = true;
+ if (top_parent && top_parent != disk->src) {
+ top_parent->secAllowWrite = true;
+ if (top_parent->dataFileStore)
+ top_parent->dataFileStore->secAllowWrite = true;
+ }
+
+ qemuDomainSaveStatus(vm);
+
if (qemuDomainStorageSourceAccessAllow(driver, vm, baseSource,
false, false, false) < 0)
goto cleanup;
@@ -3838,6 +3852,18 @@ qemuBlockCommit(virDomainObj *vm,
virErrorPtr orig_err;
virErrorPreserveLast(&orig_err);
+ /* Revert changes to domstatus XML */
+ baseSource->secAllowWrite = false;
+ if (baseSource->dataFileStore)
+ baseSource->dataFileStore->secAllowWrite = false;
+ if (top_parent && top_parent != disk->src) {
+ top_parent->secAllowWrite = false;
+ if (top_parent->dataFileStore)
+ top_parent->dataFileStore->secAllowWrite = false;
+ }
+
+ qemuDomainSaveStatus(vm);
+
/* Revert access to read-only, if possible. */
if (baseSource->dataFileStore) {
qemuDomainStorageSourceAccessAllow(driver, vm, baseSource->dataFileStore,
diff --git a/src/qemu/qemu_blockjob.c b/src/qemu/qemu_blockjob.c
index b54a5b3811..97a6afe7e3 100644
--- a/src/qemu/qemu_blockjob.c
+++ b/src/qemu/qemu_blockjob.c
@@ -1065,20 +1065,24 @@ qemuBlockJobProcessEventCompletedCommit(virQEMUDriver *driver,
/* revert access to images */
if (job->data.commit.base->dataFileStore) {
+ job->data.commit.base->dataFileStore->secAllowWrite = false;
qemuDomainStorageSourceAccessAllow(driver, vm, job->data.commit.base->dataFileStore,
true, false, false);
qemuBlockReopenReadOnly(vm, job->data.commit.base->dataFileStore, asyncJob);
}
+ job->data.commit.base->secAllowWrite = false;
qemuDomainStorageSourceAccessAllow(driver, vm, job->data.commit.base,
true, false, false);
if (job->data.commit.topparent != job->disk->src) {
if (job->data.commit.topparent->dataFileStore) {
+ job->data.commit.topparent->dataFileStore->secAllowWrite = false;
qemuDomainStorageSourceAccessAllow(driver, vm, job->data.commit.topparent->dataFileStore,
true, false, false);
qemuBlockReopenReadWrite(vm, job->data.commit.topparent->dataFileStore, asyncJob);
}
+ job->data.commit.topparent->secAllowWrite = false;
qemuDomainStorageSourceAccessAllow(driver, vm, job->data.commit.topparent,
true, false, true);
}
@@ -1177,6 +1181,10 @@ qemuBlockJobProcessEventCompletedActiveCommit(virQEMUDriver *driver,
baseparent->backingStore = NULL;
job->disk->src = job->data.commit.base;
job->disk->src->readonly = job->data.commit.top->readonly;
+ job->disk->src->secAllowWrite = false;
+ if (job->disk->src->dataFileStore) {
+ job->disk->src->dataFileStore->secAllowWrite = false;
+ }
if (qemuBlockJobProcessEventCompletedCommitBitmaps(vm, job, asyncJob) < 0)
return;
diff --git a/src/qemu/qemu_security.c b/src/qemu/qemu_security.c
index 6bb0f9170d..603feba87e 100644
--- a/src/qemu/qemu_security.c
+++ b/src/qemu/qemu_security.c
@@ -201,6 +201,13 @@ qemuSecurityMoveImageMetadata(virQEMUDriver *driver,
if (qemuDomainNamespaceEnabled(vm, QEMU_DOMAIN_NS_MOUNT))
pid = vm->pid;
+ if (dst) {
+ dst->secAllowWrite = src->secAllowWrite;
+ if (dst->backingStore && src->backingStore) {
+ dst->backingStore->secAllowWrite = src->backingStore->secAllowWrite;
+ }
+ }
+
return virSecurityManagerMoveImageMetadata(driver->securityManager,
cfg->sharedFilesystems,
pid, src, dst);
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
index 7920162cdc..fc2d4d9d41 100644
--- a/src/security/virt-aa-helper.c
+++ b/src/security/virt-aa-helper.c
@@ -833,11 +833,8 @@ add_file_path(virStorageSource *src,
if (!src->path || !virStorageSourceIsLocalStorage(src))
return 0;
- if (depth == 0) {
- if (src->readonly)
- ret = vah_add_file(buf, src->path, "Rk");
- else
- ret = vah_add_file(buf, src->path, "rwk");
+ if ((depth == 0 && !src->readonly) || src->secAllowWrite) {
+ ret = vah_add_file(buf, src->path, "rwk");
} else {
ret = vah_add_file(buf, src->path, "Rk");
}
--
2.51.0On Mon, Jan 05, 2026 at 14:27:23 -0600, Wesley Hershberger via Devel wrote:
> Adds a read-only attribute allowWrite to a disk's backingStore source
> element for the duration of a block commit operation, which records
> that write access is needed for that backingStore.
>
> This is used by the AppArmor security driver when re-generating profiles.
>
> Partial-Resolves: #692
> Bug-Ubuntu: https://bugs.launchpad.net/bugs/2126574
> Signed-off-by: Wesley Hershberger <wesley.hershberger@canonical.com>
> ---
> src/conf/domain_conf.c | 7 +++++++
> src/conf/storage_source_conf.c | 2 ++
> src/conf/storage_source_conf.h | 3 +++
> src/qemu/qemu_block.c | 26 ++++++++++++++++++++++++++
> src/qemu/qemu_blockjob.c | 8 ++++++++
> src/qemu/qemu_security.c | 7 +++++++
> src/security/virt-aa-helper.c | 7 ++-----
> 7 files changed, 55 insertions(+), 5 deletions(-)
[...]
> diff --git a/src/conf/storage_source_conf.h b/src/conf/storage_source_conf.h
> index fc868b31af..0a54ff8c9c 100644
> --- a/src/conf/storage_source_conf.h
> +++ b/src/conf/storage_source_conf.h
> @@ -449,6 +449,9 @@ struct _virStorageSource {
> * to do this decision.
> */
> bool seclabelSkipRemember;
> + /* Temporary write access to this source is required (currently only for
> + * QEMU blockcommit) */
> + bool secAllowWrite;
We internally track the writable state in the block job data (by knowing
which images are undergoing a block commit operation). This would just
duplicate the data and requiring us to keep it in sync just to be able
to smuggle it into the helper process.
I currently don't remember if you are getting the whole status XML, with
also the qemu driver private data, where you could do the same
inference, or just the normal XML with some status bits which are inside
the domain XML.
Either way I don't like adding another flag which is not the primary
source of information.
If the full status XML is fed to the helper process you'll need to look
in the 'blockjobs' section (see
tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml as example)
and lookup the images based on job type and nodename.
If the whole status XML is not fed to the helper thne I'd suggest to
pass another block of private data to the helper process rather than
sprinkling them into the runtime definitions and requiring to keep them
in sync, so that it's tightly coupled what's required by the helper
function and where we provide it from.
On Thu, Jan 15, 2026 at 3:07 AM Peter Krempa <pkrempa@redhat.com> wrote:
>
> On Mon, Jan 05, 2026 at 14:27:23 -0600, Wesley Hershberger via Devel wrote:
> > Adds a read-only attribute allowWrite to a disk's backingStore source
> > element for the duration of a block commit operation, which records
> > that write access is needed for that backingStore.
> >
> > This is used by the AppArmor security driver when re-generating profiles.
> >
> > Partial-Resolves: #692
> > Bug-Ubuntu: https://bugs.launchpad.net/bugs/2126574
> > Signed-off-by: Wesley Hershberger <wesley.hershberger@canonical.com>
> > ---
> > src/conf/domain_conf.c | 7 +++++++
> > src/conf/storage_source_conf.c | 2 ++
> > src/conf/storage_source_conf.h | 3 +++
> > src/qemu/qemu_block.c | 26 ++++++++++++++++++++++++++
> > src/qemu/qemu_blockjob.c | 8 ++++++++
> > src/qemu/qemu_security.c | 7 +++++++
> > src/security/virt-aa-helper.c | 7 ++-----
> > 7 files changed, 55 insertions(+), 5 deletions(-)
>
> [...]
>
> > diff --git a/src/conf/storage_source_conf.h b/src/conf/storage_source_conf.h
> > index fc868b31af..0a54ff8c9c 100644
> > --- a/src/conf/storage_source_conf.h
> > +++ b/src/conf/storage_source_conf.h
> > @@ -449,6 +449,9 @@ struct _virStorageSource {
> > * to do this decision.
> > */
> > bool seclabelSkipRemember;
> > + /* Temporary write access to this source is required (currently only for
> > + * QEMU blockcommit) */
> > + bool secAllowWrite;
>
>
> We internally track the writable state in the block job data (by knowing
> which images are undergoing a block commit operation). This would just
> duplicate the data and requiring us to keep it in sync just to be able
> to smuggle it into the helper process.
>
> I currently don't remember if you are getting the whole status XML, with
> also the qemu driver private data, where you could do the same
> inference, or just the normal XML with some status bits which are inside
> the domain XML.
>
> Either way I don't like adding another flag which is not the primary
> source of information.
>
> If the full status XML is fed to the helper process you'll need to look
> in the 'blockjobs' section (see
> tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml as example)
> and lookup the images based on job type and nodename.
>
> If the whole status XML is not fed to the helper thne I'd suggest to
> pass another block of private data to the helper process rather than
> sprinkling them into the runtime definitions and requiring to keep them
> in sync, so that it's tightly coupled what's required by the helper
> function and where we provide it from.
Thanks for the pointer on domstatus->blockjobs. As Michal noted, the
domstatus XML with the blockjob data isn't fed into the virt-aa-helper today.
That's the solution I'd prefer here, but...
- The qemu driver privateData is a field of virDomainObj
- The secdriver is only passed a virDomainDef
I'm not sure if/how it's possible to go from virDomainDef to virDomainObj.
There are the virDomainObjListFindBy* functions, but they require a reference
to the domain driver which I don't think we have in the secdriver either.
Here's a little additional context on these patches; I should have linked this
in the cover letter:
https://www.mail-archive.com/devel@lists.libvirt.org/msg13340.html
That's why I went with changes to the XML rather than trying to plumb the
info through separately.
Thanks for your help, I really appreciate it.
~Wesley
On Mon, Jan 19, 2026 at 11:38:43 -0600, Wesley Hershberger wrote: > On Thu, Jan 15, 2026 at 3:07 AM Peter Krempa <pkrempa@redhat.com> wrote: [...] > > If the full status XML is fed to the helper process you'll need to look > > in the 'blockjobs' section (see > > tests/qemustatusxml2xmldata/blockjob-blockdev-in.xml as example) > > and lookup the images based on job type and nodename. > > > > If the whole status XML is not fed to the helper thne I'd suggest to > > pass another block of private data to the helper process rather than > > sprinkling them into the runtime definitions and requiring to keep them > > in sync, so that it's tightly coupled what's required by the helper > > function and where we provide it from. > > Thanks for the pointer on domstatus->blockjobs. As Michal noted, the > domstatus XML with the blockjob data isn't fed into the virt-aa-helper today. > That's the solution I'd prefer here, but... > > - The qemu driver privateData is a field of virDomainObj > - The secdriver is only passed a virDomainDef > > I'm not sure if/how it's possible to go from virDomainDef to virDomainObj. > There are the virDomainObjListFindBy* functions, but they require a reference > to the domain driver which I don't think we have in the secdriver either. Hmm, yeah that's a pitty. I think the only viable solution would be via a callback function and a private data pointer. I wonder how we can address the disconnect between how apparmor works and how selinux/dac work. Specifically the part where qemu driver code needs to be able to reconstruct the state which the other drivers keep in seclabels. Obviously for the disk images you could create an specific XATTR labels noting the current state akin to the selinux label. > Here's a little additional context on these patches; I should have linked this > in the cover letter: > https://www.mail-archive.com/devel@lists.libvirt.org/msg13340.html > That's why I went with changes to the XML rather than trying to plumb the > info through separately. I understand that this feels simpler but this specific field serves no purpose other than duplicate state we store elsewhere which comes with extra cost to keep in sync and that should be avoided. The parser and formatter are complex-enough as they stand currently.
© 2016 - 2026 Red Hat, Inc.