Re: [PATCH] NFSv4: xattr handlers should check for absent nfs filehandles

[Scott Mayhew <smayhew@redhat.com>]

On Mon, 14 Apr 2025, Anna Schumaker wrote:

> Hi Scott,
> 
> On 4/11/25 4:16 PM, Scott Mayhew wrote:
> > The nfs inodes for referral anchors that have not yet been followed have
> > their filehandles zeroed out.
> > 
> > Attempting to call getxattr() on one of these will cause the nfs client
> > to send a GETATTR to the nfs server with the preceding PUTFH sans
> > filehandle.  The server will reply NFS4ERR_NOFILEHANDLE, leading to -EIO
> > being returned to the application.
> > 
> > For example:
> > 
> > $ strace -e trace=getxattr getfattr -n system.nfs4_acl /mnt/t/ref
> > getxattr("/mnt/t/ref", "system.nfs4_acl", NULL, 0) = -1 EIO (Input/output error)
> > /mnt/t/ref: system.nfs4_acl: Input/output error
> > +++ exited with 1 +++
> > 
> > Have the xattr handlers return -ENODATA instead.
> 
> This looks like a lot of repeated code. I was wondering if there is a way to
> do this with a helper function that returns -ENODATA? Or could it be checked
> inside nfs4_proc_set_acl() / nfs4_proc_get_acl() / nfs4_server_supports_acls()
> to cut down on duplication?

I'll move the check down into nfs4_proc_set_acl() /
nfs4_proc_get_acl().

