NFS client broken in Linus' tip

From: Trond Myklebust <hidden>
Date: 2014-02-03 20:22:21
Also in: linux-nfs, lkml
Subsystem: filesystems (vfs and infrastructure), nfs, sunrpc, and lockd clients, the rest · Maintainers: Alexander Viro, Christian Brauner, Trond Myklebust, Anna Schumaker, Linus Torvalds

On Mon, 2014-02-03 at 10:45 -0500, Trond Myklebust wrote:

On Feb 3, 2014, at 9:57, Christoph Hellwig [off-list ref] wrote:

quoted

On Mon, Feb 03, 2014 at 09:17:30AM -0500, Trond Myklebust wrote:

quoted

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.

quoted

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

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;

-- 
1.8.5.3


-- 
Trond Myklebust
Linux NFS client maintainer

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help