NFS client broken in Linus' tip

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(-)

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?

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.

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