[libvirt] [PATCH v2 0/5] Don't run whole sec driver in namespace

[John Ferlan]

On 01/09/2017 07:58 AM, Michal Privoznik wrote:
> v2 of:
> 
> https://www.redhat.com/archives/libvir-list/2016-December/msg00907.html
> 
> diff to v1:
> - Pushed 1/6 from the original series as it was ACKed
> - Added more comments
> - Fixed return value of virSecurityDACTransactionAppend and virSecuritySELinuxTransactionAppend
> - Dropped ignore_value() around virThreadLocalSet()
> - Unset thread local just in transactionCommit APIs
> 
> I have not implemented any rollback yet, but I've added comments
> where the implementation should go ;-)
> 
> Michal Privoznik (5):
>   security_dac: Resolve virSecurityDACSetOwnershipInternal const
>     correctness
>   security driver: Introduce transaction APIs
>   security_dac: Implement transaction APIs
>   security_selinux: Implement transaction APIs
>   qemu: Use transactions from security driver
> 
>  src/libvirt_private.syms              |   3 +
>  src/qemu/qemu_driver.c                |  25 ++--
>  src/qemu/qemu_security.c              | 100 ++++---------
>  src/security/security_dac.c           | 261 +++++++++++++++++++++++++++++++++-
>  src/security/security_driver.h        |   9 ++
>  src/security/security_manager.c       |  68 +++++++++
>  src/security/security_manager.h       |   7 +-
>  src/security/security_selinux.c       | 256 ++++++++++++++++++++++++++++++++-
>  src/security/security_stack.c         |  49 +++++++
>  src/storage/storage_backend.h         |   2 +-
>  src/storage/storage_backend_fs.c      |   2 +-
>  src/storage/storage_backend_gluster.c |   2 +-
>  src/storage/storage_driver.c          |   6 +-
>  src/storage/storage_driver.h          |   4 +-
>  src/util/virstoragefile.c             |   2 +-
>  src/util/virstoragefile.h             |   2 +-
>  16 files changed, 703 insertions(+), 95 deletions(-)
> 

BTW: Perhaps an existing bug...  If *Launch calls SetAllLabel and fails
a -1 is returned...  Once SetAllLabel succeeds, all returns afterwards
are -2. When the caller detects a -2 return, then it forces the relabel
in the exit path.

So if we have a partial success/failure, then do we really restore
anything?  Should the setting of ret = -2 go before the call to
qemuSecuritySetAllLabel?  Not related to this set of changes, but since
I was looking at the code...

I guess for now not having Rollback will be no worse than what exists.
It would seem to matter more on Set processing and much harder on
Restore processing (other than ignoring errors and trying to restore as
many as possible).

I think the Rollback thoughts came mostly because I started thinking
about other transaction applications which don't process "All", but
rather process "Some". Not rolling back on Abort or forcing a caller to
Rollback may be unexpected.

Consider that the virSecurity*TransactionRun could "save" the status of
the call to each list entry and then if there is a failure, go through
each successful one, recall the saved data label data, and restore it
(ok conceptually at least). Taking this path means not worrying about
the partial failure path I noted above. If ignoring failures on Restore
is important, then Start could save in list whether this was a "Set" or
"Restore" operation...

In any case, I'm not going to hold things up for a Rollback... As long
as the notes from patch 1 and 5 are addressed, then ACK series.

John