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@xxxxxxxxxx>
> > ---
> > 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;
> >
>
