On Wed, Nov 25, 2009 at 09:27:13AM +0200, Dan Kenigsberg wrote: > this is particularily important if said device is a file sitting on a > root_squashing nfs export. > > my previous attempt for a patch missed 3 chowns that should be avoided. > --- > src/qemu/qemu.conf | 4 ++++ > src/qemu/qemu_conf.c | 3 +++ > src/qemu/qemu_conf.h | 1 + > src/qemu/qemu_driver.c | 8 ++++---- > 4 files changed, 12 insertions(+), 4 deletions(-) > > diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf > index bca858a..892a50b 100644 > --- a/src/qemu/qemu.conf > +++ b/src/qemu/qemu.conf > @@ -96,6 +96,10 @@ > # The group ID for QEMU processes run by the system instance > #group = "root" > > +# should libvirt assume that devices are accessible to the above user:group. > +# by default, libvirt tries to chown devices before starting up a domain and > +# restore ownership to root when domain comes down. > +#assume_devices_accessible = 0 > > # What cgroup controllers to make use of with QEMU guests > # > diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c > index b1b9e5f..520a395 100644 > --- a/src/qemu/qemu_conf.c > +++ b/src/qemu/qemu_conf.c > @@ -232,6 +232,9 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, > return -1; > } > > + p = virConfGetValue (conf, "assume_devices_accessible"); > + CHECK_TYPE ("assume_devices_accessible", VIR_CONF_LONG); > + if (p) driver->avoid_dev_chown = p->l; an explicit initialization of the field would be better if p is NULL. > if (virGetGroupID(NULL, group, &driver->group) < 0) { > VIR_FREE(group); > diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h > index 675c636..3a9da73 100644 > --- a/src/qemu/qemu_conf.h > +++ b/src/qemu/qemu_conf.h > @@ -87,6 +87,7 @@ struct qemud_driver { > > uid_t user; > gid_t group; > + int avoid_dev_chown; > > unsigned int qemuVersion; > int nextvmid; > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > index 2f273eb..5f02aa2 100644 > --- a/src/qemu/qemu_driver.c > +++ b/src/qemu/qemu_driver.c > @@ -1968,7 +1968,7 @@ static int qemuDomainSetDeviceOwnership(virConnectPtr conn, > uid_t uid; > gid_t gid; > > - if (!driver->privileged) > + if (!driver->privileged || driver->avoid_dev_chown) > return 0; > > /* short circuit case of root:root */ > @@ -2002,7 +2002,7 @@ static int qemuDomainSetAllDeviceOwnership(virConnectPtr conn, > uid_t uid; > gid_t gid; > > - if (!driver->privileged) > + if (!driver->privileged || driver->avoid_dev_chown) > return 0; > > /* short circuit case of root:root */ > @@ -3438,7 +3438,7 @@ static int qemudDomainSave(virDomainPtr dom, > } > fd = -1; > > - if (driver->privileged && > + if (driver->privileged && !driver->avoid_dev_chown && > chown(path, driver->user, driver->group) < 0) { > virReportSystemError(NULL, errno, > _("unable to set ownership of '%s' to user %d:%d"), > @@ -3473,7 +3473,7 @@ static int qemudDomainSave(virDomainPtr dom, > if (rc < 0) > goto endjob; > > - if (driver->privileged && > + if (driver->privileged && !driver->avoid_dev_chown && > chown(path, 0, 0) < 0) { > virReportSystemError(NULL, errno, > _("unable to set ownership of '%s' to user %d:%d"), The core question is having this as another manual user tweak, it always makes me a bit uncomfortable if proper working of software requires manually editing a config file. If we really need this kind of options for proper operations in specific conditions can we make sure it can be set via APIs too ? I don't think we should expose something as generic as the internal Conf APIs, but these runtime option in src/qemu/qemu.conf start to accumulate and I start to wonder how to properly manage all this.

