diff mbox

NFS client broken in Linus' tip

Message ID 1391458935.17089.1.camel@leira.trondhjem.org (mailing list archive)
State New, archived
Headers show

Commit Message

Trond Myklebust Feb. 3, 2014, 8:22 p.m. UTC 
On Mon, 2014-02-03 at 10:45 -0500, Trond Myklebust wrote:
> On Feb 3, 2014, at 9:57, Christoph Hellwig <hch@infradead.org> wrote:
> 
> > On Mon, Feb 03, 2014 at 09:17:30AM -0500, Trond Myklebust wrote:
> >> As I said above, that causes posix_acl_xattr_get() to return the wrong answer (ENODATA instead of EOPNOTSUPP).
> > 
> > Is it really the wrong answer?  How does userspace care wether this
> > server doesn't support ACLs at all or none is set?  The resulting
> > behavior is the same.
> 
> It will certainly cause acl_get_file() to behave differently than previously. I’ve no idea how that will affect applications, though.
> 
> > If there's a good reason to care we might have to go with your patch,
> > but if we can avoid it I'd prefer to keep things simple.
> 
> One alternative is to simply wrap posix_acl_xattr_get() in fs/nfs/nfs3acl.c, and have it check the value of nfs_server_capable(inode, NFS_CAP_ACLS) before returning ENODATA. That’s rather ugly too...

FWIW, here is the alternative patch. I've tested it, and it seems to
work.
8<---------------------------------------------------------------------
From 2e527b12169a67e9cfcf43898ae4d15bcfa1ede9 Mon Sep 17 00:00:00 2001
From: Trond Myklebust <trond.myklebust@primarydata.com>
Date: Mon, 3 Feb 2014 13:07:05 -0500
Subject: [PATCH] NFSv3: Fix return values for get_acl()

Ensure that nfs3_get_acl() returns NULL when the server doesn't support
POSIX acls. This ensures that posix_acl_create() does the right thing,
and applies the current_umask() to the mode before returning.

Add a wrapper around posix_acl_xattr_get() so that we continue to
return EOPNOTSUPP when the server doesn't support POSIX acls. Otherwise,
the NULL return from nfs3_get_acl() will cause ENODATA to be returned.

Also add the appropriate exports to posix_acl_xattr_get, posix_acl_xattr_set
and posix_acl_xattr_list to enable their use in the wrapper.

Reported-by: Russell King <linux@arm.linux.org.uk>
Link: http://lkml.kernel.org/r/20140130140834.GW15937@n2100.arm.linux.org.uk
Cc: Christoph Hellwig <hch@lst.de>
Cc: Al Viro viro@zeniv.linux.org.uk>
Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/nfs/nfs3acl.c                | 42 ++++++++++++++++++++++++++++++++++++-----
 fs/posix_acl.c                  |  9 ++++++---
 include/linux/posix_acl_xattr.h |  8 ++++++++
 3 files changed, 51 insertions(+), 8 deletions(-)

Comments

Russell King - ARM Linux Feb. 3, 2014, 8:25 p.m. UTC | #1 
On Mon, Feb 03, 2014 at 03:22:15PM -0500, Trond Myklebust wrote:
> On Mon, 2014-02-03 at 10:45 -0500, Trond Myklebust wrote:
> > On Feb 3, 2014, at 9:57, Christoph Hellwig <hch@infradead.org> wrote:
> > 
> > > On Mon, Feb 03, 2014 at 09:17:30AM -0500, Trond Myklebust wrote:
> > >> As I said above, that causes posix_acl_xattr_get() to return the wrong answer (ENODATA instead of EOPNOTSUPP).
> > > 
> > > Is it really the wrong answer?  How does userspace care wether this
> > > server doesn't support ACLs at all or none is set?  The resulting
> > > behavior is the same.
> > 
> > It will certainly cause acl_get_file() to behave differently than previously. I’ve no idea how that will affect applications, though.
> > 
> > > If there's a good reason to care we might have to go with your patch,
> > > but if we can avoid it I'd prefer to keep things simple.
> > 
> > One alternative is to simply wrap posix_acl_xattr_get() in fs/nfs/nfs3acl.c, and have it check the value of nfs_server_capable(inode, NFS_CAP_ACLS) before returning ENODATA. That’s rather ugly too...
> 
> FWIW, here is the alternative patch. I've tested it, and it seems to
> work.

Thanks.

As there's now two fixes, which would you like me to test?

One comment on this patch though:

> +static int
> +nfs_posix_acl_xattr_get(struct dentry *dentry, const char *name,
> +		void *value, size_t size, int type)
> +{
> +	int ret;
> +
> +	ret = posix_acl_xattr_get(dentry, name, value, size, type);
> +	/*
> +	 * This check is needed to override the ENODATA error that
> +	 * posix_acl_xattr_get will return if the acl probe fails.
> +	 */
> +	if (!nfs_server_capable(dentry->d_inode, NFS_CAP_ACLS))
> +		ret = -EOPNOTSUPP;

I'm not familiar with this code, but the above looks slightly weird,
and a little suspicious - especially with the lack of blank line
before the comment.  Is the above actually intended?
Christoph Hellwig Feb. 3, 2014, 8:25 p.m. UTC | #2 
On Mon, Feb 03, 2014 at 03:22:15PM -0500, Trond Myklebust wrote:
> FWIW, here is the alternative patch. I've tested it, and it seems to
> work.

I much prefer the original one.  One major point of the series was to
get individual filesystems out of the business of providing xattr
handlers for ACLs.
Trond Myklebust Feb. 3, 2014, 8:32 p.m. UTC | #3 
On Feb 3, 2014, at 15:25, Christoph Hellwig <hch@infradead.org> wrote:

> On Mon, Feb 03, 2014 at 03:22:15PM -0500, Trond Myklebust wrote:
>> FWIW, here is the alternative patch. I've tested it, and it seems to
>> work.
> 
> I much prefer the original one.  One major point of the series was to
> get individual filesystems out of the business of providing xattr
> handlers for ACLs.
> 

Then could you and Al please provide an Acked-by? This is a userspace API, so we shouldn’t be changing return values without good reason.

--
Trond Myklebust
Linux NFS client maintainer
diff mbox

Patch

diff --git a/fs/nfs/nfs3acl.c b/fs/nfs/nfs3acl.c
index 871d6eda8dba..d1bc84f22f64 100644
--- a/fs/nfs/nfs3acl.c
+++ b/fs/nfs/nfs3acl.c
@@ -28,8 +28,10 @@  struct posix_acl *nfs3_get_acl(struct inode *inode, int type)
 	};
 	int status, count;
 
