On Fri, Apr 03, 2020 at 05:58:02PM +0200, Michal Privoznik wrote:
> This API allows drivers to separate out handling of @stdin_path
> of virSecurityManagerSetAllLabel(). The thing is, the QEMU driver
> uses transactions for virSecurityManagerSetAllLabel() which
> relabels devices from inside of domain's namespace. This is what
> we usually want. Except when resuming domain from a file. The
> file is opened before any namespace is set up and the FD is
> passed to QEMU to read the migration stream from. Because of
> this, the file lives outside of the namespace and if it so
> happens that the file is a block device (i.e. it lives under
> /dev) its copy will be created in the namespace. But the FD that
> is passed to QEMU points to the original living in the host and
> not in the namespace. So relabeling the file inside the namespace
> helps nothing.
>
> But if we have a separate API for relabeling the restore file
> then the QEMU driver can continue calling
> virSecurityManagerSetAllLabel() with transactions enabled and
> call this new API without transactions.
>
> We already have an API for relabeling a single file
> (virSecurityManagerDomainSetPathLabel()) but in case of SELinux
> it uses @imagelabel (which allows RW access) and we want to use
> @content_context (which allows RO access).
Since this patch is only adding a generic driver API rather than a specific
security backend API, I'd move ^this last paragraph to the next patch, I
actually appreciated the info there much more than here during the review.
>
> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> ---
> src/libvirt_private.syms | 1 +
> src/security/security_driver.h | 4 ++++
> src/security/security_manager.c | 29 +++++++++++++++++++++++++++++
> src/security/security_manager.h | 4 ++++
> src/security/security_stack.c | 21 +++++++++++++++++++++
> 5 files changed, 59 insertions(+)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index e276f55bb1..2c63f37fc2 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1523,6 +1523,7 @@ virSecurityDriverLookup;
> # security/security_manager.h
> virSecurityManagerCheckAllLabel;
> virSecurityManagerClearSocketLabel;
> +virSecurityManagerDomainSetIncomingPathLabel;
IncomingPath sounds a bit confusing to me, I'd go with something like HostPath
instead, in patch 3/3 you also have an explicit commentary why we're using the
respective backend API for this rather than simply using SetPathLabel.
> virSecurityManagerDomainSetPathLabel;
> virSecurityManagerGenLabel;
> virSecurityManagerGetBaseLabel;
> diff --git a/src/security/security_driver.h b/src/security/security_driver.h
> index 3353955813..6cbe8613c9 100644
> --- a/src/security/security_driver.h
> +++ b/src/security/security_driver.h
> @@ -140,6 +140,9 @@ typedef int (*virSecurityDomainSetPathLabel) (virSecurityManagerPtr mgr,
> virDomainDefPtr def,
> const char *path,
> bool allowSubtree);
> +typedef int (*virSecurityDomainSetIncomingPathLabel) (virSecurityManagerPtr mgr,
> + virDomainDefPtr def,
> + const char *path);
> typedef int (*virSecurityDomainSetChardevLabel) (virSecurityManagerPtr mgr,
> virDomainDefPtr def,
> virDomainChrSourceDefPtr dev_source,
> @@ -211,6 +214,7 @@ struct _virSecurityDriver {
> virSecurityDriverGetBaseLabel getBaseLabel;
>
> virSecurityDomainSetPathLabel domainSetPathLabel;
> + virSecurityDomainSetIncomingPathLabel domainSetIncomingPathLabel;
>
> virSecurityDomainSetChardevLabel domainSetSecurityChardevLabel;
> virSecurityDomainRestoreChardevLabel domainRestoreSecurityChardevLabel;
> diff --git a/src/security/security_manager.c b/src/security/security_manager.c
> index fe032746ff..a76b953ee4 100644
> --- a/src/security/security_manager.c
> +++ b/src/security/security_manager.c
> @@ -1077,6 +1077,35 @@ virSecurityManagerDomainSetPathLabel(virSecurityManagerPtr mgr,
> }
>
>
> +/**
> + * virSecurityManagerDomainSetIncomingPathLabel:
> + * @mgr: security manager object
> + * @vm: domain definition object
> + * @path: path to label
> + *
> + * This function relabels given @path so that @vm can restore for
maybe add "host" @path here to make it even clearer.
> + * it. This allows the driver backend to use different label than
> + * virSecurityManagerDomainSetPathLabel().
> + *
> + * Returns: 0 on success, -1 on error.
> + */
> +int
> +virSecurityManagerDomainSetIncomingPathLabel(virSecurityManagerPtr mgr,
> + virDomainDefPtr vm,
> + const char *path)
> +{
> + if (mgr->drv->domainSetIncomingPathLabel) {
> + int ret;
> + virObjectLock(mgr);
> + ret = mgr->drv->domainSetIncomingPathLabel(mgr, vm, path);
> + virObjectUnlock(mgr);
> + return ret;
> + }
> +
> + return 0;
> +}
Looks good and works:
Reviewed-by: Erik Skultety <eskultet@xxxxxxxxxx>
