[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] (v2) avoid chowning domain devices if higer-level mgmt does it for us



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]