[PATCH v1] NFSD: Define actions for the new time_deleg FATTR4 attributes

[PATCH v1] NFSD: Define actions for the new time_deleg FATTR4 attributes

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

NFSv4 clients won't send legitimate GETATTR requests for these new
attributes because they are intended to be used only with CB_GETATTR.
But NFSD has to do something besides crashing if it ever sees such
a request. The correct thing to do is ignore them.

Reported-by: rtm@csail.mit.edu
Closes: https://lore.kernel.org/linux-nfs/7819419cf0cb50d8130dc6b747765d2b8febc88a.camel@kernel.org/T/#t
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs4xdr.c | 2 ++
 1 file changed, 2 insertions(+)

Compile-tested only.

diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index c0a3c6a7c8bb..97e9e9afa80a 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -3560,6 +3560,8 @@ static const nfsd4_enc_attr nfsd4_enc_fattr4_encode_ops[] = {
 
 	[FATTR4_MODE_UMASK]		= nfsd4_encode_fattr4__noop,
 	[FATTR4_XATTR_SUPPORT]		= nfsd4_encode_fattr4_xattr_support,
+	[FATTR4_TIME_DELEG_ACCESS]	= nfsd4_encode_fattr4__noop,
+	[FATTR4_TIME_DELEG_MODIFY]	= nfsd4_encode_fattr4__noop,
 	[FATTR4_OPEN_ARGUMENTS]		= nfsd4_encode_fattr4_open_arguments,
 };
 
-- 
2.50.0

Re: [PATCH v1] NFSD: Define actions for the new time_deleg FATTR4 attributes

[Chuck Lever]

On 9/10/25 11:29 AM, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> NFSv4 clients won't send legitimate GETATTR requests for these new
> attributes because they are intended to be used only with CB_GETATTR.
> But NFSD has to do something besides crashing if it ever sees such
> a request. The correct thing to do is ignore them.
> 
> Reported-by: rtm@csail.mit.edu
> Closes: https://lore.kernel.org/linux-nfs/7819419cf0cb50d8130dc6b747765d2b8febc88a.camel@kernel.org/T/#t

Fixes: 51c0d4f7e317 ("nfsd: add support for FATTR4_OPEN_ARGUMENTS")
Cc: stable@vger.kernel.org


> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/nfs4xdr.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> Compile-tested only.
> 
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index c0a3c6a7c8bb..97e9e9afa80a 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -3560,6 +3560,8 @@ static const nfsd4_enc_attr nfsd4_enc_fattr4_encode_ops[] = {
>  
>  	[FATTR4_MODE_UMASK]		= nfsd4_encode_fattr4__noop,
>  	[FATTR4_XATTR_SUPPORT]		= nfsd4_encode_fattr4_xattr_support,
> +	[FATTR4_TIME_DELEG_ACCESS]	= nfsd4_encode_fattr4__noop,
> +	[FATTR4_TIME_DELEG_MODIFY]	= nfsd4_encode_fattr4__noop,
>  	[FATTR4_OPEN_ARGUMENTS]		= nfsd4_encode_fattr4_open_arguments,
>  };
>  


-- 
Chuck Lever

Re: [PATCH v1] NFSD: Define actions for the new time_deleg FATTR4 attributes

[Jeff Layton]

On Wed, 2025-09-10 at 11:29 -0400, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> NFSv4 clients won't send legitimate GETATTR requests for these new
> attributes because they are intended to be used only with CB_GETATTR.
> But NFSD has to do something besides crashing if it ever sees such
> a request. The correct thing to do is ignore them.
> 
> Reported-by: rtm@csail.mit.edu
> Closes: https://lore.kernel.org/linux-nfs/7819419cf0cb50d8130dc6b747765d2b8febc88a.camel@kernel.org/T/#t
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>

Maybe also this?

Fixes: 51c0d4f7e317 ("nfsd: add support for FATTR4_OPEN_ARGUMENTS")

> ---
>  fs/nfsd/nfs4xdr.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> Compile-tested only.
> 
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index c0a3c6a7c8bb..97e9e9afa80a 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -3560,6 +3560,8 @@ static const nfsd4_enc_attr nfsd4_enc_fattr4_encode_ops[] = {
>  
>  	[FATTR4_MODE_UMASK]		= nfsd4_encode_fattr4__noop,
>  	[FATTR4_XATTR_SUPPORT]		= nfsd4_encode_fattr4_xattr_support,
> +	[FATTR4_TIME_DELEG_ACCESS]	= nfsd4_encode_fattr4__noop,
> +	[FATTR4_TIME_DELEG_MODIFY]	= nfsd4_encode_fattr4__noop,
>  	[FATTR4_OPEN_ARGUMENTS]		= nfsd4_encode_fattr4_open_arguments,
>  };
>  

