Re: [PATCH 1/1] proc01: Whitelist /proc/fs/nfsd/nfsv4recoverydir

[Jeff Layton <jlayton@kernel.org>]

On Tue, 2024-04-16 at 09:52 +1000, NeilBrown wrote:
> On Tue, 16 Apr 2024, Chuck Lever wrote:
> > On Mon, Apr 15, 2024 at 01:43:37PM -0400, Jeff Layton wrote:
> > > On Mon, 2024-04-15 at 17:37 +0000, Chuck Lever III wrote:
> > > > 
> > > > > On Apr 15, 2024, at 1:35 PM, Jeff Layton <jlayton@kernel.org> wrote:
> > > > > 
> > > > > On Mon, 2024-04-15 at 17:27 +0000, Chuck Lever III wrote:
> > > > > > 
> > > > > > > On Apr 15, 2024, at 1:21 PM, Petr Vorel <pvorel@suse.cz> wrote:
> > > > > > > 
> > > > > > > /proc/fs/nfsd/nfsv4recoverydir started from kernel 6.8 report EINVAL.
> > > > > > > 
> > > > > > > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > > > > > > ---
> > > > > > > Hi,
> > > > > > > 
> > > > > > > @ Jeff, Chuck, Neil, NFS devs: The patch itself whitelist reading
> > > > > > > /proc/fs/nfsd/nfsv4recoverydir in LTP test. I suspect reading failed
> > > > > > > with EINVAL in 6.8 was a deliberate change and expected behavior when
> > > > > > > CONFIG_NFSD_LEGACY_CLIENT_TRACKING is not set:
> > > > > > 
> > > > > > I'm not sure it was deliberate. This seems like a behavior
> > > > > > regression. Jeff?
> > > > > > 
> > > > > 
> > > > > I don't think I intended to make it return -EINVAL. I guess that's what
> > > > > happens when there is no entry for it in the write_op array.
> > > > > 
> > > > > With CONFIG_NFSD_LEGACY_CLIENT_TRACKING disabled, that file has no
> > > > > meaning or value at all anymore. Maybe we should just remove the dentry
> > > > > altogether when CONFIG_NFSD_LEGACY_CLIENT_TRACKING is disabled?
> > > > 
> > > > My understanding of the rules about modifying this part of
> > > > the kernel-user interface is that the file has to stay, even
> > > > though it's now a no-op.
> > > > 
> > > 
> > > Does it?  Where are these rules written? 
> > > 
> > > What should we have it do now when read and written? Maybe EOPNOTSUPP
> > > would be better, if we can make it just return an error?
> > > 
> > > We could also make it just discard written data, and present a blank
> > > string when read. What do the rules say we are required to do here?
> > 
> > The best I could find was Documentation/process/stable-api-nonsense.rst.
> > 
> > Tell you what, you and Petr work out what you'd like to do, let's
> > figure out the right set of folks to review changes in /proc, and
> > we'll go from there. If no-one has a problem removing the file, I'm
> > not going to stand in the way.
> 
> I don't think we need any external review for this.  While the file is
> in /proc, it is not in procfs but in nfsdfs.  So people out side the
> nfsd community are unlikely to care.  And this isn't a hard removal.  It
> is just a new config option that allows a file to be removed.
> 
> I think we do want to completely remove the file, not just let it return
> an error:
> --- a/fs/nfsd/nfsctl.c
> +++ b/fs/nfsd/nfsctl.c
> @@ -51,7 +51,9 @@ enum {
>  #ifdef CONFIG_NFSD_V4
>  	NFSD_Leasetime,
>  	NFSD_Gracetime,
> +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING
>  	NFSD_RecoveryDir,
> +#endif
>  	NFSD_V4EndGrace,
>  #endif
>  	NFSD_MaxReserved
> @@ -1360,7 +1362,9 @@ static int nfsd_fill_super(struct super_block *sb, struct fs_context *fc)
>  #ifdef CONFIG_NFSD_V4
>  		[NFSD_Leasetime] = {"nfsv4leasetime", &transaction_ops, S_IWUSR|S_IRUSR},
>  		[NFSD_Gracetime] = {"nfsv4gracetime", &transaction_ops, S_IWUSR|S_IRUSR},
> +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING
>  		[NFSD_RecoveryDir] = {"nfsv4recoverydir", &transaction_ops, S_IWUSR|S_IRUSR},
> +#endif
>  		[NFSD_V4EndGrace] = {"v4_end_grace", &transaction_ops, S_IWUSR|S_IRUGO},
>  #endif
>  		/* last one */ {""}
> 

I'm fine with this patch if you want to propose it formally.

Reviewed-by: Jeff Layton <jlayton@kernel.org>

> 
> My understand of the stability rule is "if Linus doesn't hear about it,
> then it isn't a regression".  Also known as "no harm, no foul".
> 
> So if we manage the change to everyone's satisfaction, then it is
> perfectly OK to make the change.  nfs-utils already handles a missing
> file fairly well - you get a D_GENERAL log message, but that is all.
> Petr's fix for ltp should allow it to work.  I would be greatly
> surprised if anything else (except possibly other testing code) would
> care.

That was my thinking too. nfs-utils should handle the lack of this file
gracefully, and nothing else should really care. The LTP test is just
accessing all of the files under /proc so if that file goes missing, it
shouldn't care either.

We can update nfs-utils to silence the log message in later versions
too. In fact, it's probably a good time to think about removing the code
that accesses that file, since it's only used by nfsdcld to convert
"legacy" setups.
-- 
Jeff Layton <jlayton@kernel.org>