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

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

[Petr Vorel]

/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:

$ sudo cat /proc/fs/nfsd/nfsv4recoverydir
cat: /proc/fs/nfsd/nfsv4recoverydir: Invalid argument

I'm asking because It worked fine in kernel 6.7:

$ sudo cat /proc/fs/nfsd/nfsv4recoverydir
/var/lib/nfs/v4recovery

I did not bisect but I suspect suspect 74fd48739d04 ("nfsd: new Kconfig
option for legacy client tracking") from v6.8-rc1. The system I test
(openSUSE Tumbleweed) has not CONFIG_NFSD_LEGACY_CLIENT_TRACKING set and
74fd48739d04 wraps write_recoverydir setup, thus it's not set.

+#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING
        [NFSD_RecoveryDir] = write_recoverydir,
+#endif

Kind regards,
Petr

 testcases/kernel/fs/proc/proc01.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/testcases/kernel/fs/proc/proc01.c b/testcases/kernel/fs/proc/proc01.c
index c90e509a3..08b9bbc75 100644
--- a/testcases/kernel/fs/proc/proc01.c
+++ b/testcases/kernel/fs/proc/proc01.c
@@ -113,6 +113,7 @@ static const struct mapping known_issues[] = {
 	{"read", "/proc/fs/nfsd/filehandle", EINVAL},
 	{"read", "/proc/fs/nfsd/.getfs", EINVAL},
 	{"read", "/proc/fs/nfsd/.getfd", EINVAL},
+	{"read", "/proc/fs/nfsd/nfsv4recoverydir", EINVAL},
 	{"read", "/proc/self/net/rpc/use-gss-proxy", EAGAIN},
 	{"read", "/proc/sys/net/ipv6/conf/*/stable_secret", EIO},
 	{"read", "/proc/sys/vm/nr_hugepages", EOPNOTSUPP},
-- 
2.43.0

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

[Chuck Lever III]

> 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?


> $ sudo cat /proc/fs/nfsd/nfsv4recoverydir
> cat: /proc/fs/nfsd/nfsv4recoverydir: Invalid argument
> 
> I'm asking because It worked fine in kernel 6.7:
> 
> $ sudo cat /proc/fs/nfsd/nfsv4recoverydir
> /var/lib/nfs/v4recovery
> 
> I did not bisect but I suspect suspect 74fd48739d04 ("nfsd: new Kconfig
> option for legacy client tracking") from v6.8-rc1. The system I test
> (openSUSE Tumbleweed) has not CONFIG_NFSD_LEGACY_CLIENT_TRACKING set and
> 74fd48739d04 wraps write_recoverydir setup, thus it's not set.
> 
> +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING
>        [NFSD_RecoveryDir] = write_recoverydir,
> +#endif
> 
> Kind regards,
> Petr
> 
> testcases/kernel/fs/proc/proc01.c | 1 +
> 1 file changed, 1 insertion(+)
> 
> diff --git a/testcases/kernel/fs/proc/proc01.c b/testcases/kernel/fs/proc/proc01.c
> index c90e509a3..08b9bbc75 100644
> --- a/testcases/kernel/fs/proc/proc01.c
> +++ b/testcases/kernel/fs/proc/proc01.c
> @@ -113,6 +113,7 @@ static const struct mapping known_issues[] = {
> {"read", "/proc/fs/nfsd/filehandle", EINVAL},
> {"read", "/proc/fs/nfsd/.getfs", EINVAL},
> {"read", "/proc/fs/nfsd/.getfd", EINVAL},
> + {"read", "/proc/fs/nfsd/nfsv4recoverydir", EINVAL},
> {"read", "/proc/self/net/rpc/use-gss-proxy", EAGAIN},
> {"read", "/proc/sys/net/ipv6/conf/*/stable_secret", EIO},
> {"read", "/proc/sys/vm/nr_hugepages", EOPNOTSUPP},
> -- 
> 2.43.0
> 
> 

--
Chuck Lever

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

[Jeff Layton]

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?

> 
> > $ sudo cat /proc/fs/nfsd/nfsv4recoverydir
> > cat: /proc/fs/nfsd/nfsv4recoverydir: Invalid argument
> > 
> > I'm asking because It worked fine in kernel 6.7:
> > 
> > $ sudo cat /proc/fs/nfsd/nfsv4recoverydir
> > /var/lib/nfs/v4recovery
> > 
> > I did not bisect but I suspect suspect 74fd48739d04 ("nfsd: new Kconfig
> > option for legacy client tracking") from v6.8-rc1. The system I test
> > (openSUSE Tumbleweed) has not CONFIG_NFSD_LEGACY_CLIENT_TRACKING set and
> > 74fd48739d04 wraps write_recoverydir setup, thus it's not set.
> > 
> > +#ifdef CONFIG_NFSD_LEGACY_CLIENT_TRACKING
> >        [NFSD_RecoveryDir] = write_recoverydir,
> > +#endif
> > 
> > Kind regards,
> > Petr
> > 
> > testcases/kernel/fs/proc/proc01.c | 1 +
> > 1 file changed, 1 insertion(+)
> > 
> > diff --git a/testcases/kernel/fs/proc/proc01.c b/testcases/kernel/fs/proc/proc01.c
> > index c90e509a3..08b9bbc75 100644
> > --- a/testcases/kernel/fs/proc/proc01.c
> > +++ b/testcases/kernel/fs/proc/proc01.c
> > @@ -113,6 +113,7 @@ static const struct mapping known_issues[] = {
> > {"read", "/proc/fs/nfsd/filehandle", EINVAL},
> > {"read", "/proc/fs/nfsd/.getfs", EINVAL},
> > {"read", "/proc/fs/nfsd/.getfd", EINVAL},
> > + {"read", "/proc/fs/nfsd/nfsv4recoverydir", EINVAL},
> > {"read", "/proc/self/net/rpc/use-gss-proxy", EAGAIN},
> > {"read", "/proc/sys/net/ipv6/conf/*/stable_secret", EIO},
> > {"read", "/proc/sys/vm/nr_hugepages", EOPNOTSUPP},
> > -- 
> > 2.43.0
> > 
> > 
> 
> --
> Chuck Lever
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>

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

[Chuck Lever III]

> 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.


--
Chuck Lever

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

[Jeff Layton]

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?

-- 
Jeff Layton <jlayton@kernel.org>

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

[Petr Vorel]

> 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.

First, thanks a lot for handling this.

> Does it? Where are these rules written? 

I wonder myself as well.

> 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?

FYI current exceptions on /proc files in whole kernel have various errnos, e.g.
EINVAL, EOPNOTSUPP:
https://github.com/linux-test-project/ltp/blob/master/testcases/kernel/fs/proc/proc01.c#L81

Kind regards,
Petr

> 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?

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

[Chuck Lever]

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.


-- 
Chuck Lever

Re: [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

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

[Jeff Layton]

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>

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

[Chuck Lever]

On Tue, Apr 16, 2024 at 09:52:11AM +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:

'kay, no objection.

Can you send an "official" patch with a description and SOB?


> --- 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

-- 
Chuck Lever

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

[Petr Vorel]

Hi all,

> /proc/fs/nfsd/nfsv4recoverydir started from kernel 6.8 report EINVAL.

Because Neil sent fix, I withdraw this patch.

Kind regards,
Petr

[1] https://lore.kernel.org/linux-nfs/171330258224.17212.9790424282163530018@noble.neil.brown.name/