Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 18 Jul 2023 22:16:00 +0000
From:   Alberto Garcia <berto@igalia.com>
To:     Steve Dickson <steved@redhat.com>
Cc:     linux-nfs@vger.kernel.org
Subject: Re: [PATCH] systemd: Ensure that statdpath exists using
 systemd-tmpfiles
Message-ID: <ZLcPIEUeVKElhqwB@igalia.com>
References: <20230713102531.131072-1-berto@igalia.com>
 <5230337e-b028-0e86-9693-c29f7d1165b2@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <5230337e-b028-0e86-9693-c29f7d1165b2@redhat.com>
Precedence: bulk

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