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;
Message-ID: <42fe1621-d9c3-2fd5-807c-539ddd917ac8@redhat.com>
Date:   Tue, 25 Jul 2023 11:58:38 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH] systemd: Ensure that statdpath exists using
 systemd-tmpfiles
To:     Alberto Garcia <berto@igalia.com>
Cc:     linux-nfs@vger.kernel.org
References: <20230713102531.131072-1-berto@igalia.com>
 <5230337e-b028-0e86-9693-c29f7d1165b2@redhat.com>
 <ZLcPIEUeVKElhqwB@igalia.com>
Content-Language: en-US
From:   Steve Dickson <steved@redhat.com>
In-Reply-To: <ZLcPIEUeVKElhqwB@igalia.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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
>