On Wed, Nov 25, 2009 at 01:38:52PM +0200, Dan Kenigsberg wrote: > On Wed, Nov 25, 2009 at 10:31:03AM +0100, Daniel Veillard wrote: > > On Wed, Nov 25, 2009 at 09:27:13AM +0200, Dan Kenigsberg wrote: > > > this is particularily important if said device is a file sitting on a > > > root_squashing nfs export. > > > > > > my previous attempt for a patch missed 3 chowns that should be avoided. > > > --- > > > src/qemu/qemu.conf | 4 ++++ > > > src/qemu/qemu_conf.c | 3 +++ > > > src/qemu/qemu_conf.h | 1 + > > > src/qemu/qemu_driver.c | 8 ++++---- > > > 4 files changed, 12 insertions(+), 4 deletions(-) > > > > > > diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf > > > index bca858a..892a50b 100644 > > > --- a/src/qemu/qemu.conf > > > +++ b/src/qemu/qemu.conf > > > @@ -96,6 +96,10 @@ > > > # The group ID for QEMU processes run by the system instance > > > #group = "root" > > > > > > +# should libvirt assume that devices are accessible to the above user:group. > > > +# by default, libvirt tries to chown devices before starting up a domain and > > > +# restore ownership to root when domain comes down. > > > +#assume_devices_accessible = 0 > > > > > > # What cgroup controllers to make use of with QEMU guests > > > # > > > diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c > > > index b1b9e5f..520a395 100644 > > > --- a/src/qemu/qemu_conf.c > > > +++ b/src/qemu/qemu_conf.c > > > @@ -232,6 +232,9 @@ int qemudLoadDriverConfig(struct qemud_driver *driver, > > > return -1; > > > } > > > > > > + p = virConfGetValue (conf, "assume_devices_accessible"); > > > + CHECK_TYPE ("assume_devices_accessible", VIR_CONF_LONG); > > > + if (p) driver->avoid_dev_chown = p->l; > > > > an explicit initialization of the field would be better if p is NULL. > > > > > if (virGetGroupID(NULL, group, &driver->group) < 0) { > > > VIR_FREE(group); > > > diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h > > > index 675c636..3a9da73 100644 > > > --- a/src/qemu/qemu_conf.h > > > +++ b/src/qemu/qemu_conf.h > > > @@ -87,6 +87,7 @@ struct qemud_driver { > > > > > > uid_t user; > > > gid_t group; > > > + int avoid_dev_chown; > > > > > > unsigned int qemuVersion; > > > int nextvmid; > > > diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c > > > index 2f273eb..5f02aa2 100644 > > > --- a/src/qemu/qemu_driver.c > > > +++ b/src/qemu/qemu_driver.c > > > @@ -1968,7 +1968,7 @@ static int qemuDomainSetDeviceOwnership(virConnectPtr conn, > > > uid_t uid; > > > gid_t gid; > > > > > > - if (!driver->privileged) > > > + if (!driver->privileged || driver->avoid_dev_chown) > > > return 0; > > > > > > /* short circuit case of root:root */ > > > @@ -2002,7 +2002,7 @@ static int qemuDomainSetAllDeviceOwnership(virConnectPtr conn, > > > uid_t uid; > > > gid_t gid; > > > > > > - if (!driver->privileged) > > > + if (!driver->privileged || driver->avoid_dev_chown) > > > return 0; > > > > > > /* short circuit case of root:root */ > > > @@ -3438,7 +3438,7 @@ static int qemudDomainSave(virDomainPtr dom, > > > } > > > fd = -1; > > > > > > - if (driver->privileged && > > > + if (driver->privileged && !driver->avoid_dev_chown && > > > chown(path, driver->user, driver->group) < 0) { > > > virReportSystemError(NULL, errno, > > > _("unable to set ownership of '%s' to user %d:%d"), > > > @@ -3473,7 +3473,7 @@ static int qemudDomainSave(virDomainPtr dom, > > > if (rc < 0) > > > goto endjob; > > > > > > - if (driver->privileged && > > > + if (driver->privileged && !driver->avoid_dev_chown && > > > chown(path, 0, 0) < 0) { > > > virReportSystemError(NULL, errno, > > > _("unable to set ownership of '%s' to user %d:%d"), > > > > The core question is having this as another manual user tweak, it always > > makes me a bit uncomfortable if proper working of software requires manually > > editing a config file. If we really need this kind of options for proper > > operations in specific conditions can we make sure it can be set via > > APIs too ? I don't think we should expose something as generic as the > > internal Conf APIs, but these runtime option in src/qemu/qemu.conf > > start to accumulate and I start to wonder how to properly manage all > > this. > > Aren't we here to babysit software? :-) > > For this particular behavior, I personally feel that the default is > wrong. I don't like it that libvirt takes the liberty to (try to) change > ownership, I like it less when it does not truely restore ownership, and > even less, that it fails to use files over nfs. > > I understand that this behavior was meant to assist admins who upgrade > from running qemu as root to running it as a meager user. But personally > I believe that we could have politely asked them: if you use this new > feature, please make sure this new qemu user has access to your > files/devices. Now that there are people expecting current behavior, I > don't really see a nice way out. Yes this whole area of code didn't really turn out the way I hoped it would. For a start, regardless of whether it works correctly or not, we definitely need to be able to turn this chown'ing on/off, so making it configurable is good. I think I'd like todo it in a different way though. I regret putting the chown() calls directly into the QEMU driver. What I should have done was implement a new instance of the security driver framework todo the chowning. And then stack that with the SELinux security driver. This would have kept the QEMU code cleaner Second, we need to implement some kind of persistence mechanism that security drivers can use to save the 'before' state. We already need this for the SELinux security driver so it can record the original disk labels, in the same way we need it for ownership. Third, we need to ignore failure to chown & carry on anyway - if it is truely fatal,then QEMU itself will show an error, we don't need to preempt that particularly if we can get it wrong as on NFS. Fouth, we need to enhance the filesystem storage driver, so that it honours the permissions info in the volume XML when creating files based volumes, so that files get the correct ownership immediately. This is the only way to make things work on a root-squashing NFS server.

Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :| -- Libvir-list mailing list Libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list