| Message ID | 20200311195954.27117-7-fllinden@amazon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | server side user xattr support (RFC 8276) | expand |
Hi Frank,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on nfsd/nfsd-next]
[also build test WARNING on nfs/linux-next linus/master v5.6-rc5 next-20200311]
[cannot apply to cel/for-next]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Frank-van-der-Linden/server-side-user-xattr-support-RFC-8276/20200312-064928
base: git://linux-nfs.org/~bfields/linux.git nfsd-next
reproduce:
# apt-get install sparse
# sparse version: v0.6.1-174-g094d5a94-dirty
make ARCH=x86_64 allmodconfig
make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> fs/nfsd/vfs.c:2102:13: sparse: sparse: incorrect type in assignment (different base types) @@ expected int err @@ got restricted __int err @@
>> fs/nfsd/vfs.c:2102:13: sparse: expected int err
>> fs/nfsd/vfs.c:2102:13: sparse: got restricted __be32
>> fs/nfsd/vfs.c:2104:24: sparse: sparse: incorrect type in return expression (different base types) @@ expected restricted __be32 @@ got be32 @@
>> fs/nfsd/vfs.c:2104:24: sparse: expected restricted __be32
>> fs/nfsd/vfs.c:2104:24: sparse: got int err
fs/nfsd/vfs.c:2108:21: sparse: sparse: incorrect type in assignment (different base types) @@ expected int err @@ got restricted __int err @@
fs/nfsd/vfs.c:2108:21: sparse: expected int err
fs/nfsd/vfs.c:2108:21: sparse: got restricted __be32
fs/nfsd/vfs.c:2112:16: sparse: sparse: incorrect type in return expression (different base types) @@ expected restricted __be32 @@ got be32 @@
fs/nfsd/vfs.c:2112:16: sparse: expected restricted __be32
fs/nfsd/vfs.c:2112:16: sparse: got int err
fs/nfsd/vfs.c:2121:13: sparse: sparse: incorrect type in assignment (different base types) @@ expected int err @@ got restricted __int err @@
fs/nfsd/vfs.c:2121:13: sparse: expected int err
fs/nfsd/vfs.c:2121:13: sparse: got restricted __be32
fs/nfsd/vfs.c:2123:24: sparse: sparse: incorrect type in return expression (different base types) @@ expected restricted __be32 @@ got be32 @@
fs/nfsd/vfs.c:2123:24: sparse: expected restricted __be32
fs/nfsd/vfs.c:2123:24: sparse: got int err
fs/nfsd/vfs.c:2128:21: sparse: sparse: incorrect type in assignment (different base types) @@ expected int err @@ got restricted __int err @@
fs/nfsd/vfs.c:2128:21: sparse: expected int err
fs/nfsd/vfs.c:2128:21: sparse: got restricted __be32
fs/nfsd/vfs.c:2132:16: sparse: sparse: incorrect type in return expression (different base types) @@ expected restricted __be32 @@ got be32 @@
fs/nfsd/vfs.c:2132:16: sparse: expected restricted __be32
fs/nfsd/vfs.c:2132:16: sparse: got int err
fs/nfsd/vfs.c:2148:13: sparse: sparse: incorrect type in assignment (different base types) @@ expected int err @@ got restricted __int err @@
fs/nfsd/vfs.c:2148:13: sparse: expected int err
fs/nfsd/vfs.c:2148:13: sparse: got restricted __be32
fs/nfsd/vfs.c:2150:24: sparse: sparse: incorrect type in return expression (different base types) @@ expected restricted __be32 @@ got be32 @@
fs/nfsd/vfs.c:2150:24: sparse: expected restricted __be32
fs/nfsd/vfs.c:2150:24: sparse: got int err
fs/nfsd/vfs.c:2172:13: sparse: sparse: incorrect type in assignment (different base types) @@ expected int err @@ got restricted __int err @@
fs/nfsd/vfs.c:2172:13: sparse: expected int err
fs/nfsd/vfs.c:2172:13: sparse: got restricted __be32
fs/nfsd/vfs.c:2174:24: sparse: sparse: incorrect type in return expression (different base types) @@ expected restricted __be32 @@ got be32 @@
fs/nfsd/vfs.c:2174:24: sparse: expected restricted __be32
fs/nfsd/vfs.c:2174:24: sparse: got int err
vim +2102 fs/nfsd/vfs.c
2094
2095 __be32
2096 nfsd_getxattr(struct svc_rqst *rqstp, struct svc_fh *fhp, char *name,
2097 void *buf, int *lenp)
2098 {
2099 ssize_t lerr;
2100 int err;
2101
> 2102 err = fh_verify(rqstp, fhp, 0, NFSD_MAY_READ);
2103 if (err)
> 2104 return err;
2105
2106 lerr = vfs_getxattr(fhp->fh_dentry, name, buf, *lenp);
2107 if (lerr < 0)
2108 err = nfsd_xattr_errno(lerr);
2109 else
2110 *lenp = lerr;
2111
2112 return err;
2113 }
2114
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
> On Mar 11, 2020, at 3:59 PM, Frank van der Linden <fllinden@amazon.com> wrote: > > This adds the filehandle based functions for the xattr operations > that call in to the vfs layer to do the actual work. Before posting again, use "make C=1" to clear up new sparse errors, and scripts/checkpatch.pl to find and correct whitespace damage introduced in this series. > Signed-off-by: Frank van der Linden <fllinden@amazon.com> > --- > fs/nfsd/vfs.c | 130 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > fs/nfsd/vfs.h | 10 +++++ > 2 files changed, 140 insertions(+) > > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c > index 0aa02eb18bd3..115449009bc0 100644 > --- a/fs/nfsd/vfs.c > +++ b/fs/nfsd/vfs.c > @@ -2058,6 +2058,136 @@ static int exp_rdonly(struct svc_rqst *rqstp, struct svc_export *exp) > return nfsexp_flags(rqstp, exp) & NFSEXP_READONLY; > } > > +#ifdef CONFIG_NFSD_V4 > +/* > + * Helper function to translate error numbers. In the case of xattr operations, > + * some error codes need to be translated outside of the standard translations. > + * > + * ENODATA needs to be translated to nfserr_noxattr. > + * E2BIG to nfserr_xattr2big. > + * > + * Additionally, vfs_listxattr can return -ERANGE. This means that the > + * file has too many extended attributes to retrieve inside an > + * XATTR_LIST_MAX sized buffer. This is a bug in the xattr implementation: > + * filesystems will allow the adding of extended attributes until they hit > + * their own internal limit. This limit may be larger than XATTR_LIST_MAX. > + * So, at that point, the attributes are present and valid, but can't > + * be retrieved using listxattr, since the upper level xattr code enforces > + * the XATTR_LIST_MAX limit. > + * > + * This bug means that we need to deal with listxattr returning -ERANGE. The > + * best mapping is to return TOOSMALL. > + */ > +static __be32 > +nfsd_xattr_errno(int err) > +{ > + switch (err) { > + case -ENODATA: > + return nfserr_noxattr; > + case -E2BIG: > + return nfserr_xattr2big; > + case -ERANGE: > + return nfserr_toosmall; > + } > + return nfserrno(err); > +} > + > +__be32 > +nfsd_getxattr(struct svc_rqst *rqstp, struct svc_fh *fhp, char *name, > + void *buf, int *lenp) > +{ > + ssize_t lerr; > + int err; > + > + err = fh_verify(rqstp, fhp, 0, NFSD_MAY_READ); > + if (err) > + return err; > + > + lerr = vfs_getxattr(fhp->fh_dentry, name, buf, *lenp); > + if (lerr < 0) > + err = nfsd_xattr_errno(lerr); > + else > + *lenp = lerr; > + > + return err; > +} > + > +__be32 > +nfsd_listxattr(struct svc_rqst *rqstp, struct svc_fh *fhp, void *buf, int *lenp) > +{ > + ssize_t lerr; > + int err; > + > + err = fh_verify(rqstp, fhp, 0, NFSD_MAY_READ); > + if (err) > + return err; > + > + lerr = vfs_listxattr(fhp->fh_dentry, buf, *lenp); > + > + if (lerr < 0) > + err = nfsd_xattr_errno(lerr); > + else > + *lenp = lerr; > + > + return err; > +} > + > +/* > + * Removexattr and setxattr need to call fh_lock to both lock the inode > + * and set the change attribute. Since the top-level vfs_removexattr > + * and vfs_setxattr calls already do their own inode_lock calls, call > + * the _locked variant. Pass in a NULL pointer for delegated_inode, > + * and let the client deal with NFS4ERR_DELAY (same as with e.g. > + * setattr and remove). > + */ > +__be32 > +nfsd_removexattr(struct svc_rqst *rqstp, struct svc_fh *fhp, char *name) > +{ > + int err, ret; > + > + err = fh_verify(rqstp, fhp, 0, NFSD_MAY_WRITE); > + if (err) > + return err; > + > + ret = fh_want_write(fhp); > + if (ret) > + return nfserrno(ret); > + > + fh_lock(fhp); > + > + ret = __vfs_removexattr_locked(fhp->fh_dentry, name, NULL); > + > + fh_unlock(fhp); > + fh_drop_write(fhp); > + > + return nfsd_xattr_errno(ret); > +} > + > +__be32 > +nfsd_setxattr(struct svc_rqst *rqstp, struct svc_fh *fhp, char *name, > + void *buf, u32 len, u32 flags) > +{ > + int err, ret; > + > + err = fh_verify(rqstp, fhp, 0, NFSD_MAY_WRITE); > + if (err) > + return err; > + > + ret = fh_want_write(fhp); > + if (ret) > + return nfserrno(ret); > + fh_lock(fhp); > + > + ret = __vfs_setxattr_locked(fhp->fh_dentry, name, buf, len, flags, > + NULL); > + > + fh_unlock(fhp); > + fh_drop_write(fhp); > + > + return nfsd_xattr_errno(ret); > +} > +#endif > + > /* > * Check for a user's access permissions to this inode. > */ > diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h > index 3eb660ad80d1..2d2cf5b0543b 100644 > --- a/fs/nfsd/vfs.h > +++ b/fs/nfsd/vfs.h > @@ -76,6 +76,16 @@ __be32 do_nfsd_create(struct svc_rqst *, struct svc_fh *, > __be32 nfsd_commit(struct svc_rqst *, struct svc_fh *, > loff_t, unsigned long, __be32 *verf); > #endif /* CONFIG_NFSD_V3 */ > +#ifdef CONFIG_NFSD_V4 > +__be32 nfsd_getxattr(struct svc_rqst *rqstp, struct svc_fh *fhp, > + char *name, void *buf, int *lenp); > +__be32 nfsd_listxattr(struct svc_rqst *rqstp, struct svc_fh *fhp, > + void *buf, int *lenp); > +__be32 nfsd_removexattr(struct svc_rqst *rqstp, struct svc_fh *fhp, > + char *name); > +__be32 nfsd_setxattr(struct svc_rqst *rqstp, struct svc_fh *fhp, > + char *name, void *buf, u32 len, u32 flags); > +#endif > int nfsd_open_break_lease(struct inode *, int); > __be32 nfsd_open(struct svc_rqst *, struct svc_fh *, umode_t, > int, struct file **); > -- > 2.16.6 > -- Chuck Lever
On Thu, Mar 12, 2020 at 12:23:57PM -0400, Chuck Lever wrote: > > On Mar 11, 2020, at 3:59 PM, Frank van der Linden <fllinden@amazon.com> wrote: > > > > This adds the filehandle based functions for the xattr operations > > that call in to the vfs layer to do the actual work. > > Before posting again, use "make C=1" to clear up new sparse > errors, and scripts/checkpatch.pl to find and correct whitespace > damage introduced in this series. Hi Chuck, Thanks for this comment (and the other ones). I forgot to run sparse - I have fixed the __be32/int type mismatches it flagged in my tree. I ran checkpath.pl before sending these in. Looks like I missed one line that is too long. The warnings I didn't fix were: == WARNING: function definition argument 'struct dentry *' should also have an identifier name #156: FILE: include/linux/xattr.h:54: +int __vfs_setxattr_locked(struct dentry *, const char *, const void *, size_t, int, struct inode **); == ..changing this would make the prototype declaration of that function not match with the style of the ones around it (which also don't have argument names in their declarations) - so I decided not to. The other one is: === WARNING: please, no spaces at the start of a line #46: FILE: fs/nfsd/vfs.c:616: + {^INFS4_ACCESS_XAREAD,^INFSD_MAY_READ^I^I^I},$ === The warning is correct, but the array that is part of, and the surrounding accessmap arrays, all have the same spacing. So to both silence checkpacth and make the code look consistent, I'd have to change the spacing in all those arrays (nfs3_regaccess, nfs3_diraccess, nfs3_anyaccess). This didn't seem right, so I didn't do it. I'll be happy to send a separate whitespace cleanup for that, not sure if it should be part of this series, though. Frank
> On Mar 12, 2020, at 1:16 PM, Frank van der Linden <fllinden@amazon.com> wrote: > > On Thu, Mar 12, 2020 at 12:23:57PM -0400, Chuck Lever wrote: >>> On Mar 11, 2020, at 3:59 PM, Frank van der Linden <fllinden@amazon.com> wrote: >>> >>> This adds the filehandle based functions for the xattr operations >>> that call in to the vfs layer to do the actual work. >> >> Before posting again, use "make C=1" to clear up new sparse >> errors, and scripts/checkpatch.pl to find and correct whitespace >> damage introduced in this series. > > Hi Chuck, > > Thanks for this comment (and the other ones). > > I forgot to run sparse - I have fixed the __be32/int type mismatches > it flagged in my tree. > > I ran checkpath.pl before sending these in. Looks like I missed > one line that is too long. The warnings I didn't fix were: > > == > WARNING: function definition argument 'struct dentry *' should also have an identifier name > #156: FILE: include/linux/xattr.h:54: > +int __vfs_setxattr_locked(struct dentry *, const char *, const void *, size_t, int, struct inode **); > == > > ..changing this would make the prototype declaration of that function not > match with the style of the ones around it (which also don't have argument > names in their declarations) - so I decided not to. > > The other one is: > > === > WARNING: please, no spaces at the start of a line > #46: FILE: fs/nfsd/vfs.c:616: > + {^INFS4_ACCESS_XAREAD,^INFSD_MAY_READ^I^I^I},$ > === > > The warning is correct, but the array that is part of, and the surrounding > accessmap arrays, all have the same spacing. So to both silence checkpacth > and make the code look consistent, I'd have to change the spacing in > all those arrays (nfs3_regaccess, nfs3_diraccess, nfs3_anyaccess). > > This didn't seem right, so I didn't do it. Fair enough, please add a comment to that effect to the patch description. > I'll be happy to send a separate whitespace cleanup for that, not sure > if it should be part of this series, though. > > Frank -- Chuck Lever
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c index 0aa02eb18bd3..115449009bc0 100644 --- a/fs/nfsd/vfs.c +++ b/fs/nfsd/vfs.c @@ -2058,6 +2058,136 @@ static int exp_rdonly(struct svc_rqst *rqstp, struct svc_export *exp) return nfsexp_flags(rqstp, exp) & NFSEXP_READONLY; } +#ifdef CONFIG_NFSD_V4 +/* + * Helper function to translate error numbers. In the case of xattr operations, + * some error codes need to be translated outside of the standard translations. + * + * ENODATA needs to be translated to nfserr_noxattr. + * E2BIG to nfserr_xattr2big. + * + * Additionally, vfs_listxattr can return -ERANGE. This means that the + * file has too many extended attributes to retrieve inside an + * XATTR_LIST_MAX sized buffer. This is a bug in the xattr implementation: + * filesystems will allow the adding of extended attributes until they hit + * their own internal limit. This limit may be larger than XATTR_LIST_MAX. + * So, at that point, the attributes are present and valid, but can't + * be retrieved using listxattr, since the upper level xattr code enforces + * the XATTR_LIST_MAX limit. + * + * This bug means that we need to deal with listxattr returning -ERANGE. The + * best mapping is to return TOOSMALL. + */ +static __be32 +nfsd_xattr_errno(int err) +{ + switch (err) { + case -ENODATA: + return nfserr_noxattr; + case -E2BIG: + return nfserr_xattr2big; + case -ERANGE: + return nfserr_toosmall; + } + return nfserrno(err); +} + +__be32 +nfsd_getxattr(struct svc_rqst *rqstp, struct svc_fh *fhp, char *name, + void *buf, int *lenp) +{ + ssize_t lerr; + int err; + + err = fh_verify(rqstp, fhp, 0, NFSD_MAY_READ); + if (err) + return err; + + lerr = vfs_getxattr(fhp->fh_dentry, name, buf, *lenp); + if (lerr < 0) + err = nfsd_xattr_errno(lerr); + else + *lenp = lerr; + + return err; +} + +__be32 +nfsd_listxattr(struct svc_rqst *rqstp, struct svc_fh *fhp, void *buf, int *lenp) +{ + ssize_t lerr; + int err; + + err = fh_verify(rqstp, fhp, 0, NFSD_MAY_READ); + if (err) + return err; + + lerr = vfs_listxattr(fhp->fh_dentry, buf, *lenp); + + if (lerr < 0) + err = nfsd_xattr_errno(lerr); + else + *lenp = lerr; + + return err; +} + +/* + * Removexattr and setxattr need to call fh_lock to both lock the inode + * and set the change attribute. Since the top-level vfs_removexattr + * and vfs_setxattr calls already do their own inode_lock calls, call + * the _locked variant. Pass in a NULL pointer for delegated_inode, + * and let the client deal with NFS4ERR_DELAY (same as with e.g. + * setattr and remove). + */ +__be32 +nfsd_removexattr(struct svc_rqst *rqstp, struct svc_fh *fhp, char *name) +{ + int err, ret; + + err = fh_verify(rqstp, fhp, 0, NFSD_MAY_WRITE); + if (err) + return err; + + ret = fh_want_write(fhp); + if (ret) + return nfserrno(ret); + + fh_lock(fhp); + + ret = __vfs_removexattr_locked(fhp->fh_dentry, name, NULL); + + fh_unlock(fhp); + fh_drop_write(fhp); + + return nfsd_xattr_errno(ret); +} + +__be32 +nfsd_setxattr(struct svc_rqst *rqstp, struct svc_fh *fhp, char *name, + void *buf, u32 len, u32 flags) +{ + int err, ret; + + err = fh_verify(rqstp, fhp, 0, NFSD_MAY_WRITE); + if (err) + return err; + + ret = fh_want_write(fhp); + if (ret) + return nfserrno(ret); + fh_lock(fhp); + + ret = __vfs_setxattr_locked(fhp->fh_dentry, name, buf, len, flags, + NULL); + + fh_unlock(fhp); + fh_drop_write(fhp); + + return nfsd_xattr_errno(ret); +} +#endif + /* * Check for a user's access permissions to this inode. */ diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h index 3eb660ad80d1..2d2cf5b0543b 100644 --- a/fs/nfsd/vfs.h +++ b/fs/nfsd/vfs.h @@ -76,6 +76,16 @@ __be32 do_nfsd_create(struct svc_rqst *, struct svc_fh *, __be32 nfsd_commit(struct svc_rqst *, struct svc_fh *, loff_t, unsigned long, __be32 *verf); #endif /* CONFIG_NFSD_V3 */ +#ifdef CONFIG_NFSD_V4 +__be32 nfsd_getxattr(struct svc_rqst *rqstp, struct svc_fh *fhp, + char *name, void *buf, int *lenp); +__be32 nfsd_listxattr(struct svc_rqst *rqstp, struct svc_fh *fhp, + void *buf, int *lenp); +__be32 nfsd_removexattr(struct svc_rqst *rqstp, struct svc_fh *fhp, + char *name); +__be32 nfsd_setxattr(struct svc_rqst *rqstp, struct svc_fh *fhp, + char *name, void *buf, u32 len, u32 flags); +#endif int nfsd_open_break_lease(struct inode *, int); __be32 nfsd_open(struct svc_rqst *, struct svc_fh *, umode_t, int, struct file **);
This adds the filehandle based functions for the xattr operations that call in to the vfs layer to do the actual work. Signed-off-by: Frank van der Linden <fllinden@amazon.com> --- fs/nfsd/vfs.c | 130 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ fs/nfsd/vfs.h | 10 +++++ 2 files changed, 140 insertions(+)