[PATCH 0/4 v2] tools/hotplug: systemd changes for 4.5

[PATCH 0/4 v2] tools/hotplug: systemd changes for 4.5

[Olaf Hering]

This is a resend of this series, with just the low hanging fruits:
http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg00669.html

The mentioned wrapper to run xenstored from systemd without duplicate
functionality found in the sysv runlevel script will be send in another patch,
once it is ready.

Olaf

Olaf Hering (4):
  tools/hotplug: remove SELinux options from var-lib-xenstored.mount
  tools/hotplug: remove XENSTORED_ROOTDIR from service file
  tools/hotplug: remove EnvironmentFile from
    xen-qemu-dom0-disk-backend.service
  tools/hotplug: use xencommons as EnvironmentFile

 tools/hotplug/Linux/systemd/var-lib-xenstored.mount.in            | 4 +---
 tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in | 1 -
 tools/hotplug/Linux/systemd/xenconsoled.service.in                | 2 +-
 tools/hotplug/Linux/systemd/xenstored.service.in                  | 1 -
 4 files changed, 2 insertions(+), 6 deletions(-)

[PATCH 1/4] tools/hotplug: remove SELinux options from var-lib-xenstored.mount

[Olaf Hering]

Using SELinux mount options per default breaks several systems. Either
the context= mount option is not known at all to the kernel, or the
default value "none" is unknown to SELinux. In both cases the unit will
fail.

The proper place to specify mount options is /etc/fstab. Appearently
systemd is kind enough to use values from there even if Options= or
What= is specified in a .mount file.

Remove XENSTORED_MOUNT_CTX, the reference to a non-existant
EnvironmentFile and trim default Options= for the mount point.

The removed code was first mentioned in the patch referenced below, with
the following description:
...
 * Some systems define the selinux context in the systemd Option for the
   /var/lib/xenstored tmpfs:
       Options=mode=755,context="system_u:object_r:xenstored_var_lib_t:s0"
   For the upstream version we remove that and let systems specify the context
   on their system /etc/default/xenstored or /etc/sysconfig/xenstored
   $XENSTORED_MOUNT_CTX variable
...
It is nowhere stated (on xen-devel) what "Some systems" means, which is
unfortunately common practice in nearly all opensource projects.
http://lists.xenproject.org/archives/html/xen-devel/2014-03/msg02462.html

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: M A Young <m.a.young@durham.ac.uk>
Cc: Luis R. Rodriguez <mcgrof@do-not-panic.com>
---
 tools/hotplug/Linux/systemd/var-lib-xenstored.mount.in | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tools/hotplug/Linux/systemd/var-lib-xenstored.mount.in b/tools/hotplug/Linux/systemd/var-lib-xenstored.mount.in
index d5e04db..11a7d50 100644
--- a/tools/hotplug/Linux/systemd/var-lib-xenstored.mount.in
+++ b/tools/hotplug/Linux/systemd/var-lib-xenstored.mount.in
@@ -6,9 +6,7 @@ ConditionPathExists=/proc/xen/capabilities
 RefuseManualStop=true
 
 [Mount]
-Environment=XENSTORED_MOUNT_CTX=none
-EnvironmentFile=-@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xenstored
 What=xenstore
 Where=@XEN_LIB_STORED@
 Type=tmpfs
-Options=mode=755,context="$XENSTORED_MOUNT_CTX"
+Options=mode=755

[PATCH 2/4] tools/hotplug: remove XENSTORED_ROOTDIR from service file

[Olaf Hering]

There is no need to export XENSTORED_ROOTDIR. This variable can be
enabled in sysconfig/xencommons. If the variable is unset xenstored
will automatically use @XEN_LIB_STORED@.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/hotplug/Linux/systemd/xenstored.service.in | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/hotplug/Linux/systemd/xenstored.service.in b/tools/hotplug/Linux/systemd/xenstored.service.in
index 780bdd6..0f0ac58 100644
--- a/tools/hotplug/Linux/systemd/xenstored.service.in
+++ b/tools/hotplug/Linux/systemd/xenstored.service.in
@@ -9,7 +9,6 @@ ConditionPathExists=/proc/xen/capabilities
 [Service]
 Type=notify
 Environment=XENSTORED_ARGS=
-Environment=XENSTORED_ROOTDIR=@XEN_LIB_STORED@
 Environment=XENSTORED=@XENSTORED@
 EnvironmentFile=-@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons
 ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities

[PATCH 3/4] tools/hotplug: remove EnvironmentFile from xen-qemu-dom0-disk-backend.service

[Olaf Hering]

The references Environment file does not exist, and the service file
does not make use of variables anyway.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in b/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in
index 0a5807a..274cec0 100644
--- a/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in
+++ b/tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in
@@ -8,7 +8,6 @@ ConditionPathExists=/proc/xen/capabilities
 
 [Service]
 Type=simple
