[libvirt] [PATCH] avoid chowning domain devices if higer-level mgmt does it for us
this is particularily important if said device is a file sitting on a root_squashing nfs export. --- src/qemu/qemu.conf | 4 ++++ src/qemu/qemu_conf.c | 3 +++ src/qemu/qemu_conf.h | 1 + src/qemu/qemu_driver.c | 2 +- 4 files changed, 9 insertions(+), 1 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; 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..4c5de80 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 */ -- 1.6.5.2
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; 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"), -- 1.6.5.2
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. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
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. Dan
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 :|
On Wed, Nov 25, 2009 at 11:49:32AM +0000, Daniel P. Berrange wrote:
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.
Dan, can you add appropriate BZs for this (or add this plan to the existing BZ)? I don't want to lose track of this, it is critical for rhel 6. --Hugh
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- ======================================================== Hugh Brock, hbrock@redhat.com, +1-215-564-3232 Deltacloud API + Portal http://deltacloud.org Libvirt virtualization library http://libvirt.org ======================================================== Register now for Red Hat Virtual Experience, December 9. Enterprise Linux, virtualization, cloud, and more. http://www.redhat.com/virtualexperience
Only now did I notice another problem with root_squasing nfs, unrelated to chown and unadressed by my patch: qemudDomainSave tries to create the target image file and write into it as root. How reasonable would it be to seteuid() to qemu user before doing so?
On Sun, Nov 29, 2009 at 03:54:30PM +0200, Dan Kenigsberg wrote:
Only now did I notice another problem with root_squasing nfs, unrelated to chown and unadressed by my patch: qemudDomainSave tries to create the target image file and write into it as root. How reasonable would it be to seteuid() to qemu user before doing so?
The trouble with directly doing seteuid() is that it affects all threads in the process, not just the thread doing the save() operation. We would thus need to fork a tiny helper program before doing the seteuid() step to create the file. 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 :|
participants (4)
-
Dan Kenigsberg -
Daniel P. Berrange -
Daniel Veillard -
Hugh O. Brock