[PATCH] systemd: Ensure that statdpath exists using systemd-tmpfiles

[PATCH] systemd: Ensure that statdpath exists using systemd-tmpfiles

[Alberto Garcia]

The NFS utils store their state under /var/lib/nfs and they can
generally handle the case where that directory is missing by creating
the appropriate files and directories automatically.

This is not the case of rpc-statd: if sm and sm.bak (under $statdpath,
which also defaults to /var/lib/nfs) are missing the daemon will
refuse to start and will exit with an error.

If nfs-utils is configured with systemd support it can take advantage
of systemd-tmpfiles to ensure that the state directories are always
present and have the appropriate ownership.

This would normally be handled with the StateDirectory directive in
rpc-statd.service, however that method would not be able to change the
ownership of the directories to $statduser because this daemon needs
to be run as root, and only later changes its uid and gid.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 configure.ac              | 1 +
 systemd/Makefile.am       | 5 +++++
 systemd/nfs-utils.conf.in | 4 ++++
 3 files changed, 10 insertions(+)
 create mode 100644 systemd/nfs-utils.conf.in

diff --git a/configure.ac b/configure.ac
index 6fbcb974..fe958ab3 100644
--- a/configure.ac
+++ b/configure.ac
@@ -695,6 +695,7 @@ AC_CONFIG_COMMANDS_PRE([eval eval _rpc_pipefsmount=$rpc_pipefsmount])
 
 AC_CONFIG_FILES([
 	Makefile
+	systemd/nfs-utils.conf
 	systemd/rpc-gssd.service
 	systemd/rpc_pipefs.target
 	systemd/var-lib-nfs-rpc_pipefs.mount
diff --git a/systemd/Makefile.am b/systemd/Makefile.am
index b4483222..6127986e 100644
--- a/systemd/Makefile.am
+++ b/systemd/Makefile.am
@@ -5,6 +5,9 @@ MAINTAINERCLEANFILES = Makefile.in
 udev_rulesdir = /usr/lib/udev/rules.d/
 udev_files = 60-nfs.rules
 
+sdtmpfilesdir = /usr/lib/tmpfiles.d/
+sdtmpfiles_files = nfs-utils.conf
+
 unit_files =  \
     nfs-client.target \
     rpc_pipefs.target \
@@ -85,4 +88,6 @@ install-data-hook: $(unit_files) $(udev_files)
 	cp $(rpc_pipefs_mount_file) $(DESTDIR)/$(unitdir)/$(rpc_pipefsmount)
 	mkdir -p $(DESTDIR)/$(udev_rulesdir)
 	cp $(udev_files) $(DESTDIR)/$(udev_rulesdir)
+	mkdir -p $(DESTDIR)/$(sdtmpfilesdir)
+	cp $(sdtmpfiles_files) $(DESTDIR)/$(sdtmpfilesdir)
 endif
diff --git a/systemd/nfs-utils.conf.in b/systemd/nfs-utils.conf.in
new file mode 100644
index 00000000..a44c337e
--- /dev/null
+++ b/systemd/nfs-utils.conf.in
@@ -0,0 +1,4 @@
+# This is a systemd-tmpfiles configuration file
+# type path			mode	uid	gid	age	argument
+d      @statdpath@/sm		0700	@statduser@	:root	-       -
+d      @statdpath@/sm.bak	0700	@statduser@	:root	-	-
-- 
2.39.2

Re: [PATCH] systemd: Ensure that statdpath exists using systemd-tmpfiles

[Steve Dickson]

Hello,

On 7/13/23 6:25 AM, Alberto Garcia wrote:
> The NFS utils store their state under /var/lib/nfs and they can
> generally handle the case where that directory is missing by creating
> the appropriate files and directories automatically.
> 
> This is not the case of rpc-statd: if sm and sm.bak (under $statdpath,
> which also defaults to /var/lib/nfs) are missing the daemon will
> refuse to start and will exit with an error.
Why are they would be missing? They are created on the
nfs-utils installation.

> 
> If nfs-utils is configured with systemd support it can take advantage
> of systemd-tmpfiles to ensure that the state directories are always
> present and have the appropriate ownership.
> 
> This would normally be handled with the StateDirectory directive in
> rpc-statd.service, however that method would not be able to change the
> ownership of the directories to $statduser because this daemon needs
> to be run as root, and only later changes its uid and gid.
Just curious... how did you test this patch? When I apply it
I get this error

Failed to insert: creating /var/lib/nfs/statd/sm/<client>: Permission denied
STAT_FAIL to <server> for SM_MON of <server_ip>

Maybe this is packing issue but I'm thinking it is more
of systemd issue... the permissions on the sm directory
are
283 drwx------. 2 nobody rpcuser 6 Apr 18 20:00 /var/lib/nfs/statd/sm
instead of
283 drwx------. 2 rpcuser rpcuser 6 Apr 18 20:00 /var/lib/nfs/statd/sm

Even when I change the owner to rpcuser, I still get the
permission error...

steved.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>   configure.ac              | 1 +
>   systemd/Makefile.am       | 5 +++++
>   systemd/nfs-utils.conf.in | 4 ++++
>   3 files changed, 10 insertions(+)
>   create mode 100644 systemd/nfs-utils.conf.in
> 
> diff --git a/configure.ac b/configure.ac
> index 6fbcb974..fe958ab3 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -695,6 +695,7 @@ AC_CONFIG_COMMANDS_PRE([eval eval _rpc_pipefsmount=$rpc_pipefsmount])
>   
>   AC_CONFIG_FILES([
>   	Makefile
> +	systemd/nfs-utils.conf
>   	systemd/rpc-gssd.service
>   	systemd/rpc_pipefs.target
>   	systemd/var-lib-nfs-rpc_pipefs.mount
> diff --git a/systemd/Makefile.am b/systemd/Makefile.am
> index b4483222..6127986e 100644
> --- a/systemd/Makefile.am
> +++ b/systemd/Makefile.am
> @@ -5,6 +5,9 @@ MAINTAINERCLEANFILES = Makefile.in
>   udev_rulesdir = /usr/lib/udev/rules.d/
>   udev_files = 60-nfs.rules
>   
> +sdtmpfilesdir = /usr/lib/tmpfiles.d/
> +sdtmpfiles_files = nfs-utils.conf
> +
>   unit_files =  \
>       nfs-client.target \
>       rpc_pipefs.target \
> @@ -85,4 +88,6 @@ install-data-hook: $(unit_files) $(udev_files)
>   	cp $(rpc_pipefs_mount_file) $(DESTDIR)/$(unitdir)/$(rpc_pipefsmount)
>   	mkdir -p $(DESTDIR)/$(udev_rulesdir)
>   	cp $(udev_files) $(DESTDIR)/$(udev_rulesdir)
> +	mkdir -p $(DESTDIR)/$(sdtmpfilesdir)
> +	cp $(sdtmpfiles_files) $(DESTDIR)/$(sdtmpfilesdir)
>   endif
> diff --git a/systemd/nfs-utils.conf.in b/systemd/nfs-utils.conf.in
> new file mode 100644
> index 00000000..a44c337e
> --- /dev/null
> +++ b/systemd/nfs-utils.conf.in
> @@ -0,0 +1,4 @@
> +# This is a systemd-tmpfiles configuration file
> +# type path			mode	uid	gid	age	argument
> +d      @statdpath@/sm		0700	@statduser@	:root	-       -
> +d      @statdpath@/sm.bak	0700	@statduser@	:root	-	-

Re: [PATCH] systemd: Ensure that statdpath exists using systemd-tmpfiles

[Alberto Garcia]

On Sat, Jul 15, 2023 at 04:53:02PM -0400, Steve Dickson wrote:
> > This is not the case of rpc-statd: if sm and sm.bak (under
> > $statdpath, which also defaults to /var/lib/nfs) are missing the
> > daemon will refuse to start and will exit with an error.
> Why are they would be missing? They are created on the nfs-utils
> installation.

Hello,

yes, in a traditional Linux system that is indeed the case. The idea
behind this is to add support to factory reset and stateless scenarios
like the ones described here:

   https://0pointer.net/blog/projects/stateless.html

The goal is that a system can boot with an empty /var and
all necessary files and directories are created without user
intervention. In the case of nfs-utils this is already happening
except for rpc-statd.

For projects that use systemd this is generally easy to do without
touching the code because systemd provides directives that can be used
to ensure that /var/lib/foo, /var/log/foo, etc. exist before a service
is started.

In the rpc-statd case this would normally be as simple as adding
something like "StateDirectory=nfs/sm nfs/sm.bak" to the .service
file. However it seems that this one is a bit special because it goes
like this if I'm not mistaken:

1. The configure script determines $statduser (the value of
   --with-statduser, else rpcuser if available, else nobody).

2. 'make install' creates sm / sm.bak followed by chown $statduser

3. rpc.statd starts as root, then does lstat("/var/lib/nfs/sm", &st)
   and finally setgid(st.st_gid) / setuid(st.st_uid). At this point
   uid/gid is not necessarily what was set during configure/make
   install ($statduser/root) because downstreams can create a
   different user/group and change the ownership of those directories.

StateDirectory and similar directives from systemd can only create
directories owned by the user that starts the service, but since here
the service needs to run as root this would not work.

systemd-tmpfiles can be used for cases like this one, and that's why I
chose it for this patch.

> Just curious... how did you test this patch? When I apply it
> I get this error
> 
> Failed to insert: creating /var/lib/nfs/statd/sm/<client>: Permission denied
> STAT_FAIL to <server> for SM_MON of <server_ip>
> 
> Maybe this is packing issue but I'm thinking it is more
> of systemd issue... the permissions on the sm directory
> are
> 283 drwx------. 2 nobody rpcuser 6 Apr 18 20:00 /var/lib/nfs/statd/sm
> instead of
> 283 drwx------. 2 rpcuser rpcuser 6 Apr 18 20:00 /var/lib/nfs/statd/sm

Are you creating a package with the patched sources? If it's something
like the Fedora one then I think that the problem is that since the
configure script does not use --with-statduser then there's a mismatch
between the user that appears in nfs-utils.conf (added by this patch)
and these lines from the .spec file:

%dir %attr(700,rpcuser,rpcuser) %{_sharedstatedir}/nfs/statd
%dir %attr(700,rpcuser,rpcuser) %{_sharedstatedir}/nfs/statd/sm
%dir %attr(700,rpcuser,rpcuser) %{_sharedstatedir}/nfs/statd/sm.bak

So probably /var/lib/nfs/statd/sm is drwx------ nobody but
            /var/lib/nfs/statd    is drwx------ rpcuser ?

Passing --with-statduser=rpcuser to configure should fix this problem.

After having a look at a couple of downstream packages it seems that
they simply don't use --with-statduser at all and change the ownership
to whatever user/group they want in their post-installation scripts.
So they would need to start doing it if this patch is included in
nfs-utils.

I realize that although this should be trivial to handle by downstream
packagers it does require manual intervention so I'm not expecting it
to be completely uncontroversial. But if you like the overall idea I'm
happy to discuss / iterate this patch further. This can of course be
applied only by the downstreams who are interested in this feature,
but since nfs-utils already uses systemd and the change is rather
small I thought it made more sense to have it directly upstream.

Regards,

Berto

Re: [PATCH] systemd: Ensure that statdpath exists using systemd-tmpfiles

[Steve Dickson]

Sorry for the delay... I'll take another look

steved.

On 7/18/23 6:16 PM, Alberto Garcia wrote:
> On Sat, Jul 15, 2023 at 04:53:02PM -0400, Steve Dickson wrote:
>>> This is not the case of rpc-statd: if sm and sm.bak (under
>>> $statdpath, which also defaults to /var/lib/nfs) are missing the
>>> daemon will refuse to start and will exit with an error.
>> Why are they would be missing? They are created on the nfs-utils
>> installation.
> 
> Hello,
> 
> yes, in a traditional Linux system that is indeed the case. The idea
> behind this is to add support to factory reset and stateless scenarios
> like the ones described here:
> 
>     https://0pointer.net/blog/projects/stateless.html
> 
> The goal is that a system can boot with an empty /var and
> all necessary files and directories are created without user
> intervention. In the case of nfs-utils this is already happening
> except for rpc-statd.
> 
> For projects that use systemd this is generally easy to do without
> touching the code because systemd provides directives that can be used
> to ensure that /var/lib/foo, /var/log/foo, etc. exist before a service
> is started.
> 
> In the rpc-statd case this would normally be as simple as adding
> something like "StateDirectory=nfs/sm nfs/sm.bak" to the .service
> file. However it seems that this one is a bit special because it goes
> like this if I'm not mistaken:
> 
> 1. The configure script determines $statduser (the value of
>     --with-statduser, else rpcuser if available, else nobody).
> 
> 2. 'make install' creates sm / sm.bak followed by chown $statduser
> 
> 3. rpc.statd starts as root, then does lstat("/var/lib/nfs/sm", &st)
>     and finally setgid(st.st_gid) / setuid(st.st_uid). At this point
>     uid/gid is not necessarily what was set during configure/make
>     install ($statduser/root) because downstreams can create a
>     different user/group and change the ownership of those directories.
> 
> StateDirectory and similar directives from systemd can only create
> directories owned by the user that starts the service, but since here
> the service needs to run as root this would not work.
> 
> systemd-tmpfiles can be used for cases like this one, and that's why I
> chose it for this patch.
> 
>> Just curious... how did you test this patch? When I apply it
>> I get this error
>>
>> Failed to insert: creating /var/lib/nfs/statd/sm/<client>: Permission denied
>> STAT_FAIL to <server> for SM_MON of <server_ip>
>>
>> Maybe this is packing issue but I'm thinking it is more
>> of systemd issue... the permissions on the sm directory
>> are
>> 283 drwx------. 2 nobody rpcuser 6 Apr 18 20:00 /var/lib/nfs/statd/sm
>> instead of
>> 283 drwx------. 2 rpcuser rpcuser 6 Apr 18 20:00 /var/lib/nfs/statd/sm
> 
> Are you creating a package with the patched sources? If it's something
> like the Fedora one then I think that the problem is that since the
> configure script does not use --with-statduser then there's a mismatch
> between the user that appears in nfs-utils.conf (added by this patch)
> and these lines from the .spec file:
> 
> %dir %attr(700,rpcuser,rpcuser) %{_sharedstatedir}/nfs/statd
> %dir %attr(700,rpcuser,rpcuser) %{_sharedstatedir}/nfs/statd/sm
> %dir %attr(700,rpcuser,rpcuser) %{_sharedstatedir}/nfs/statd/sm.bak
> 
> So probably /var/lib/nfs/statd/sm is drwx------ nobody but
>              /var/lib/nfs/statd    is drwx------ rpcuser ?
> 
> Passing --with-statduser=rpcuser to configure should fix this problem.
> 
> After having a look at a couple of downstream packages it seems that
> they simply don't use --with-statduser at all and change the ownership
> to whatever user/group they want in their post-installation scripts.
> So they would need to start doing it if this patch is included in
> nfs-utils.
> 
> I realize that although this should be trivial to handle by downstream
> packagers it does require manual intervention so I'm not expecting it
> to be completely uncontroversial. But if you like the overall idea I'm
> happy to discuss / iterate this patch further. This can of course be
> applied only by the downstreams who are interested in this feature,
> but since nfs-utils already uses systemd and the change is rather
> small I thought it made more sense to have it directly upstream.
> 
> Regards,
> 
> Berto
>