-EnvironmentFile=-@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xenstored
 PIDFile=@XEN_RUN_DIR@/qemu-dom0.pid
 ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities
 ExecStartPre=/bin/mkdir -p @XEN_RUN_DIR@

[PATCH 4/4] tools/hotplug: use xencommons as EnvironmentFile

[Olaf Hering]

The referenced sysconfig/xenconsoled does not exist. If anything needs to be
specified it has to go into the existing sysconfig/xencommons file.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/hotplug/Linux/systemd/xenconsoled.service.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/hotplug/Linux/systemd/xenconsoled.service.in b/tools/hotplug/Linux/systemd/xenconsoled.service.in
index cb44cd6..74d0428 100644
--- a/tools/hotplug/Linux/systemd/xenconsoled.service.in
+++ b/tools/hotplug/Linux/systemd/xenconsoled.service.in
@@ -9,7 +9,7 @@ Type=simple
 Environment=XENCONSOLED_ARGS=
 Environment=XENCONSOLED_LOG=none
 Environment=XENCONSOLED_LOG_DIR=@XEN_LOG_DIR@/console
-EnvironmentFile=-@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xenconsoled
+EnvironmentFile=-@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons
 PIDFile=@XEN_RUN_DIR@/xenconsoled.pid
 ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities
 ExecStartPre=/bin/mkdir -p ${XENCONSOLED_LOG_DIR}

Re: [PATCH 0/4 v2] tools/hotplug: systemd changes for 4.5

[Konrad Rzeszutek Wilk]

On Mon, Dec 08, 2014 at 11:18:05AM +0100, Olaf Hering wrote:
> This is a resend of this series, with just the low hanging fruits:
> http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg00669.html
> 

This looks like it would fix some of the issues I saw. I will test it
over today.

Please also CC Michael (Fedora Xen maintainer) on these changes (I've CC-ed
him here).

 The mentioned wrapper to run xenstored from systemd without duplicate
> functionality found in the sysv runlevel script will be send in another patch,
> once it is ready.
> 
> Olaf
> 
> Olaf Hering (4):
>   tools/hotplug: remove SELinux options from var-lib-xenstored.mount
>   tools/hotplug: remove XENSTORED_ROOTDIR from service file
>   tools/hotplug: remove EnvironmentFile from
>     xen-qemu-dom0-disk-backend.service
>   tools/hotplug: use xencommons as EnvironmentFile
> 
>  tools/hotplug/Linux/systemd/var-lib-xenstored.mount.in            | 4 +---
>  tools/hotplug/Linux/systemd/xen-qemu-dom0-disk-backend.service.in | 1 -
>  tools/hotplug/Linux/systemd/xenconsoled.service.in                | 2 +-
>  tools/hotplug/Linux/systemd/xenstored.service.in                  | 1 -
>  4 files changed, 2 insertions(+), 6 deletions(-)
> 

Re: [PATCH 0/4 v2] tools/hotplug: systemd changes for 4.5

[Olaf Hering]

On Wed, Dec 10, Konrad Rzeszutek Wilk wrote:

> On Mon, Dec 08, 2014 at 11:18:05AM +0100, Olaf Hering wrote:
> > This is a resend of this series, with just the low hanging fruits:
> > http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg00669.html
> This looks like it would fix some of the issues I saw. I will test it
> over today.
> Please also CC Michael (Fedora Xen maintainer) on these changes (I've CC-ed
> him here).

It would be nice to know if the entire chain of dependencies fails, or
just that unit. Furthermore it would be nice to know if there needs to
be anyhing related to SELinux in the xen sources. In other words, would
xenstored behave correctly if that tmpfs mount would be done without any
options?

Olaf

Re: [PATCH 0/4 v2] tools/hotplug: systemd changes for 4.5

[M A Young]

On Thu, 11 Dec 2014, Olaf Hering wrote:

> On Wed, Dec 10, Konrad Rzeszutek Wilk wrote:
>
>> On Mon, Dec 08, 2014 at 11:18:05AM +0100, Olaf Hering wrote:
>>> This is a resend of this series, with just the low hanging fruits:
>>> http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg00669.html
>> This looks like it would fix some of the issues I saw. I will test it
>> over today.
>> Please also CC Michael (Fedora Xen maintainer) on these changes (I've CC-ed
>> him here).
>
> It would be nice to know if the entire chain of dependencies fails, or
> just that unit. Furthermore it would be nice to know if there needs to
> be anyhing related to SELinux in the xen sources. In other words, would
> xenstored behave correctly if that tmpfs mount would be done without any
> options?

Yes, you do need to set explicit selinux permissions when mounting 
/var/lib/xenstored as otherwise it gets a tmpfs selinux context which 
xenstored can't use in enforcing mode.

The other selinux issue is that it seems you can't run xenstored through a 
shell script wrapper, because it still has startup shell script selinux 
permissions when it is trying to connect to the sockets, so it doesn't 
work. It does work if you run xenstored directly from the systemd file.

 	Michael Young

Re: [PATCH 0/4 v2] tools/hotplug: systemd changes for 4.5