> 
> > 
> > Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> > ---
> >  fs/nfs/nfs4proc.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 71 insertions(+)
> > 
> > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > index 970f28dbf253..a01592930370 100644
> > --- a/fs/nfs/nfs4proc.c
> > +++ b/fs/nfs/nfs4proc.c
> > @@ -7933,6 +7933,11 @@ static int nfs4_xattr_set_nfs4_acl(const struct xattr_handler *handler,
> >  				   const char *key, const void *buf,
> >  				   size_t buflen, int flags)
> >  {
> > +	struct nfs_fh *fh = NFS_FH(inode);
> > +
> > +	if (unlikely(fh->size == 0))
> > +		return -ENODATA;
> > +
> >  	return nfs4_proc_set_acl(inode, buf, buflen, NFS4ACL_ACL);
> >  }
> >  
> > @@ -7940,11 +7945,21 @@ static int nfs4_xattr_get_nfs4_acl(const struct xattr_handler *handler,
> >  				   struct dentry *unused, struct inode *inode,
> >  				   const char *key, void *buf, size_t buflen)
> >  {
> > +	struct nfs_fh *fh = NFS_FH(inode);
> > +
> > +	if (unlikely(fh->size == 0))
> > +		return -ENODATA;
> > +
> >  	return nfs4_proc_get_acl(inode, buf, buflen, NFS4ACL_ACL);
> >  }
> >  
> >  static bool nfs4_xattr_list_nfs4_acl(struct dentry *dentry)
> >  {
> > +	struct nfs_fh *fh = NFS_FH(d_inode(dentry));
> > +
> > +	if (unlikely(fh->size == 0))
> > +		return -ENODATA;
> 
> Note that the return value of this function is a boolean, and not an integer,
> so callers probably won't care about a specific return value.

Doh!  I blame the fact that I was fighting a severe head and chest cold
last week.  After a fresh look, I don't think we need to change the list
handlers at all - those get called by the listxattr inode operation,
which nfs_referral_inode_operations does not have.

-Scott
> 
> > +
> >  	return nfs4_server_supports_acls(NFS_SB(dentry->d_sb), NFS4ACL_ACL);
> >  }
> >  
> > @@ -7957,6 +7972,11 @@ static int nfs4_xattr_set_nfs4_dacl(const struct xattr_handler *handler,
> >  				    const char *key, const void *buf,
> >  				    size_t buflen, int flags)
> >  {
> > +	struct nfs_fh *fh = NFS_FH(inode);
> > +
> > +	if (unlikely(fh->size == 0))
> > +		return -ENODATA;
> > +
> >  	return nfs4_proc_set_acl(inode, buf, buflen, NFS4ACL_DACL);
> >  }
> >  
> > @@ -7964,11 +7984,21 @@ static int nfs4_xattr_get_nfs4_dacl(const struct xattr_handler *handler,
> >  				    struct dentry *unused, struct inode *inode,
> >  				    const char *key, void *buf, size_t buflen)
> >  {
> > +	struct nfs_fh *fh = NFS_FH(inode);
> > +
> > +	if (unlikely(fh->size == 0))
> > +		return -ENODATA;
> > +
> >  	return nfs4_proc_get_acl(inode, buf, buflen, NFS4ACL_DACL);
> >  }
> >  
> >  static bool nfs4_xattr_list_nfs4_dacl(struct dentry *dentry)
> >  {
> > +	struct nfs_fh *fh = NFS_FH(d_inode(dentry));
> > +
> > +	if (unlikely(fh->size == 0))
> > +		return -ENODATA;
> 
> Here's another boolean return type.
> 
> > +
> >  	return nfs4_server_supports_acls(NFS_SB(dentry->d_sb), NFS4ACL_DACL);
> >  }
> >  
> > @@ -7980,6 +8010,11 @@ static int nfs4_xattr_set_nfs4_sacl(const struct xattr_handler *handler,
> >  				    const char *key, const void *buf,
> >  				    size_t buflen, int flags)
> >  {
> > +	struct nfs_fh *fh = NFS_FH(inode);
> > +
> > +	if (unlikely(fh->size == 0))
> > +		return -ENODATA;
> > +
> >  	return nfs4_proc_set_acl(inode, buf, buflen, NFS4ACL_SACL);
> >  }
> >  
> > @@ -7987,11 +8022,21 @@ static int nfs4_xattr_get_nfs4_sacl(const struct xattr_handler *handler,
> >  				    struct dentry *unused, struct inode *inode,
> >  				    const char *key, void *buf, size_t buflen)
> >  {
> > +	struct nfs_fh *fh = NFS_FH(inode);
> > +
> > +	if (unlikely(fh->size == 0))
> > +		return -ENODATA;
> > +
> >  	return nfs4_proc_get_acl(inode, buf, buflen, NFS4ACL_SACL);
> >  }
> >  
> >  static bool nfs4_xattr_list_nfs4_sacl(struct dentry *dentry)
> >  {
> > +	struct nfs_fh *fh = NFS_FH(d_inode(dentry));
> > +
> > +	if (unlikely(fh->size == 0))
> > +		return -ENODATA;
> 
> Boolean return type.
> 
> Thanks,
> Anna
> 
> > +
> >  	return nfs4_server_supports_acls(NFS_SB(dentry->d_sb), NFS4ACL_SACL);
> >  }
> >  
> > @@ -8005,6 +8050,11 @@ static int nfs4_xattr_set_nfs4_label(const struct xattr_handler *handler,
> >  				     const char *key, const void *buf,
> >  				     size_t buflen, int flags)
> >  {
> > +	struct nfs_fh *fh = NFS_FH(inode);
> > +
> > +	if (unlikely(fh->size == 0))
> > +		return -ENODATA;
> > +
> >  	if (security_ismaclabel(key))
> >  		return nfs4_set_security_label(inode, buf, buflen);
> >  
> > @@ -8015,6 +8065,11 @@ static int nfs4_xattr_get_nfs4_label(const struct xattr_handler *handler,
> >  				     struct dentry *unused, struct inode *inode,
> >  				     const char *key, void *buf, size_t buflen)
> >  {
> > +	struct nfs_fh *fh = NFS_FH(inode);
> > +
> > +	if (unlikely(fh->size == 0))
> > +		return -ENODATA;
> > +
> >  	if (security_ismaclabel(key))
> >  		return nfs4_get_security_label(inode, buf, buflen);
> >  	return -EOPNOTSUPP;
> > @@ -8023,8 +8078,12 @@ static int nfs4_xattr_get_nfs4_label(const struct xattr_handler *handler,
> >  static ssize_t
> >  nfs4_listxattr_nfs4_label(struct inode *inode, char *list, size_t list_len)
> >  {
> > +	struct nfs_fh *fh = NFS_FH(inode);
> >  	int len = 0;
> >  
> > +	if (unlikely(fh->size == 0))
> > +		return -ENODATA;
> > +
> >  	if (nfs_server_capable(inode, NFS_CAP_SECURITY_LABEL)) {
> >  		len = security_inode_listsecurity(inode, list, list_len);
> >  		if (len >= 0 && list_len && len > list_len)
> > @@ -8056,9 +8115,13 @@ static int nfs4_xattr_set_nfs4_user(const struct xattr_handler *handler,
> >  				    const char *key, const void *buf,
> >  				    size_t buflen, int flags)
> >  {
> > +	struct nfs_fh *fh = NFS_FH(inode);
> >  	u32 mask;
> >  	int ret;
> >  
> > +	if (unlikely(fh->size == 0))
> > +		return -ENODATA;
> > +
> >  	if (!nfs_server_capable(inode, NFS_CAP_XATTR))
> >  		return -EOPNOTSUPP;
> >  
> > @@ -8093,9 +8156,13 @@ static int nfs4_xattr_get_nfs4_user(const struct xattr_handler *handler,
> >  				    struct dentry *unused, struct inode *inode,
> >  				    const char *key, void *buf, size_t buflen)
> >  {
> > +	struct nfs_fh *fh = NFS_FH(inode);
> >  	u32 mask;
> >  	ssize_t ret;
> >  
> > +	if (unlikely(fh->size == 0))
> > +		return -ENODATA;
> > +
> >  	if (!nfs_server_capable(inode, NFS_CAP_XATTR))
> >  		return -EOPNOTSUPP;
> >  
> > @@ -8120,6 +8187,7 @@ static int nfs4_xattr_get_nfs4_user(const struct xattr_handler *handler,
> >  static ssize_t
> >  nfs4_listxattr_nfs4_user(struct inode *inode, char *list, size_t list_len)
> >  {
> > +	struct nfs_fh *fh = NFS_FH(inode);
> >  	u64 cookie;
> >  	bool eof;
> >  	ssize_t ret, size;
> > @@ -8127,6 +8195,9 @@ nfs4_listxattr_nfs4_user(struct inode *inode, char *list, size_t list_len)
> >  	size_t buflen;
> >  	u32 mask;
> >  
> > +	if (unlikely(fh->size == 0))
> > +		return -ENODATA;
> > +
> >  	if (!nfs_server_capable(inode, NFS_CAP_XATTR))
> >  		return 0;
> >  
>