On Thu, May 09, 2024 at 06:40:01 -0700, Andrea Bolognani wrote:
> On Thu, May 09, 2024 at 01:58:21PM GMT, Peter Krempa wrote:
> > On Thu, May 02, 2024 at 19:39:39 +0200, Andrea Bolognani wrote:
> > > +# libvirt will normally prevent migration if the storage backing the VM is not
> > > +# on a shared filesystems. Sometimes, however, the storage *is* shared despite
> > > +# not being detected as such: for example, this is the case when one of the
> > > +# hosts involved in the migration is exporting its local storage to the other
> > > +# one via NFS.
> >
> > I wanted to suggest using VIR_MIGRATE_UNSAFE flag to bypass this check,
> > but doing so without disabling dynamic ownership on the image itself
> > would still make the security driver cut off access to this file.
>
> This was suggested in the past, but the argument against it was that
> it's too vague: we want to skip the check on whether the storage is
> shared, and that one only, while "unsafe migration" could potentially
> cover a lot of unrelated scenarios.
>
> The other argument against using a migration flag was that it's an
> all-or-nothing proposition: either you perform the check for all
> paths, or you skip it for all paths. A configuration option allows us
> more granular control, where we can designate exactly which paths
> should not be subject to the check, retaining the default behavior
> for all the other ones.
Fair enough.
> > > +# Any directory listed here will be assumed to live on a shared filesystem,
> > > +# making migration possible in scenarios such as the one described above.
> > > +#
> > > +# Due to how label remembering is implemented, it will generally be necessary
> > > +# to set remember_owner=0 *on both sides of the migration* as well.
> >
> > So IIUC the problem here is that on the box where the storage is local
> > this will create the XATTR entries used for remembering the lablels
> > (which also includes a refcount), which given that older NFS doesn't
> > support security labelling would then be leaked?
>
> Yeah, the expectation when remembering is enabled is that both sides
> of the migration will keep the record updated.
>
> This works when XATTRs/SELinux labels are supported by the shared
> filesystem or are *not* supported and both sides access it through
> it, in which case it all ends up being no-op and works by omission :)
>
> In our scenario, migrating from local to NFS will work but then
> migrating back won't, because the existing information will lead
> libvirt to believe that the image is already in use - which it is,
> but not in the way that it expects :)
There are two distinct things in play here when migrating in scenario
that this patch is dealing with:
1) Actual security/selinux labels
- these must properly allow access to the image during the whole
time and partially also from two hosts at once
2) The additional XATTR props libvirt uses to remember security labels
- these need to be refcounted properly
In addition to that there's two directions of the migration
o) Migration out from the host that uses this feature
i) Migration in to the host using this feature
Also note that for migration in you need to consider also cases when the
VM was never started at the host using this feature yet, and is freshly
migrated in.
Now things I see as problem in case when NFS not supporting xattr is
used. This means that the remote VM can set XATTRs and must use
'virt_use_nfs' sebool.
1.o) The selinux label will be leaked/kept on the image as the target of
the migration isn't able to remove the label
1.i) A new selinux label must be applied to the image
- this is what also patch 5/5 does for TPM
- applying security label might actually prevent the NFS server from
exporting the image if configured poorly
2.o) The libvirt XATTR label will be leaked with a refcount (preventing
further local start)
2.i) This should work IIUC as the label will simply be added
I'll definitely will need to check the behaviour of selinux labels
first myself.
While just removing 2 out of existence by disabling it may seem
lucrative I think we even might need the feature if we'll need to solve
1.o).
IMO the only proper option to do this across the XATTR boundary will be
to have an additional step in the finalizing phase of migration that
will unref the libvirt labels. In case when the last reference is gone
it'd need to also restore the label, same as it does now. During
migration there'll need to be a period while two refs are on the libvirt
xattrs.
As said I'll need to actually check what's really happening in regards
of the selinux labels.
> In general, all the bookkeeping and relabeling is pretty much broken
> by design when migration is involved, and things only appear to work
> because requests are silently ignored.
As shown above the libvirt XATTRs are only a part of this.
> > Looking at the code we seem to be calling 'qemuSecurityRestoreAllLabel'
> > with the 'migrated' flag set when migrating at all times. This should
> > make it theoretically possible to simply clear out the XATTRs for any
> > file which resides on the paths in question if we're migrating away
> > rather than ignoring them, which would solve the issue.
>
> This would involve touching the file's attributes on the source side
> after the VM is already running on the destination side, which is
> something that the existing code understandably steers very clear of.
>
> I've tried fairly hard to make things work "just so" and by now I'm
> convinced that there is no solution which gets it right without
> adding a lot of additional complexity/fragility and introducing its
> own trade-offs.
>
> Asking users to disable owner remembering is not too unreasonable
> IMO. Especially when we take into account the fact that I'm targeting
> KubeVirt's needs specifically with this feature, and they *already*
> do that anyway.
Well, doing it like this makes it not very attractive for any other use.
Doing this just replaces the ugly hack in kubevirt with a rather
unattractive implementation of this in libvirt.
For anyone wanting to use this outside of Kubevirt they'd need to give
up the label remembering.
Also as noted above the libvirt xattr stuff migt be needed to actually
prevent leaking selinux labels.
> > This would go together with the 'syntax-sugar'-ish flavor to this
> > feature. Additionally it would help in case, when the user configures
> > these paths after the VM was started and thus the XATTRs are already
> > applied. Those would not then be cleared on migration.
>
> That sounds like a corner case of a corner case, and a direct result
> of the admin failing to configure the host correctly in the first
> place. I wouldn't be too concerned about it.
Sure, but proper support for using it together with security label
remembering would fix it anyways.
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
