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

[Chuck Lever <chuck.lever@oracle.com>]

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