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

[Anna Schumaker <anna.schumaker@oracle.com>]

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?

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

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