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

[NeilBrown]

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 */ {""}


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.

NeilBrown