Thanks for fixing this!

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

Re: [PATCH v1] NFSD: Define actions for the new time_deleg FATTR4 attributes

[Chuck Lever]

On 9/10/25 8:29 AM, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> NFSv4 clients won't send legitimate GETATTR requests for these new
> attributes because they are intended to be used only with CB_GETATTR.
> But NFSD has to do something besides crashing if it ever sees such
> a request. The correct thing to do is ignore them.
> 
> Reported-by: rtm@csail.mit.edu
> Closes: https://lore.kernel.org/linux-nfs/7819419cf0cb50d8130dc6b747765d2b8febc88a.camel@kernel.org/T/#t
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/nfs4xdr.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> Compile-tested only.
> 
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index c0a3c6a7c8bb..97e9e9afa80a 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -3560,6 +3560,8 @@ static const nfsd4_enc_attr nfsd4_enc_fattr4_encode_ops[] = {
>  
>  	[FATTR4_MODE_UMASK]		= nfsd4_encode_fattr4__noop,
>  	[FATTR4_XATTR_SUPPORT]		= nfsd4_encode_fattr4_xattr_support,
> +	[FATTR4_TIME_DELEG_ACCESS]	= nfsd4_encode_fattr4__noop,
> +	[FATTR4_TIME_DELEG_MODIFY]	= nfsd4_encode_fattr4__noop,
>  	[FATTR4_OPEN_ARGUMENTS]		= nfsd4_encode_fattr4_open_arguments,
>  };
>  

I think we might need more here, because this introduces a bug.
(Not one that a working client will hit, though).

nfsd4_encode_fattr4() needs to clear these two bits before processing
the bitmask. Otherwise the client will expect to see nfs4time objects in
the returned attribute list.

I'm not sure if I should remove TIME_DELEG_ACCESS and TIME_DELEG_MODIFY
from the "supported attributes" mask for forward GETATTR requests,
though; or should nfsd4_encode_fattr4() explicitly mask these two
out when it copies word 2 of the request_mask to the reply_mask?


-- 
Chuck Lever

Re: [PATCH v1] NFSD: Define actions for the new time_deleg FATTR4 attributes

[Jeff Layton]

On Mon, 2025-09-29 at 09:16 -0400, Chuck Lever wrote:
> On 9/10/25 8:29 AM, Chuck Lever wrote:
> > From: Chuck Lever <chuck.lever@oracle.com>
> > 
> > NFSv4 clients won't send legitimate GETATTR requests for these new
> > attributes because they are intended to be used only with CB_GETATTR.
> > But NFSD has to do something besides crashing if it ever sees such
> > a request. The correct thing to do is ignore them.
> > 
> > Reported-by: rtm@csail.mit.edu
> > Closes: https://lore.kernel.org/linux-nfs/7819419cf0cb50d8130dc6b747765d2b8febc88a.camel@kernel.org/T/#t
> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > ---
> >  fs/nfsd/nfs4xdr.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > Compile-tested only.
> > 
> > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > index c0a3c6a7c8bb..97e9e9afa80a 100644
> > --- a/fs/nfsd/nfs4xdr.c
> > +++ b/fs/nfsd/nfs4xdr.c
> > @@ -3560,6 +3560,8 @@ static const nfsd4_enc_attr nfsd4_enc_fattr4_encode_ops[] = {
> >  
> >  	[FATTR4_MODE_UMASK]		= nfsd4_encode_fattr4__noop,
> >  	[FATTR4_XATTR_SUPPORT]		= nfsd4_encode_fattr4_xattr_support,
> > +	[FATTR4_TIME_DELEG_ACCESS]	= nfsd4_encode_fattr4__noop,
> > +	[FATTR4_TIME_DELEG_MODIFY]	= nfsd4_encode_fattr4__noop,
> >  	[FATTR4_OPEN_ARGUMENTS]		= nfsd4_encode_fattr4_open_arguments,
> >  };
> >  
> 
> I think we might need more here, because this introduces a bug.
> (Not one that a working client will hit, though).
> 
> nfsd4_encode_fattr4() needs to clear these two bits before processing
> the bitmask. Otherwise the client will expect to see nfs4time objects in
> the returned attribute list.
> 

