Re: [libvirt] [PATCH 3/6] security driver: Introduce transaction APIs
On 01/07/2017 03:04 PM, John Ferlan wrote:
On 12/19/2016 10:57 AM, Michal Privoznik wrote:
> With our new qemu namespace code in place, the relabelling of
> devices is done not as good is it could: a child process is
> spawned, it enters the mount namespace of the qemu process and
> then runs desired API of the security driver.
Extra CR/LF between paragraphs
> Problem with this approach is that internal state transition of
> the security driver done in the child process is not reflected in
> the parent process. While currently it wouldn't matter that much,
> it is fairly easy to forge about that. We should take the extra
s/forge/forget
> step now while this limitation is still freshly in our minds.
s/freshly/fresh
>
> Three new APIs are introduced here:
> virSecurityManagerTransactionStart()
> virSecurityManagerTransactionCommit()
> virSecurityManagerTransactionAbort()
>
> The Start() is going to be used to let security driver know that
> we are starting a new transaction. During a transaction no
> security labels are actually touched, but rather recorded and
> only at Commit() phase they are actually updated. Should
> something go wrong Abort() aborts the transaction freeing up all
> memory allocated by transaction.
So what happens if one is halfway through a commit? No chance to
rollback the already committed and we drop the yet to be committed.
Ah, you mean when an error occurs during Commit()? Well, nothing special
- we don't do any Rollback() now either. The caller calls secdriver's
Restore() API eventually. Also, what would happen if an error occurs
during Rollback()?
I'm not saying we shouldn't have it some day, but I don't see much need
for it today. This is no worse than what we currently have.
I wonder if there should be a Rollback() API as well... That would mean
Commit would need to provide more context upon return though.
>
> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
> ---
> src/libvirt_private.syms | 3 +++
> src/security/security_driver.h | 9 ++++++++
> src/security/security_manager.c | 38 ++++++++++++++++++++++++++++++++
> src/security/security_manager.h | 5 +++++
> src/security/security_stack.c | 49 +++++++++++++++++++++++++++++++++++++++++
I know qemu doesn't used it, but would _apparmor also benefit from
similar functionality?
Not sure. Frankly, apparmor is terra incognita for me. But my gut
feeling is that whenever there's a problem in a secdriver we fix DAC and
SELinux but very occasionally apparmor. It just works :-)
What's here is OK, but concerns raised in patch 4 make me pause on ACK.
Fair enough. Let me check.
Michal
John