[PATCH] qemu: Add audit entries for suspend and resume
We recently received a request from certification auditors to provide audit entries for suspend and resume. This small patch uses the existing virtDomainAudit{Start,Stop} functions with new reasons "suspended" and "resumed". Signed-off-by: Jim Fehlig <jfehlig@suse.com> --- For suspend, I initially wrote the following virDomainAuditStart(vm, virDomainPausedReasonTypeToString(reason), true); but I'm not sure it makes sense in resume, where we have reasons such as VIR_DOMAIN_CRASHED_PANICKED. For symmetry, it seemed best to go with "suspended" and "resumed". src/qemu/qemu_driver.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f1a633fdd3..c670bb681e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1682,6 +1682,7 @@ static int qemuDomainSuspend(virDomainPtr dom) goto endjob; } qemuDomainSaveStatus(vm); + virDomainAuditStart(vm, "suspended", true); ret = 0; endjob: @@ -1738,6 +1739,7 @@ static int qemuDomainResume(virDomainPtr dom) } } qemuDomainSaveStatus(vm); + virDomainAuditStop(vm, "resumed"); ret = 0; endjob: -- 2.43.0
Happy new year everyone! Any comments on this small patch? Regards, Jim On 12/16/24 16:56, Jim Fehlig wrote:
We recently received a request from certification auditors to provide audit entries for suspend and resume. This small patch uses the existing virtDomainAudit{Start,Stop} functions with new reasons "suspended" and "resumed".
Signed-off-by: Jim Fehlig <jfehlig@suse.com> ---
For suspend, I initially wrote the following
virDomainAuditStart(vm, virDomainPausedReasonTypeToString(reason), true);
but I'm not sure it makes sense in resume, where we have reasons such as VIR_DOMAIN_CRASHED_PANICKED. For symmetry, it seemed best to go with "suspended" and "resumed".
src/qemu/qemu_driver.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f1a633fdd3..c670bb681e 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -1682,6 +1682,7 @@ static int qemuDomainSuspend(virDomainPtr dom) goto endjob; } qemuDomainSaveStatus(vm); + virDomainAuditStart(vm, "suspended", true); ret = 0;
endjob: @@ -1738,6 +1739,7 @@ static int qemuDomainResume(virDomainPtr dom) } } qemuDomainSaveStatus(vm); + virDomainAuditStop(vm, "resumed"); ret = 0;
endjob:
On 12/17/24 00:56, Jim Fehlig via Devel wrote:
We recently received a request from certification auditors to provide audit entries for suspend and resume. This small patch uses the existing virtDomainAudit{Start,Stop} functions with new reasons "suspended" and "resumed".
Signed-off-by: Jim Fehlig <jfehlig@suse.com> ---
For suspend, I initially wrote the following
virDomainAuditStart(vm, virDomainPausedReasonTypeToString(reason), true);
but I'm not sure it makes sense in resume, where we have reasons such as VIR_DOMAIN_CRASHED_PANICKED. For symmetry, it seemed best to go with "suspended" and "resumed".
src/qemu/qemu_driver.c | 2 ++ 1 file changed, 2 insertions(+)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
On Tue, Jan 07, 2025 at 12:06:59PM +0100, Michal Prívozník wrote:
On 12/17/24 00:56, Jim Fehlig via Devel wrote:
We recently received a request from certification auditors to provide audit entries for suspend and resume. This small patch uses the existing virtDomainAudit{Start,Stop} functions with new reasons "suspended" and "resumed".
Signed-off-by: Jim Fehlig <jfehlig@suse.com> ---
For suspend, I initially wrote the following
virDomainAuditStart(vm, virDomainPausedReasonTypeToString(reason), true);
but I'm not sure it makes sense in resume, where we have reasons such as VIR_DOMAIN_CRASHED_PANICKED. For symmetry, it seemed best to go with "suspended" and "resumed".
src/qemu/qemu_driver.c | 2 ++ 1 file changed, 2 insertions(+)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Actually, I'm not convinced it makese sense to call virDomainAuditStart / virDomainAuditStop for these cases. Start is used when a domain is created (eg QEMU spawned) and records all the host resources that are now used. Stop is used when a domain is destroyed (eg QEMU killed) and thus indicates that host resources are no longer in use. Resume / suspend are not creating/destroying a domain, they are merely changing the CPU running state. I'm not really convinced that these operations are compelling to audit, since they're not changing what host resources are in use. Even when guest CPUs stopped, you still have incidental host CPU usage by the emulator itself, and all the other host resources remain open by the emulator. If we really do need to audit this, I'd suggest completely distinct audit events from stop/start, but personally I'd push back against this auditors request first, as it doesn't fit with the rationale for auditing IMHO. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 1/7/25 04:22, Daniel P. Berrangé wrote:
On Tue, Jan 07, 2025 at 12:06:59PM +0100, Michal Prívozník wrote:
On 12/17/24 00:56, Jim Fehlig via Devel wrote:
We recently received a request from certification auditors to provide audit entries for suspend and resume. This small patch uses the existing virtDomainAudit{Start,Stop} functions with new reasons "suspended" and "resumed".
Signed-off-by: Jim Fehlig <jfehlig@suse.com> ---
For suspend, I initially wrote the following
virDomainAuditStart(vm, virDomainPausedReasonTypeToString(reason), true);
but I'm not sure it makes sense in resume, where we have reasons such as VIR_DOMAIN_CRASHED_PANICKED. For symmetry, it seemed best to go with "suspended" and "resumed".
src/qemu/qemu_driver.c | 2 ++ 1 file changed, 2 insertions(+)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Actually, I'm not convinced it makese sense to call virDomainAuditStart / virDomainAuditStop for these cases.
Start is used when a domain is created (eg QEMU spawned) and records all the host resources that are now used.
Stop is used when a domain is destroyed (eg QEMU killed) and thus indicates that host resources are no longer in use.
Thanks for pointing that out. I was lazy and didn't look at the impl of Audit{Start,Stop} :-/. AuditStart is definitely not correct for suspend!
Resume / suspend are not creating/destroying a domain, they are merely changing the CPU running state.
I'm not really convinced that these operations are compelling to audit, since they're not changing what host resources are in use. Even when guest CPUs stopped, you still have incidental host CPU usage by the emulator itself, and all the other host resources remain open by the emulator.
If we really do need to audit this, I'd suggest completely distinct audit events from stop/start, but personally I'd push back against this auditors request first, as it doesn't fit with the rationale for auditing IMHO.
I attempted pushing back prior to writing this patch. From a non-public bug comment: "IMO, it's hard to classify suspend and resume as lifecycle operations. Suspend simply suspends execution of the VM's vcpus. All VM resources are still allocated and in use. In practice, I wonder if these operations are even used..." I've made another attempt and referenced this thread :-). Regards, Jim
participants (3)
-
Daniel P. Berrangé -
Jim Fehlig -
Michal Prívozník