[PATCH] tools/hotplug: Use env rather than sh in xenstored.service

[PATCH] tools/hotplug: Use env rather than sh in xenstored.service

[George Dunlap]

Using sh to exec xenstored breaks on selinux systems (at least, on
CentOS 7).  The only purpose of doing that was to be able to expand
the $XENSTORED variable; this can be done with /usr/bin/env instead,
which still works on systemd systems (at least on CentOS 7).

Suggested-by: Olaf Hering <olaf@aepfle.de>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
---

One could argue that this is a bug in 4.6 and should be accepted.  I
could also see an argument, however, that this late in the cycle we
should just wait until 4.6.1.  I'll leave it up to Wei to decide.

CC: Ian Campbell <ian.campbell@citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Olaf Hering <olaf@aepfle.de>
---
 tools/hotplug/Linux/systemd/xenstored.service.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/hotplug/Linux/systemd/xenstored.service.in b/tools/hotplug/Linux/systemd/xenstored.service.in
index a5f836b..09964f3 100644
--- a/tools/hotplug/Linux/systemd/xenstored.service.in
+++ b/tools/hotplug/Linux/systemd/xenstored.service.in
@@ -15,7 +15,7 @@ EnvironmentFile=-@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons
 ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities
 ExecStartPre=-/bin/rm -f @XEN_LIB_STORED@/tdb*
 ExecStartPre=/bin/mkdir -p @XEN_RUN_DIR@
-ExecStart=/bin/sh -c "exec $XENSTORED --no-fork $XENSTORED_ARGS"
+ExecStart=/usr/bin/env $XENSTORED --no-fork $XENSTORED_ARGS
 
 [Install]
 WantedBy=multi-user.target
-- 
1.9.1

Re: [PATCH] tools/hotplug: Use env rather than sh in xenstored.service

[Wei Liu]

On Tue, Sep 15, 2015 at 11:30:00AM +0100, George Dunlap wrote:
> Using sh to exec xenstored breaks on selinux systems (at least, on
> CentOS 7).  The only purpose of doing that was to be able to expand
> the $XENSTORED variable; this can be done with /usr/bin/env instead,
> which still works on systemd systems (at least on CentOS 7).
> 
> Suggested-by: Olaf Hering <olaf@aepfle.de>
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> ---
> 
> One could argue that this is a bug in 4.6 and should be accepted.  I
> could also see an argument, however, that this late in the cycle we
> should just wait until 4.6.1.  I'll leave it up to Wei to decide.
> 

We don't have testing visibility on whether this change will break
systemd without selinux. All distros probably have patches to unit files
anyway so this is not urgent. I would say let's wait until 4.6.1.

Furthermore, a link to systemd manual regarding envvar expansion without
quotes in systemd unit file would also increase confidence why it
doesn't break things.

Wei.

> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Olaf Hering <olaf@aepfle.de>
> ---
>  tools/hotplug/Linux/systemd/xenstored.service.in | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/hotplug/Linux/systemd/xenstored.service.in b/tools/hotplug/Linux/systemd/xenstored.service.in
> index a5f836b..09964f3 100644
> --- a/tools/hotplug/Linux/systemd/xenstored.service.in
> +++ b/tools/hotplug/Linux/systemd/xenstored.service.in
> @@ -15,7 +15,7 @@ EnvironmentFile=-@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons
>  ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities
>  ExecStartPre=-/bin/rm -f @XEN_LIB_STORED@/tdb*
>  ExecStartPre=/bin/mkdir -p @XEN_RUN_DIR@
> -ExecStart=/bin/sh -c "exec $XENSTORED --no-fork $XENSTORED_ARGS"
> +ExecStart=/usr/bin/env $XENSTORED --no-fork $XENSTORED_ARGS
>  
>  [Install]
>  WantedBy=multi-user.target
> -- 
> 1.9.1

Re: [PATCH] tools/hotplug: Use env rather than sh in xenstored.service

[George Dunlap]

On 09/15/2015 11:30 AM, George Dunlap wrote:
> Using sh to exec xenstored breaks on selinux systems (at least, on
> CentOS 7).  The only purpose of doing that was to be able to expand
> the $XENSTORED variable; this can be done with /usr/bin/env instead,
> which still works on systemd systems (at least on CentOS 7).
> 
> Suggested-by: Olaf Hering <olaf@aepfle.de>
> Signed-off-by: George Dunlap <george.dunlap@citrix.com>
> ---
> 
> One could argue that this is a bug in 4.6 and should be accepted.  I
> could also see an argument, however, that this late in the cycle we
> should just wait until 4.6.1.  I'll leave it up to Wei to decide.
> 
> CC: Ian Campbell <ian.campbell@citrix.com>
> CC: Ian Jackson <ian.jackson@citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Olaf Hering <olaf@aepfle.de>
> ---
>  tools/hotplug/Linux/systemd/xenstored.service.in | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/hotplug/Linux/systemd/xenstored.service.in b/tools/hotplug/Linux/systemd/xenstored.service.in
> index a5f836b..09964f3 100644
> --- a/tools/hotplug/Linux/systemd/xenstored.service.in
> +++ b/tools/hotplug/Linux/systemd/xenstored.service.in
> @@ -15,7 +15,7 @@ EnvironmentFile=-@CONFIG_DIR@/@CONFIG_LEAF_DIR@/xencommons
>  ExecStartPre=/bin/grep -q control_d /proc/xen/capabilities
>  ExecStartPre=-/bin/rm -f @XEN_LIB_STORED@/tdb*
>  ExecStartPre=/bin/mkdir -p @XEN_RUN_DIR@
> -ExecStart=/bin/sh -c "exec $XENSTORED --no-fork $XENSTORED_ARGS"
> +ExecStart=/usr/bin/env $XENSTORED --no-fork $XENSTORED_ARGS

Actually, hold off on this one -- apparently env destroys the selinux
context in a way that's too permissive:

# ps axZ | grep xenstored
system_u:system_r:unconfined_service_t:s0 612 ? Ss    0:00
/usr/sbin/xenstored --no-fork

IOW, a side effect this patch is to "fix" the /var/lib/xenstored tmpfs
selinux context problem by basically disabling that selinux limitation. :-/

 -George