| Message ID | 20230713102531.131072-1-berto@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | systemd: Ensure that statdpath exists using systemd-tmpfiles | expand |
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 - -
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
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 >
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 - -
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