On Mon, Mar 11, 2013 at 05:13:29PM +0100, Michal Privoznik wrote:
> On filesystems supporting ACLs we don't need to do a chown but we
> can just set ACLs to gain access for qemu. However, since we are
> setting these on too low level, where we don't know if disk is
> just a read only or read write, we set read write access
> unconditionally.
>
> >From implementation POV, a reference counter is introduced, so ACL is
> restored only on the last restore attempt in order to not cut off other
> domains. And since a file may had an ACL for a user already set, we need
> to keep this as well. Both these, the reference counter and original ACL
> are stored as extended attributes named trusted.refCount and
> trusted.oldACL respectively.
> ---
>
> diff to v2:
> -basically squashed functionality of 2/4 and 4/4 from previous
> round
>
> src/security/security_dac.c | 209 ++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 182 insertions(+), 27 deletions(-)
>
> diff --git a/src/security/security_dac.c b/src/security/security_dac.c
> index 0b274b7..46cc686 100644
> --- a/src/security/security_dac.c
> +++ b/src/security/security_dac.c
> @@ -25,6 +25,7 @@
>
> #include "security_dac.h"
> #include "virerror.h"
> +#include "virfile.h"
> #include "virutil.h"
> #include "viralloc.h"
> #include "virlog.h"
> @@ -34,6 +35,8 @@
>
> #define VIR_FROM_THIS VIR_FROM_SECURITY
> #define SECURITY_DAC_NAME "dac"
> +#define SECURITY_DAC_XATTR_OLD_ACL "trusted.oldACL"
> +#define SECURITY_DAC_XATTR_REFCOUNT "trusted.refCount"
I think we want 'libvirt.dac' as a prefix for any xattrs we set
from this DAC driver.
> @@ -265,11 +406,8 @@ static const char * virSecurityDACGetDOI(virSecurityManagerPtr mgr ATTRIBUTE_UNU
> }
>
> static int
> -virSecurityDACSetOwnership(const char *path, uid_t uid, gid_t gid)
> +virSecurityDACChown(const char *path, uid_t uid, gid_t gid)
> {
> - VIR_INFO("Setting DAC user and group on '%s' to '%ld:%ld'",
> - path, (long) uid, (long) gid);
> -
> if (chown(path, uid, gid) < 0) {
> struct stat sb;
> int chown_errno = errno;
In the traditional chown() codepath, you're never preserving the original
uid/gid values in an xattr. It not unknown for admins to mount filesystems
with '-o noacl', which blocks use of ACLs, but still allows for xattrs to
be used by apps. So we can preserve uid/gid in this case
> +
> + /* Oops, something went wrong. If ACL or XATTR are not
> + * supported fall back to chown then. */
>
> - /* XXX record previous ownership */
> - rc = virSecurityDACSetOwnership(newpath, 0, 0);
> + rc = virSecurityDACChown(path, 0, 0);
Dropping this comment is bogus since you've not fixed the problem
it referred to.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list