-	if (!nfs_server_capable(inode, NFS_CAP_ACLS))
-		return ERR_PTR(-EOPNOTSUPP);
+	if (!nfs_server_capable(inode, NFS_CAP_ACLS)) {
+		cache_no_acl(inode);
+		return NULL;
+	}
 
 	status = nfs_revalidate_inode(server, inode);
 	if (status < 0)
@@ -70,7 +72,7 @@  struct posix_acl *nfs3_get_acl(struct inode *inode, int type)
 			dprintk("NFS_V3_ACL extension not supported; disabling\n");
 			server->caps &= ~NFS_CAP_ACLS;
 		case -ENOTSUPP:
-			status = -EOPNOTSUPP;
+			status = 0;
 		default:
 			goto getout;
 	}
@@ -242,8 +244,38 @@  fail:
 	return PTR_ERR(alloc);
 }
 
+static int
+nfs_posix_acl_xattr_get(struct dentry *dentry, const char *name,
+		void *value, size_t size, int type)
+{
+	int ret;
+
+	ret = posix_acl_xattr_get(dentry, name, value, size, type);
+	/*
+	 * This check is needed to override the ENODATA error that
+	 * posix_acl_xattr_get will return if the acl probe fails.
+	 */
+	if (!nfs_server_capable(dentry->d_inode, NFS_CAP_ACLS))
+		ret = -EOPNOTSUPP;
+	return ret;
+}
+
+static const struct xattr_handler nfs_posix_acl_access_xattr_handler = {
+	.prefix = POSIX_ACL_XATTR_ACCESS,
+	.flags = ACL_TYPE_ACCESS,
+	.get = nfs_posix_acl_xattr_get,
+	.set = posix_acl_xattr_set,
+};
+
+static const struct xattr_handler nfs_posix_acl_default_xattr_handler = {
+	.prefix = POSIX_ACL_XATTR_DEFAULT,
+	.flags = ACL_TYPE_DEFAULT,
+	.get = nfs_posix_acl_xattr_get,
+	.set = posix_acl_xattr_set,
+};
+
 const struct xattr_handler *nfs3_xattr_handlers[] = {
-	&posix_acl_access_xattr_handler,
-	&posix_acl_default_xattr_handler,
+	&nfs_posix_acl_access_xattr_handler,
+	&nfs_posix_acl_default_xattr_handler,
 	NULL,
 };
diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index 38bae5a0ea25..835167f92952 100644
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -750,7 +750,7 @@  posix_acl_to_xattr(struct user_namespace *user_ns, const struct posix_acl *acl,
 }
 EXPORT_SYMBOL (posix_acl_to_xattr);
 
-static int
+int
 posix_acl_xattr_get(struct dentry *dentry, const char *name,
 		void *value, size_t size, int type)
 {
@@ -773,8 +773,9 @@  posix_acl_xattr_get(struct dentry *dentry, const char *name,
 
 	return error;
 }
+EXPORT_SYMBOL_GPL(posix_acl_xattr_get);
 
-static int
+int
 posix_acl_xattr_set(struct dentry *dentry, const char *name,
 		const void *value, size_t size, int flags, int type)
 {
@@ -809,8 +810,9 @@  out:
 	posix_acl_release(acl);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(posix_acl_xattr_set);
 
-static size_t
+size_t
 posix_acl_xattr_list(struct dentry *dentry, char *list, size_t list_size,
 		const char *name, size_t name_len, int type)
 {
@@ -832,6 +834,7 @@  posix_acl_xattr_list(struct dentry *dentry, char *list, size_t list_size,
 		memcpy(list, xname, size);
 	return size;
 }
+EXPORT_SYMBOL_GPL(posix_acl_xattr_list);
 
 const struct xattr_handler posix_acl_access_xattr_handler = {
 	.prefix = POSIX_ACL_XATTR_ACCESS,
diff --git a/include/linux/posix_acl_xattr.h b/include/linux/posix_acl_xattr.h
index 6f14ee295822..fc062003a456 100644
--- a/include/linux/posix_acl_xattr.h
+++ b/include/linux/posix_acl_xattr.h
@@ -69,6 +69,14 @@  struct posix_acl *posix_acl_from_xattr(struct user_namespace *user_ns,
 int posix_acl_to_xattr(struct user_namespace *user_ns,
 		       const struct posix_acl *acl, void *buffer, size_t size);
 
+int posix_acl_xattr_get(struct dentry *dentry, const char *name,
+			void *value, size_t size, int type);
+int posix_acl_xattr_set(struct dentry *dentry, const char *name,
+			const void *value, size_t size, int flags, int type);
+size_t posix_acl_xattr_list(struct dentry *dentry, char *list,
+			    size_t list_size, const char *name,
+			    size_t name_len, int type);
+
 extern const struct xattr_handler posix_acl_access_xattr_handler;
 extern const struct xattr_handler posix_acl_default_xattr_handler;