[06/14] nfsd: define xattr functions to call in to their vfs counterparts
diff mbox series

Message ID 20200311195954.27117-7-fllinden@amazon.com
State New
Headers show
Series
  • server side user xattr support (RFC 8276)
Related show

Commit Message

Frank van der Linden March 11, 2020, 7:59 p.m. UTC
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(+)

Comments

kernel test robot March 12, 2020, 7:37 a.m. UTC | #1
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
Chuck Lever March 12, 2020, 4:23 p.m. UTC | #2
> 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
Frank van der Linden March 12, 2020, 5:16 p.m. UTC | #3
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
Chuck Lever March 12, 2020, 5:57 p.m. UTC | #4
> 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

Patch
diff mbox series

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 **);