Isn't that a problem for any attr that is set to
nfsd4_encode_fattr4__noop ? The client is going to expect to see
something in there for it.

> I'm not sure if I should remove TIME_DELEG_ACCESS and TIME_DELEG_MODIFY
> from the "supported attributes" mask for forward GETATTR requests,
> though; or should nfsd4_encode_fattr4() explicitly mask these two
> out when it copies word 2 of the request_mask to the reply_mask?
> 

Why not both? IMO:

We shouldn't advertise them as supported attrs, since they weren't
intended to be queryable via GETATTR. At the same time, we should deal
properly with attempts to query them even though we said we don't
support them (probably by just masking them off).

If we did that, then we could add a WARN_ON_ONCE() to
nfsd4_encode_fattr4__noop() since it (theoretically) should never get
called.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v1] NFSD: Define actions for the new time_deleg FATTR4 attributes

[Chuck Lever]

On 9/29/25 6:29 AM, Jeff Layton wrote:
> On Mon, 2025-09-29 at 09:16 -0400, Chuck Lever wrote:
>> On 9/10/25 8:29 AM, Chuck Lever wrote:
>>> From: Chuck Lever <chuck.lever@oracle.com>
>>>
>>> NFSv4 clients won't send legitimate GETATTR requests for these new
>>> attributes because they are intended to be used only with CB_GETATTR.
>>> But NFSD has to do something besides crashing if it ever sees such
>>> a request. The correct thing to do is ignore them.
>>>
>>> Reported-by: rtm@csail.mit.edu
>>> Closes: https://lore.kernel.org/linux-nfs/7819419cf0cb50d8130dc6b747765d2b8febc88a.camel@kernel.org/T/#t
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>>  fs/nfsd/nfs4xdr.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> Compile-tested only.
>>>
>>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>>> index c0a3c6a7c8bb..97e9e9afa80a 100644
>>> --- a/fs/nfsd/nfs4xdr.c
>>> +++ b/fs/nfsd/nfs4xdr.c
>>> @@ -3560,6 +3560,8 @@ static const nfsd4_enc_attr nfsd4_enc_fattr4_encode_ops[] = {
>>>  
>>>  	[FATTR4_MODE_UMASK]		= nfsd4_encode_fattr4__noop,
>>>  	[FATTR4_XATTR_SUPPORT]		= nfsd4_encode_fattr4_xattr_support,
>>> +	[FATTR4_TIME_DELEG_ACCESS]	= nfsd4_encode_fattr4__noop,
>>> +	[FATTR4_TIME_DELEG_MODIFY]	= nfsd4_encode_fattr4__noop,
>>>  	[FATTR4_OPEN_ARGUMENTS]		= nfsd4_encode_fattr4_open_arguments,
>>>  };
>>>  
>>
>> I think we might need more here, because this introduces a bug.
>> (Not one that a working client will hit, though).
>>
>> nfsd4_encode_fattr4() needs to clear these two bits before processing
>> the bitmask. Otherwise the client will expect to see nfs4time objects in
>> the returned attribute list.
>>
> 
> Isn't that a problem for any attr that is set to
> nfsd4_encode_fattr4__noop ? The client is going to expect to see
> something in there for it.

I need to review. __noop might just mean "don't return anything"
rather than "not supported".


>> I'm not sure if I should remove TIME_DELEG_ACCESS and TIME_DELEG_MODIFY
>> from the "supported attributes" mask for forward GETATTR requests,
>> though; or should nfsd4_encode_fattr4() explicitly mask these two
>> out when it copies word 2 of the request_mask to the reply_mask?
>>
> 
> Why not both? IMO:
> 
> We shouldn't advertise them as supported attrs, since they weren't
> intended to be queryable via GETATTR. At the same time, we should deal
> properly with attempts to query them even though we said we don't
> support them (probably by just masking them off).

Do clients query SUPPORTED_ATTRS and look for these two bits to
know whether to expect attribute delegation?


> If we did that, then we could add a WARN_ON_ONCE() to
> nfsd4_encode_fattr4__noop() since it (theoretically) should never get
> called.


-- 
Chuck Lever

