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

[Jeff Layton <jlayton@kernel.org>]

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>