[Olaf Hering]

On Thu, Dec 11, M A Young wrote:

> Yes, you do need to set explicit selinux permissions when mounting
> /var/lib/xenstored as otherwise it gets a tmpfs selinux context which
> xenstored can't use in enforcing mode.

Is that "enforcing mode" the default? And would it be too cumbersome to
have these context settings in fstab?

> The other selinux issue is that it seems you can't run xenstored through a
> shell script wrapper, because it still has startup shell script selinux
> permissions when it is trying to connect to the sockets, so it doesn't work.
> It does work if you run xenstored directly from the systemd file.

This sounds like xenstored has to parse the possible environment
variables found in sysconfig.xencommons all by itself? Is there perhaps
a way out of the SELinux jail?

Olaf

Re: [PATCH 0/4 v2] tools/hotplug: systemd changes for 4.5

[Olaf Hering]

On Thu, Dec 11, Olaf Hering wrote:

> This sounds like xenstored has to parse the possible environment
> variables found in sysconfig.xencommons all by itself? Is there perhaps
> a way out of the SELinux jail?

Does all that work with the sysv runlevel scripts?

Olaf

Re: [PATCH 0/4 v2] tools/hotplug: systemd changes for 4.5

[M A Young]

On Thu, 11 Dec 2014, Olaf Hering wrote:

> On Thu, Dec 11, Olaf Hering wrote:
>
>> This sounds like xenstored has to parse the possible environment
>> variables found in sysconfig.xencommons all by itself? Is there perhaps
>> a way out of the SELinux jail?
>
> Does all that work with the sysv runlevel scripts?

I assume so, but Fedora hasn't used the upstream sysv scripts for a long 
time (from before I started maintaining it) so I don't know for sure.

 	Michael Young

Re: [PATCH 0/4 v2] tools/hotplug: systemd changes for 4.5

[Konrad Rzeszutek Wilk]

On Thu, Dec 11, 2014 at 01:04:24PM +0100, Olaf Hering wrote:
> On Thu, Dec 11, M A Young wrote:
> 
> > Yes, you do need to set explicit selinux permissions when mounting
> > /var/lib/xenstored as otherwise it gets a tmpfs selinux context which
> > xenstored can't use in enforcing mode.
> 
> Is that "enforcing mode" the default? And would it be too cumbersome to

Yes.
> have these context settings in fstab?

That would be a question for the SELinux maintainer..
> 
> > The other selinux issue is that it seems you can't run xenstored through a
> > shell script wrapper, because it still has startup shell script selinux
> > permissions when it is trying to connect to the sockets, so it doesn't work.
> > It does work if you run xenstored directly from the systemd file.
> 
> This sounds like xenstored has to parse the possible environment
> variables found in sysconfig.xencommons all by itself? Is there perhaps
> a way out of the SELinux jail?

We do want to be in the SELinux jail as you call it.

This is what it looks to be doing:

[konrad@laptop SOURCES]$ more var-lib-xenstored.mount 
[Unit]
Description=mount xenstore file system
ConditionPathExists=/proc/xen
RefuseManualStop=true

[Mount]
What=xenstore
Where=/var/lib/xenstored
Type=tmpfs
Options=mode=755,context="system_u:object_r:xenstored_var_lib_t:s0"
[konrad@laptop SOURCES]$ 

I wonder if we can detect the context during build-time (an autoconf function
that checks whether the build is done for Fedora?)

But what if the version of Fedora is different and the object is called
something else?
> 
> Olaf

Re: [PATCH 0/4 v2] tools/hotplug: systemd changes for 4.5

[Olaf Hering]

On Thu, Dec 11, Konrad Rzeszutek Wilk wrote:

> I wonder if we can detect the context during build-time (an autoconf function
> that checks whether the build is done for Fedora?)
> But what if the version of Fedora is different and the object is called
> something else?

Exactly. The build host is not the host where the code runs. It just
happens to be the same in your case.

This and the fact that xenstored cant (appearently) be launched via a
wrapper script makes we wonder how to deal with SELinux...

Olaf

Re: [PATCH 0/4 v2] tools/hotplug: systemd changes for 4.5

[Konrad Rzeszutek Wilk]

On December 11, 2014 2:20:59 PM EST, Olaf Hering <olaf@aepfle.de> wrote:
>On Thu, Dec 11, Konrad Rzeszutek Wilk wrote:
>
>> I wonder if we can detect the context during build-time (an autoconf
>function
>> that checks whether the build is done for Fedora?)
>> But what if the version of Fedora is different and the object is
>called
>> something else?
>
>Exactly. The build host is not the host where the code runs. It just
>happens to be the same in your case.

The file I pasted was the one from the SOURCES directory. Since the end result is an RPM and it works without issues I would surmise the context is OK - for this version  of Fedora. But maybe older ones had this?
>
>This and the fact that xenstored cant (appearently) be launched via a
>wrapper script makes we wonder how to deal with SELinux...
>
>Olaf