Re: [PATCH v1] NFSD: Define actions for the new time_deleg FATTR4 attributes

[Jeff Layton]

On Mon, 2025-09-29 at 09:32 -0400, Chuck Lever wrote:
> On 9/29/25 6:29 AM, Jeff Layton wrote:
> > On Mon, 2025-09-29 at 09:16 -0400, Chuck Lever wrote:
> > > On 9/10/25 8:29 AM, Chuck Lever wrote:
> > > > From: Chuck Lever <chuck.lever@oracle.com>
> > > > 
> > > > NFSv4 clients won't send legitimate GETATTR requests for these new
> > > > attributes because they are intended to be used only with CB_GETATTR.
> > > > But NFSD has to do something besides crashing if it ever sees such
> > > > a request. The correct thing to do is ignore them.
> > > > 
> > > > Reported-by: rtm@csail.mit.edu
> > > > Closes: https://lore.kernel.org/linux-nfs/7819419cf0cb50d8130dc6b747765d2b8febc88a.camel@kernel.org/T/#t
> > > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > > > ---
> > > >  fs/nfsd/nfs4xdr.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > Compile-tested only.
> > > > 
> > > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > > > index c0a3c6a7c8bb..97e9e9afa80a 100644
> > > > --- a/fs/nfsd/nfs4xdr.c
> > > > +++ b/fs/nfsd/nfs4xdr.c
> > > > @@ -3560,6 +3560,8 @@ static const nfsd4_enc_attr nfsd4_enc_fattr4_encode_ops[] = {
> > > >  
> > > >  	[FATTR4_MODE_UMASK]		= nfsd4_encode_fattr4__noop,
> > > >  	[FATTR4_XATTR_SUPPORT]		= nfsd4_encode_fattr4_xattr_support,
> > > > +	[FATTR4_TIME_DELEG_ACCESS]	= nfsd4_encode_fattr4__noop,
> > > > +	[FATTR4_TIME_DELEG_MODIFY]	= nfsd4_encode_fattr4__noop,
> > > >  	[FATTR4_OPEN_ARGUMENTS]		= nfsd4_encode_fattr4_open_arguments,
> > > >  };
> > > >  
> > > 
> > > I think we might need more here, because this introduces a bug.
> > > (Not one that a working client will hit, though).
> > > 
> > > nfsd4_encode_fattr4() needs to clear these two bits before processing
> > > the bitmask. Otherwise the client will expect to see nfs4time objects in
> > > the returned attribute list.
> > > 
> > 
> > Isn't that a problem for any attr that is set to
> > nfsd4_encode_fattr4__noop ? The client is going to expect to see
> > something in there for it.
> 
> I need to review. __noop might just mean "don't return anything"
> rather than "not supported".
> 

It just returns nfs_ok without encoding anything for that attr bit.

> 
> > > I'm not sure if I should remove TIME_DELEG_ACCESS and TIME_DELEG_MODIFY
> > > from the "supported attributes" mask for forward GETATTR requests,
> > > though; or should nfsd4_encode_fattr4() explicitly mask these two
> > > out when it copies word 2 of the request_mask to the reply_mask?
> > > 
> > 
> > Why not both? IMO:
> > 
> > We shouldn't advertise them as supported attrs, since they weren't
> > intended to be queryable via GETATTR. At the same time, we should deal
> > properly with attempts to query them even though we said we don't
> > support them (probably by just masking them off).
> 
> Do clients query SUPPORTED_ATTRS and look for these two bits to
> know whether to expect attribute delegation?
> 

The Linux client does:

static bool nfs4_server_delegtime_capable(struct nfs4_server_caps_res *res)
{
        u32 share_access_want = res->open_caps.oa_share_access_want[0];
        u32 attr_bitmask = res->attr_bitmask[2];

        return (share_access_want & NFS4_SHARE_WANT_DELEG_TIMESTAMPS) &&
               ((attr_bitmask & FATTR4_WORD2_NFS42_TIME_DELEG_MASK) ==
                                        FATTR4_WORD2_NFS42_TIME_DELEG_MASK);
}


...so I guess we can't report them as unsupported. They do still need
to be supported for SETATTR. Still, we should just mask them off if
someone tries to query for them.

> > If we did that, then we could add a WARN_ON_ONCE() to
> > nfsd4_encode_fattr4__noop() since it (theoretically) should never get
> > called.
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v1] NFSD: Define actions for the new time_deleg FATTR4 attributes

[Chuck Lever]

On 9/29/25 6:39 AM, Jeff Layton wrote:
>> Do clients query SUPPORTED_ATTRS and look for these two bits to
>> know whether to expect attribute delegation?
>>
> The Linux client does:
> 
> static bool nfs4_server_delegtime_capable(struct nfs4_server_caps_res *res)
> {
>         u32 share_access_want = res->open_caps.oa_share_access_want[0];
>         u32 attr_bitmask = res->attr_bitmask[2];
> 
>         return (share_access_want & NFS4_SHARE_WANT_DELEG_TIMESTAMPS) &&
>                ((attr_bitmask & FATTR4_WORD2_NFS42_TIME_DELEG_MASK) ==
>                                         FATTR4_WORD2_NFS42_TIME_DELEG_MASK);
> }
> 
> 
> ...so I guess we can't report them as unsupported. They do still need
> to be supported for SETATTR. Still, we should just mask them off if
> someone tries to query for them.

There appears to be a bit of a gray area here. RFC 8881 Section 18.7.3
states:

> The server MUST return a value for each attribute that the client
> requests if the attribute is supported by the server for the target
> file system. If the server does not support a particular attribute on
> the target file system, then it MUST NOT return the attribute value
> and MUST NOT set the attribute bit in the result bitmap. The server
> MUST return an error if it supports an attribute on the target but
> cannot obtain its value. In that case, no attribute values will be
> returned.

RFC 9754 Section 5 states:

> These new attributes are invalid to be used with GETATTR, VERIFY, and
> NVERIFY, and they can only be used with CB_GETATTR and SETATTR by a
> client holding an appropriate delegation.

This text does not prescribe a specific server response if it should be
presented with a GETATTR operation (or a READDIR, one assumes) that
queries delegated time stamps.

Perhaps, rather than clearing the result bitmask, NFS4ERR_INVAL is the
mandated server response.


-- 
Chuck Lever

Re: [PATCH v1] NFSD: Define actions for the new time_deleg FATTR4 attributes

[Jeff Layton]

On Mon, 2025-09-29 at 12:37 -0400, Chuck Lever wrote:
> On 9/29/25 6:39 AM, Jeff Layton wrote:
> > > Do clients query SUPPORTED_ATTRS and look for these two bits to
> > > know whether to expect attribute delegation?
> > > 
> > The Linux client does:
> > 
> > static bool nfs4_server_delegtime_capable(struct nfs4_server_caps_res *res)
> > {
> >         u32 share_access_want = res->open_caps.oa_share_access_want[0];
> >         u32 attr_bitmask = res->attr_bitmask[2];
> > 
> >         return (share_access_want & NFS4_SHARE_WANT_DELEG_TIMESTAMPS) &&
> >                ((attr_bitmask & FATTR4_WORD2_NFS42_TIME_DELEG_MASK) ==
> >                                         FATTR4_WORD2_NFS42_TIME_DELEG_MASK);
> > }
> > 
> > 
> > ...so I guess we can't report them as unsupported. They do still need
> > to be supported for SETATTR. Still, we should just mask them off if
> > someone tries to query for them.
> 
> There appears to be a bit of a gray area here. RFC 8881 Section 18.7.3
> states:
> 
> > The server MUST return a value for each attribute that the client
> > requests if the attribute is supported by the server for the target
> > file system. If the server does not support a particular attribute on
> > the target file system, then it MUST NOT return the attribute value
> > and MUST NOT set the attribute bit in the result bitmap. The server
> > MUST return an error if it supports an attribute on the target but
> > cannot obtain its value. In that case, no attribute values will be
> > returned.
> 
> RFC 9754 Section 5 states:
> 
> > These new attributes are invalid to be used with GETATTR, VERIFY, and
> > NVERIFY, and they can only be used with CB_GETATTR and SETATTR by a
> > client holding an appropriate delegation.
> 
> This text does not prescribe a specific server response if it should be
> presented with a GETATTR operation (or a READDIR, one assumes) that
> queries delegated time stamps.
> 
> Perhaps, rather than clearing the result bitmask, NFS4ERR_INVAL is the
> mandated server response.
> 

It would be very strange for a legit client to request this, so a hard
error would be fine too.
-- 
Jeff Layton <jlayton@kernel.org>