Phantom ACL-related xattrs on 3.14.4 NFS client

Phantom ACL-related xattrs on 3.14.4 NFS client

[Philippe Troin]

This happens on an NFS client running on:
Linux ceramic32 3.14.4 #1 SMP Fri May 30 00:52:07 PDT 2014 i686 i686 i386 GNU/Linux
(also happens on x86_64).

The NFS server can be either 3.14 or 3.13, it doesn't change a thing.

Mount options are:
(from /proc/mtab)
ceramic:/export/home/phil /home/phil nfs rw,nosuid,nodev,relatime,vers=3,rsize=1048576,wsize=1048576,namlen=255,hard,proto=tcp,timeo=600,retrans=2,sec=sys,mountaddr=172.17.1.2,mountvers=3,mountport=20048,mountproto=tcp,local_lock=none,addr=172.17.1.2 0 0
(This is NFSv3)

The symptom:

Run getfacl on any NFS inode.  See there are no ACLs:

        % getfacl .
        # file: .
        # owner: phil
        # group: phil
        user::rwx
        group::r-x
        other::r-x

Yet, getfattr says there are some acl-related xattrs:

        % getfattr -m '.*' .
        # file: .
        system.posix_acl_access
        system.posix_acl_default

But when you want to retrieve these phantom xattrs, I get errors:

        % getfattr -n system.posix_acl_access . 
        .: system.posix_acl_access: No such attribute
        [1]    1136 exit 1     getfattr -n system.posix_acl_access .
        % getfattr -n system.posix_acl_default .
        .: system.posix_acl_default: No such attribute
        [1]    1146 exit 1     getfattr -n system.posix_acl_default .
        
I've noticed because it breaks the patch utility.

This is a regression from 3.13, probably due to the 3.14 NFS ACL overhaul.

I'm ready & willing to try patches.

Phil.

Re: Phantom ACL-related xattrs on 3.14.4 NFS client

[Trond Myklebust]

On Fri, Jun 6, 2014 at 3:29 PM, Philippe Troin <phil@coldcreektech.com> wrote:
> This happens on an NFS client running on:
> Linux ceramic32 3.14.4 #1 SMP Fri May 30 00:52:07 PDT 2014 i686 i686 i386 GNU/Linux
> (also happens on x86_64).
>
> The NFS server can be either 3.14 or 3.13, it doesn't change a thing.
>
> Mount options are:
> (from /proc/mtab)
> ceramic:/export/home/phil /home/phil nfs rw,nosuid,nodev,relatime,vers=3,rsize=1048576,wsize=1048576,namlen=255,hard,proto=tcp,timeo=600,retrans=2,sec=sys,mountaddr=172.17.1.2,mountvers=3,mountport=20048,mountproto=tcp,local_lock=none,addr=172.17.1.2 0 0
> (This is NFSv3)
>
> The symptom:
>
> Run getfacl on any NFS inode.  See there are no ACLs:
>
>         % getfacl .
>         # file: .
>         # owner: phil
>         # group: phil
>         user::rwx
>         group::r-x
>         other::r-x
>
> Yet, getfattr says there are some acl-related xattrs:
>
>         % getfattr -m '.*' .
>         # file: .
>         system.posix_acl_access
>         system.posix_acl_default
>
> But when you want to retrieve these phantom xattrs, I get errors:
>
>         % getfattr -n system.posix_acl_access .
>         .: system.posix_acl_access: No such attribute
>         [1]    1136 exit 1     getfattr -n system.posix_acl_access .
>         % getfattr -n system.posix_acl_default .
>         .: system.posix_acl_default: No such attribute
>         [1]    1146 exit 1     getfattr -n system.posix_acl_default .
>
> I've noticed because it breaks the patch utility.
>
> This is a regression from 3.13, probably due to the 3.14 NFS ACL overhaul.
>

Christoph, what is the intended interface for telling
posix_acl_xattr_list() that there are no acls on a particular file?
Should there perhaps be a call to get_acl()?

-- 
Trond Myklebust

Linux NFS client maintainer, PrimaryData

trond.myklebust@primarydata.com

Re: Phantom ACL-related xattrs on 3.14.4 NFS client

[Christoph Hellwig]

On Fri, Jun 06, 2014 at 04:37:03PM -0400, Trond Myklebust wrote:
> Christoph, what is the intended interface for telling
> posix_acl_xattr_list() that there are no acls on a particular file?
> Should there perhaps be a call to get_acl()?

The interface is to not call posix_acl_xattr_list unless you have ACLs.
Every implementation does this, except for generic_listxattr which is
only used by NFS.

Philippe, can you test the patch below?


diff --git a/fs/nfs/nfs3acl.c b/fs/nfs/nfs3acl.c
index 871d6ed..e083827 100644
--- a/fs/nfs/nfs3acl.c
+++ b/fs/nfs/nfs3acl.c
@@ -247,3 +247,45 @@ const struct xattr_handler *nfs3_xattr_handlers[] = {
 	&posix_acl_default_xattr_handler,
 	NULL,
 };
+
+static int
+nfs3_list_one_acl(struct inode *inode, int type, const char *name, void *data,
+		size_t size, ssize_t *result)
+{
+	struct posix_acl *acl;
+	char *p = data + *result;
+
+	acl = get_acl(inode, type);
+	if (!acl)
+		return 0;
+
+	posix_acl_release(acl);
+
+	*result += strlen(name);
+	if (!size)
+		return 0;
+	if (*result > size)
+		return -ERANGE;
+
+	strcpy(p, name);
+	return 0;
+}
+
+ssize_t
+nfs3_listxattr(struct dentry *dentry, char *data, size_t size)
+{
+	struct inode *inode = dentry->d_inode;
+	ssize_t result = 0;
+	int error;
+
+	error = nfs3_list_one_acl(inode, ACL_TYPE_ACCESS,
+			POSIX_ACL_XATTR_ACCESS, data, size, &result);
+	if (error)
+		return error;
+
+	error = nfs3_list_one_acl(inode, ACL_TYPE_DEFAULT,
+			POSIX_ACL_XATTR_DEFAULT, data, size, &result);
+	if (error)
+		return error;
+	return result;
+}
diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
index db60149..0e2bb26 100644
--- a/fs/nfs/nfs3proc.c
+++ b/fs/nfs/nfs3proc.c
@@ -891,7 +891,7 @@ static const struct inode_operations nfs3_dir_inode_operations = {
 	.getattr	= nfs_getattr,
 	.setattr	= nfs_setattr,
 #ifdef CONFIG_NFS_V3_ACL
-	.listxattr	= generic_listxattr,
+	.listxattr	= nfs3_listxattr,
 	.getxattr	= generic_getxattr,
 	.setxattr	= generic_setxattr,
 	.removexattr	= generic_removexattr,
@@ -905,7 +905,7 @@ static const struct inode_operations nfs3_file_inode_operations = {
 	.getattr	= nfs_getattr,
 	.setattr	= nfs_setattr,
 #ifdef CONFIG_NFS_V3_ACL
-	.listxattr	= generic_listxattr,
+	.listxattr	= nfs3_listxattr,
 	.getxattr	= generic_getxattr,
 	.setxattr	= generic_setxattr,
 	.removexattr	= generic_removexattr,

Re: Phantom ACL-related xattrs on 3.14.4 NFS client

[Philippe Troin]

Hi Trond & Christoph,

It's still broken, but in a different way.
The phantom attrs are gone, but the attr/acl interaction is still
uncertain.

I have tested vanilla  3.14.5 + this patch on x86_64.
Mount options are the same as last time (NFSv3).

This is what I see on the client:

        nfsv3client% mkdir x
        nfsv3client% cd x
        nfsv3client% getfattr -m '.*' .
        nfsv3client% getfacl .
        # file: .
        # owner: phil
        # group: phil
        user::rwx
        group::rwx
        other::r-x

OK so far: no more phantom attrs.
This is where things get dodgy:
        
        nfsv3client% setfacl -m u:root:r .   
        nfsv3client% getfacl .
        # file: .
        # owner: phil
        # group: phil
        user::rwx
        user:root:r--
        group::rwx
        mask::rwx
        other::r-x
        
        nfsv3client% getfattr -m '.*' .
        [1]    2123 segmentation fault  getfattr -m '.*' .
        % strace getfattr -m '.*' . 2>&1 | tail -n 20
        fstat(3, {st_mode=S_IFREG|0644, st_size=26254, ...}) = 0
        mmap(NULL, 26254, PROT_READ, MAP_SHARED, 3, 0) = 0x7f46a1450000
        close(3)                                = 0
        getrlimit(RLIMIT_NOFILE, {rlim_cur=1024, rlim_max=4*1024}) = 0
        lstat(".", {st_mode=S_IFDIR|0775, st_size=4096, ...}) = 0
        listxattr(".", NULL, 0)                 = 23
        listxattr(".", "system.posix_acl_access", 256) = 23
        brk(0)                                  = 0x1138000
        brk(0x1178000)                          = 0x1178000
        brk(0)                                  = 0x1178000
        brk(0)                                  = 0x1178000
        brk(0x1159000)                          = 0x1159000
        brk(0)                                  = 0x1159000
        mmap(NULL, 266240, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f46a140f000
        brk(0)                                  = 0x1159000
        brk(0)                                  = 0x1159000
        brk(0x1139000)                          = 0x1139000
        brk(0)                                  = 0x1139000
        --- SIGSEGV {si_signo=SIGSEGV, si_code=SEGV_MAPERR, si_addr=0x11586e8} ---
        +++ killed by SIGSEGV +++
        [1]    2311 segmentation fault  strace getfattr -m '.*' . 2>&1
        | 
               2312 done                tail -n 20

I have no idea get getfattr crashes right after the listxattr() syscall,
but it surely doesn't on the NFSv3 server nor with 3.13.
A quick check on the NFS server shows the the ACLs are correctly set:

        nfsv3server% cd /path/to/x
        nfsv3server% getfacl .
        # file: .
        # owner: phil
        # group: phil
        user::rwx
        user:root:r--
        group::rwx
        mask::rwx
        other::r-x
        
        nfsv3server% getfattr -m '.*' .
        # file: .
        system.posix_acl_access

Back on the client, clearing the ACL confuses the client further:

        nfsv3client% setfacl -b .                               
        nfsv3client% getfacl .                                  
        # file: .
        # owner: phil
        # group: phil
        user::rwx
        group::rwx
        other::r-x
        
        nfsv3client% strace getfattr -m '.*' . 2>&1 | tail -n 20
        fstat(3, {st_mode=S_IFREG|0644, st_size=26254, ...}) = 0
        mmap(NULL, 26254, PROT_READ, MAP_SHARED, 3, 0) = 0x7fc7e3f9a000
        close(3)                                = 0
        getrlimit(RLIMIT_NOFILE, {rlim_cur=1024, rlim_max=4*1024}) = 0
        lstat(".", {st_mode=S_IFDIR|0775, st_size=4096, ...}) = 0
        listxattr(".", NULL, 0)                 = 23
        listxattr(".", "system.posix_acl_access", 256) = 23
        brk(0)                                  = 0x1655000
        brk(0x1695000)                          = 0x1695000
        brk(0)                                  = 0x1695000
        brk(0)                                  = 0x1695000
        brk(0x1676000)                          = 0x1676000
        brk(0)                                  = 0x1676000
        mmap(NULL, 266240, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7fc7e3f59000
        brk(0)                                  = 0x1676000
        brk(0)                                  = 0x1676000
        brk(0x1656000)                          = 0x1656000
        brk(0)                                  = 0x1656000
        --- SIGSEGV {si_signo=SIGSEGV, si_code=SEGV_MAPERR, si_addr=0x16756e8} ---
        +++ killed by SIGSEGV +++
        [1]    2353 segmentation fault  strace getfattr -m '.*' . 2>&1
        | 
               2354 done                tail -n 20
        nfsv3client% getfattr -n system.posix_acl_access .
        # file: .
        system.posix_acl_access=0sAgAAAAEABwD/////BAAHAP////8gAAUA/////w==

See how:
      * getfacl says there's no ACLs
      * getfattr says there's still a system.posix_acl_access attr.
Interestingly, the server says otherwise:

        nfsv3server% getfattr -m '.*' .
        nfsv3server% getfattr -n system.posix_acl_access .
        .: system.posix_acl_access: No such attribute
        [1]    2233 exit 1     getfattr -n system.posix_acl_access .
        nfsv3server% getfacl  .                          
        # file: .
        # owner: phil
        # group: phil
        user::rwx
        group::rwx
        other::r-x

Phil.

On Sat, 2014-06-07 at 07:04 -0700, Christoph Hellwig wrote:
> On Fri, Jun 06, 2014 at 04:37:03PM -0400, Trond Myklebust wrote:
> > Christoph, what is the intended interface for telling
> > posix_acl_xattr_list() that there are no acls on a particular file?
> > Should there perhaps be a call to get_acl()?
> 
> The interface is to not call posix_acl_xattr_list unless you have ACLs.
> Every implementation does this, except for generic_listxattr which is
> only used by NFS.
> 
> Philippe, can you test the patch below?
> 
> 
> diff --git a/fs/nfs/nfs3acl.c b/fs/nfs/nfs3acl.c
> index 871d6ed..e083827 100644
> --- a/fs/nfs/nfs3acl.c
> +++ b/fs/nfs/nfs3acl.c
> @@ -247,3 +247,45 @@ const struct xattr_handler *nfs3_xattr_handlers[] = {
>  	&posix_acl_default_xattr_handler,
>  	NULL,
>  };
> +
> +static int
> +nfs3_list_one_acl(struct inode *inode, int type, const char *name, void *data,
> +		size_t size, ssize_t *result)
> +{
> +	struct posix_acl *acl;
> +	char *p = data + *result;
> +
> +	acl = get_acl(inode, type);
> +	if (!acl)
> +		return 0;
> +
> +	posix_acl_release(acl);
> +
> +	*result += strlen(name);
> +	if (!size)
> +		return 0;
> +	if (*result > size)
> +		return -ERANGE;
> +
> +	strcpy(p, name);
> +	return 0;
> +}
> +
> +ssize_t
> +nfs3_listxattr(struct dentry *dentry, char *data, size_t size)
> +{
> +	struct inode *inode = dentry->d_inode;
> +	ssize_t result = 0;
> +	int error;
> +
> +	error = nfs3_list_one_acl(inode, ACL_TYPE_ACCESS,
> +			POSIX_ACL_XATTR_ACCESS, data, size, &result);
> +	if (error)
> +		return error;
> +
> +	error = nfs3_list_one_acl(inode, ACL_TYPE_DEFAULT,
> +			POSIX_ACL_XATTR_DEFAULT, data, size, &result);
> +	if (error)
> +		return error;
> +	return result;
> +}
> diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
> index db60149..0e2bb26 100644
> --- a/fs/nfs/nfs3proc.c
> +++ b/fs/nfs/nfs3proc.c
> @@ -891,7 +891,7 @@ static const struct inode_operations nfs3_dir_inode_operations = {
>  	.getattr	= nfs_getattr,
>  	.setattr	= nfs_setattr,
>  #ifdef CONFIG_NFS_V3_ACL
> -	.listxattr	= generic_listxattr,
> +	.listxattr	= nfs3_listxattr,
>  	.getxattr	= generic_getxattr,
>  	.setxattr	= generic_setxattr,
>  	.removexattr	= generic_removexattr,
> @@ -905,7 +905,7 @@ static const struct inode_operations nfs3_file_inode_operations = {
>  	.getattr	= nfs_getattr,
>  	.setattr	= nfs_setattr,
>  #ifdef CONFIG_NFS_V3_ACL
> -	.listxattr	= generic_listxattr,
> +	.listxattr	= nfs3_listxattr,
>  	.getxattr	= generic_getxattr,
>  	.setxattr	= generic_setxattr,
>  	.removexattr	= generic_removexattr,

Re: Phantom ACL-related xattrs on 3.14.4 NFS client

[J. Bruce Fields]

On Sat, Jun 07, 2014 at 07:48:21PM -0700, Philippe Troin wrote:
> Hi Trond & Christoph,
> 
> It's still broken, but in a different way.
> The phantom attrs are gone, but the attr/acl interaction is still
> uncertain.
> 
> I have tested vanilla  3.14.5 + this patch on x86_64.
> Mount options are the same as last time (NFSv3).
> 
> This is what I see on the client:
> 
>         nfsv3client% mkdir x
>         nfsv3client% cd x
>         nfsv3client% getfattr -m '.*' .
>         nfsv3client% getfacl .
>         # file: .
>         # owner: phil
>         # group: phil
>         user::rwx
>         group::rwx
>         other::r-x
> 
> OK so far: no more phantom attrs.
> This is where things get dodgy:
>         
>         nfsv3client% setfacl -m u:root:r .   
>         nfsv3client% getfacl .
>         # file: .
>         # owner: phil
>         # group: phil
>         user::rwx
>         user:root:r--
>         group::rwx
>         mask::rwx
>         other::r-x
>         
>         nfsv3client% getfattr -m '.*' .
>         [1]    2123 segmentation fault  getfattr -m '.*' .

Is there a backtrace or anything in the system logs?

--b.

>         % strace getfattr -m '.*' . 2>&1 | tail -n 20
>         fstat(3, {st_mode=S_IFREG|0644, st_size=26254, ...}) = 0
>         mmap(NULL, 26254, PROT_READ, MAP_SHARED, 3, 0) = 0x7f46a1450000
>         close(3)                                = 0
>         getrlimit(RLIMIT_NOFILE, {rlim_cur=1024, rlim_max=4*1024}) = 0
>         lstat(".", {st_mode=S_IFDIR|0775, st_size=4096, ...}) = 0
>         listxattr(".", NULL, 0)                 = 23
>         listxattr(".", "system.posix_acl_access", 256) = 23
>         brk(0)                                  = 0x1138000
>         brk(0x1178000)                          = 0x1178000
>         brk(0)                                  = 0x1178000
>         brk(0)                                  = 0x1178000
>         brk(0x1159000)                          = 0x1159000
>         brk(0)                                  = 0x1159000
>         mmap(NULL, 266240, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f46a140f000
>         brk(0)                                  = 0x1159000
>         brk(0)                                  = 0x1159000
>         brk(0x1139000)                          = 0x1139000
>         brk(0)                                  = 0x1139000
>         --- SIGSEGV {si_signo=SIGSEGV, si_code=SEGV_MAPERR, si_addr=0x11586e8} ---
>         +++ killed by SIGSEGV +++
>         [1]    2311 segmentation fault  strace getfattr -m '.*' . 2>&1
>         | 
>                2312 done                tail -n 20
> 
> I have no idea get getfattr crashes right after the listxattr() syscall,
> but it surely doesn't on the NFSv3 server nor with 3.13.
> A quick check on the NFS server shows the the ACLs are correctly set:
> 
>         nfsv3server% cd /path/to/x
>         nfsv3server% getfacl .
>         # file: .
>         # owner: phil
>         # group: phil
>         user::rwx
>         user:root:r--
>         group::rwx
>         mask::rwx
>         other::r-x
>         
>         nfsv3server% getfattr -m '.*' .
>         # file: .
>         system.posix_acl_access
> 
> Back on the client, clearing the ACL confuses the client further:
> 
>         nfsv3client% setfacl -b .                               
>         nfsv3client% getfacl .                                  
>         # file: .
>         # owner: phil
>         # group: phil
>         user::rwx
>         group::rwx
>         other::r-x
>         
>         nfsv3client% strace getfattr -m '.*' . 2>&1 | tail -n 20
>         fstat(3, {st_mode=S_IFREG|0644, st_size=26254, ...}) = 0
>         mmap(NULL, 26254, PROT_READ, MAP_SHARED, 3, 0) = 0x7fc7e3f9a000
>         close(3)                                = 0
>         getrlimit(RLIMIT_NOFILE, {rlim_cur=1024, rlim_max=4*1024}) = 0
>         lstat(".", {st_mode=S_IFDIR|0775, st_size=4096, ...}) = 0
>         listxattr(".", NULL, 0)                 = 23
>         listxattr(".", "system.posix_acl_access", 256) = 23
>         brk(0)                                  = 0x1655000
>         brk(0x1695000)                          = 0x1695000
>         brk(0)                                  = 0x1695000
>         brk(0)                                  = 0x1695000
>         brk(0x1676000)                          = 0x1676000
>         brk(0)                                  = 0x1676000
>         mmap(NULL, 266240, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7fc7e3f59000
>         brk(0)                                  = 0x1676000
>         brk(0)                                  = 0x1676000
>         brk(0x1656000)                          = 0x1656000
>         brk(0)                                  = 0x1656000
>         --- SIGSEGV {si_signo=SIGSEGV, si_code=SEGV_MAPERR, si_addr=0x16756e8} ---
>         +++ killed by SIGSEGV +++
>         [1]    2353 segmentation fault  strace getfattr -m '.*' . 2>&1
>         | 
>                2354 done                tail -n 20
>         nfsv3client% getfattr -n system.posix_acl_access .
>         # file: .
>         system.posix_acl_access=0sAgAAAAEABwD/////BAAHAP////8gAAUA/////w==
> 
> See how:
>       * getfacl says there's no ACLs
>       * getfattr says there's still a system.posix_acl_access attr.
> Interestingly, the server says otherwise:
> 
>         nfsv3server% getfattr -m '.*' .
>         nfsv3server% getfattr -n system.posix_acl_access .
>         .: system.posix_acl_access: No such attribute
>         [1]    2233 exit 1     getfattr -n system.posix_acl_access .
>         nfsv3server% getfacl  .                          
>         # file: .
>         # owner: phil
>         # group: phil
>         user::rwx
>         group::rwx
>         other::r-x
> 
> Phil.
> 
> On Sat, 2014-06-07 at 07:04 -0700, Christoph Hellwig wrote:
> > On Fri, Jun 06, 2014 at 04:37:03PM -0400, Trond Myklebust wrote:
> > > Christoph, what is the intended interface for telling
> > > posix_acl_xattr_list() that there are no acls on a particular file?
> > > Should there perhaps be a call to get_acl()?
> > 
> > The interface is to not call posix_acl_xattr_list unless you have ACLs.
> > Every implementation does this, except for generic_listxattr which is
> > only used by NFS.
> > 
> > Philippe, can you test the patch below?
> > 
> > 
> > diff --git a/fs/nfs/nfs3acl.c b/fs/nfs/nfs3acl.c
> > index 871d6ed..e083827 100644
> > --- a/fs/nfs/nfs3acl.c
> > +++ b/fs/nfs/nfs3acl.c
> > @@ -247,3 +247,45 @@ const struct xattr_handler *nfs3_xattr_handlers[] = {
> >  	&posix_acl_default_xattr_handler,
> >  	NULL,
> >  };
> > +
> > +static int
> > +nfs3_list_one_acl(struct inode *inode, int type, const char *name, void *data,
> > +		size_t size, ssize_t *result)
> > +{
> > +	struct posix_acl *acl;
> > +	char *p = data + *result;
> > +
> > +	acl = get_acl(inode, type);
> > +	if (!acl)
> > +		return 0;
> > +
> > +	posix_acl_release(acl);
> > +
> > +	*result += strlen(name);
> > +	if (!size)
> > +		return 0;
> > +	if (*result > size)
> > +		return -ERANGE;
> > +
> > +	strcpy(p, name);
> > +	return 0;
> > +}
> > +
> > +ssize_t
> > +nfs3_listxattr(struct dentry *dentry, char *data, size_t size)
> > +{
> > +	struct inode *inode = dentry->d_inode;
> > +	ssize_t result = 0;
> > +	int error;
> > +
> > +	error = nfs3_list_one_acl(inode, ACL_TYPE_ACCESS,
> > +			POSIX_ACL_XATTR_ACCESS, data, size, &result);
> > +	if (error)
> > +		return error;
> > +
> > +	error = nfs3_list_one_acl(inode, ACL_TYPE_DEFAULT,
> > +			POSIX_ACL_XATTR_DEFAULT, data, size, &result);
> > +	if (error)
> > +		return error;
> > +	return result;
> > +}
> > diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
> > index db60149..0e2bb26 100644
> > --- a/fs/nfs/nfs3proc.c
> > +++ b/fs/nfs/nfs3proc.c
> > @@ -891,7 +891,7 @@ static const struct inode_operations nfs3_dir_inode_operations = {
> >  	.getattr	= nfs_getattr,
> >  	.setattr	= nfs_setattr,
> >  #ifdef CONFIG_NFS_V3_ACL
> > -	.listxattr	= generic_listxattr,
> > +	.listxattr	= nfs3_listxattr,
> >  	.getxattr	= generic_getxattr,
> >  	.setxattr	= generic_setxattr,
> >  	.removexattr	= generic_removexattr,
> > @@ -905,7 +905,7 @@ static const struct inode_operations nfs3_file_inode_operations = {
> >  	.getattr	= nfs_getattr,
> >  	.setattr	= nfs_setattr,
> >  #ifdef CONFIG_NFS_V3_ACL
> > -	.listxattr	= generic_listxattr,
> > +	.listxattr	= nfs3_listxattr,
> >  	.getxattr	= generic_getxattr,
> >  	.setxattr	= generic_setxattr,
> >  	.removexattr	= generic_removexattr,
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Phantom ACL-related xattrs on 3.14.4 NFS client

[Philippe Troin]

On Mon, 2014-06-09 at 10:46 -0400, J. Bruce Fields wrote:
> On Sat, Jun 07, 2014 at 07:48:21PM -0700, Philippe Troin wrote:
> > Hi Trond & Christoph,
> > 
> > It's still broken, but in a different way.
> > The phantom attrs are gone, but the attr/acl interaction is still
> > uncertain.
> > 
> > I have tested vanilla  3.14.5 + this patch on x86_64.
> > Mount options are the same as last time (NFSv3).
> > 
> > This is what I see on the client:
> > 
> >         nfsv3client% mkdir x
> >         nfsv3client% cd x
> >         nfsv3client% getfattr -m '.*' .
> >         nfsv3client% getfacl .
> >         # file: .
> >         # owner: phil
> >         # group: phil
> >         user::rwx
> >         group::rwx
> >         other::r-x
> > 
> > OK so far: no more phantom attrs.
> > This is where things get dodgy:
> >         
> >         nfsv3client% setfacl -m u:root:r .   
> >         nfsv3client% getfacl .
> >         # file: .
> >         # owner: phil
> >         # group: phil
> >         user::rwx
> >         user:root:r--
> >         group::rwx
> >         mask::rwx
> >         other::r-x
> >         
> >         nfsv3client% getfattr -m '.*' .
> >         [1]    2123 segmentation fault  getfattr -m '.*' .
> 
> Is there a backtrace or anything in the system logs?

No, nothing but the SIGSEGV getting logged in dmesg.

Since I've tested on 3.14.5, 3.14.6 came out, and contains NFSd related
patches that look to address further ACL issues.  I'm going to be trying
that out.

Phil.

Re: Phantom ACL-related xattrs on 3.14.4 NFS client

[Philippe Troin]

Trond, Christoph,

Since my last email, I've been testing 3.14.6.
Stock 3.14.6 is still broken, and Christoph's patch does help, but does
not entirely cure the problem.

On Sat, 2014-06-07 at 19:48 -0700, Philippe Troin wrote:
> It's still broken, but in a different way.
> The phantom attrs are gone, but the attr/acl interaction is still
> uncertain.
> 
> I have tested vanilla  3.14.5 + this patch on x86_64.
> Mount options are the same as last time (NFSv3).
> 
> This is what I see on the client:
> 
>         nfsv3client% mkdir x
>         nfsv3client% cd x
>         nfsv3client% getfattr -m '.*' .
>         nfsv3client% getfacl .
>         # file: .
>         # owner: phil
>         # group: phil
>         user::rwx
>         group::rwx
>         other::r-x
> 
> OK so far: no more phantom attrs.
> This is where things get dodgy:
>         
>         nfsv3client% setfacl -m u:root:r .   
>         nfsv3client% getfacl .
>         # file: .
>         # owner: phil
>         # group: phil
>         user::rwx
>         user:root:r--
>         group::rwx
>         mask::rwx
>         other::r-x
>         
>         nfsv3client% getfattr -m '.*' .
>         [1]    2123 segmentation fault  getfattr -m '.*' .
>         % strace getfattr -m '.*' . 2>&1 | tail -n 20
>         fstat(3, {st_mode=S_IFREG|0644, st_size=26254, ...}) = 0
>         mmap(NULL, 26254, PROT_READ, MAP_SHARED, 3, 0) = 0x7f46a1450000
>         close(3)                                = 0
>         getrlimit(RLIMIT_NOFILE, {rlim_cur=1024, rlim_max=4*1024}) = 0
>         lstat(".", {st_mode=S_IFDIR|0775, st_size=4096, ...}) = 0
>         listxattr(".", NULL, 0)                 = 23
>         listxattr(".", "system.posix_acl_access", 256) = 23
>         brk(0)                                  = 0x1138000
>         brk(0x1178000)                          = 0x1178000
>         brk(0)                                  = 0x1178000
>         brk(0)                                  = 0x1178000
>         brk(0x1159000)                          = 0x1159000
>         brk(0)                                  = 0x1159000
>         mmap(NULL, 266240, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f46a140f000
>         brk(0)                                  = 0x1159000
>         brk(0)                                  = 0x1159000
>         brk(0x1139000)                          = 0x1139000
>         brk(0)                                  = 0x1139000
>         --- SIGSEGV {si_signo=SIGSEGV, si_code=SEGV_MAPERR, si_addr=0x11586e8} ---
>         +++ killed by SIGSEGV +++
>         [1]    2311 segmentation fault  strace getfattr -m '.*' . 2>&1
>         | 
>                2312 done                tail -n 20

I have since discovered that getfattr crashes because on an NFSv3 mount,
listxattr() does not NULL terminate the attribute strings.

Compare a broken 3.14.6:
        listxattr(".", NULL, 0)                 = 23
        listxattr(".", "system.posix_acl_access", 256) = 23
vs a working 3.13:
        listxattr(".", NULL, 0)      = 24
        listxattr(".", "system.posix_acl_access\0", 256) = 24

The above behavior happens with or without Christoph's patch.

Also, with Christoph's patch applied:

> On Sat, 2014-06-07 at 07:04 -0700, Christoph Hellwig wrote:
> > On Fri, Jun 06, 2014 at 04:37:03PM -0400, Trond Myklebust wrote:
> > > Christoph, what is the intended interface for telling
> > > posix_acl_xattr_list() that there are no acls on a particular file?
> > > Should there perhaps be a call to get_acl()?
> > 
> > The interface is to not call posix_acl_xattr_list unless you have ACLs.
> > Every implementation does this, except for generic_listxattr which is
> > only used by NFS.
> > 
> > Philippe, can you test the patch below?
> > 
> > 
> > diff --git a/fs/nfs/nfs3acl.c b/fs/nfs/nfs3acl.c
> > index 871d6ed..e083827 100644
> > --- a/fs/nfs/nfs3acl.c
> > +++ b/fs/nfs/nfs3acl.c
> > @@ -247,3 +247,45 @@ const struct xattr_handler *nfs3_xattr_handlers[] = {
> >  	&posix_acl_default_xattr_handler,
> >  	NULL,
> >  };
> > +
> > +static int
> > +nfs3_list_one_acl(struct inode *inode, int type, const char *name, void *data,
> > +		size_t size, ssize_t *result)
> > +{
> > +	struct posix_acl *acl;
> > +	char *p = data + *result;
> > +
> > +	acl = get_acl(inode, type);
> > +	if (!acl)
> > +		return 0;
> > +
> > +	posix_acl_release(acl);
> > +
> > +	*result += strlen(name);
> > +	if (!size)
> > +		return 0;
> > +	if (*result > size)
> > +		return -ERANGE;
> > +
> > +	strcpy(p, name);
> > +	return 0;
> > +}
> > +
> > +ssize_t
> > +nfs3_listxattr(struct dentry *dentry, char *data, size_t size)
> > +{
> > +	struct inode *inode = dentry->d_inode;
> > +	ssize_t result = 0;
> > +	int error;
> > +
> > +	error = nfs3_list_one_acl(inode, ACL_TYPE_ACCESS,
> > +			POSIX_ACL_XATTR_ACCESS, data, size, &result);
> > +	if (error)
> > +		return error;
> > +
> > +	error = nfs3_list_one_acl(inode, ACL_TYPE_DEFAULT,
> > +			POSIX_ACL_XATTR_DEFAULT, data, size, &result);
> > +	if (error)
> > +		return error;
> > +	return result;
> > +}
> > diff --git a/fs/nfs/nfs3proc.c b/fs/nfs/nfs3proc.c
> > index db60149..0e2bb26 100644
> > --- a/fs/nfs/nfs3proc.c
> > +++ b/fs/nfs/nfs3proc.c
> > @@ -891,7 +891,7 @@ static const struct inode_operations nfs3_dir_inode_operations = {
> >  	.getattr	= nfs_getattr,
> >  	.setattr	= nfs_setattr,
> >  #ifdef CONFIG_NFS_V3_ACL
> > -	.listxattr	= generic_listxattr,
> > +	.listxattr	= nfs3_listxattr,
> >  	.getxattr	= generic_getxattr,
> >  	.setxattr	= generic_setxattr,
> >  	.removexattr	= generic_removexattr,
> > @@ -905,7 +905,7 @@ static const struct inode_operations nfs3_file_inode_operations = {
> >  	.getattr	= nfs_getattr,
> >  	.setattr	= nfs_setattr,
> >  #ifdef CONFIG_NFS_V3_ACL
> > -	.listxattr	= generic_listxattr,
> > +	.listxattr	= nfs3_listxattr,
> >  	.getxattr	= generic_getxattr,
> >  	.setxattr	= generic_setxattr,
> >  	.removexattr	= generic_removexattr,

Now, if a file has no ACLs, there are no phantom xattrs appearing
anymore.
However, if ACLs are created, then removed, the ACL xattrs will stick
after the ACL clearing.

For example:

  setfacl -m u:root:r .
  setfacl -b .
  getfattr -m '.*' .

will still show a system.posix_acl_access xattr.

Phil.

Re: Phantom ACL-related xattrs on 3.14.4 NFS client

[Christoph Hellwig]

On Tue, Jun 10, 2014 at 02:20:03PM -0700, Philippe Troin wrote:
> Trond, Christoph,
> 
> Since my last email, I've been testing 3.14.6.
> Stock 3.14.6 is still broken, and Christoph's patch does help, but does
> not entirely cure the problem.

Can you send me the output of 

getfattr -n system.posix_acl_access -e hex <file>

for the working case, and the current kernel with my previous patch?

Re: Phantom ACL-related xattrs on 3.14.4 NFS client

[Philippe Troin]

Christoph,

On Wed, 2014-06-11 at 00:24 -0700, Christoph Hellwig wrote:
> On Tue, Jun 10, 2014 at 02:20:03PM -0700, Philippe Troin wrote:
> > Trond, Christoph,
> > 
> > Since my last email, I've been testing 3.14.6.
> > Stock 3.14.6 is still broken, and Christoph's patch does help, but does
> > not entirely cure the problem.
> 
> Can you send me the output of 
> 
> getfattr -n system.posix_acl_access -e hex <file>
> 
> for the working case, and the current kernel with my previous patch?

Here's the output on the broken kernel (vanilla 3.14.6 + your patch):

        % mkdir x
        % cd x
        % getfacl .                                   
        # file: .
        # owner: phil
        # group: phil
        user::rwx
        group::rwx
        other::r-x
        
        % getfattr -e hex -n system.posix_acl_access .
        .: system.posix_acl_access: No such attribute
        [2]    1901 exit 1     getfattr -e hex -n system.posix_acl_access .
        % setfacl -m u:root:r .                       
        % getfacl .                                   
        # file: .
        # owner: phil
        # group: phil
        user::rwx
        user:root:r--
        group::rwx
        mask::rwx
        other::r-x
        
        % getfattr -e hex -n system.posix_acl_access .
        # file: .
        system.posix_acl_access=0x0200000001000700ffffffff020004000000000004000700ffffffff10000700ffffffff20000500ffffffff
        
        % setfacl -b .                                
        % getfacl .                                   
        # file: .
        # owner: phil
        # group: phil
        user::rwx
        group::rwx
        other::r-x
        
        % getfattr -e hex -n system.posix_acl_access .
        # file: .
        system.posix_acl_access=0x0200000001000700ffffffff04000700ffffffff20000500ffffffff
        
On a working system (3.13.11 + Fedora patches), the output is the same.
So there's no regression here between 3.13.11 and 3.14.6 + your patch.
I would argue that this behavior (system.posix_acl_access still present
after clear the ACLs with setfacl -b) is wrong, and in fact there are no
traces of this xattr on the server, but it's not new.
I had missed that this counter-intuitive behavior was already in earlier
kernels.  My apologies.
Trond, what's your take on that one?

So, the only regression remaining between 3.13.11 and 3.14.6 + your
patch is the one where listxattr(2) and friends do not NUL-terminate the
xattr names they return.  This is detailed in
<1402435203.24047.9.camel@ceramic.home.fifi.org> I sent yesterday.

Phil.

Re: Phantom ACL-related xattrs on 3.14.4 NFS client

[Christoph Hellwig]

On Wed, Jun 11, 2014 at 09:15:18AM -0700, Philippe Troin wrote:
> So, the only regression remaining between 3.13.11 and 3.14.6 + your
> patch is the one where listxattr(2) and friends do not NUL-terminate the
> xattr names they return.  This is detailed in
> <1402435203.24047.9.camel@ceramic.home.fifi.org> I sent yesterday.

Oh, that's a bug in my patch.  The following incremental patch should
fix it:

diff --git a/fs/nfs/nfs3acl.c b/fs/nfs/nfs3acl.c
index e083827..8f854dd 100644
--- a/fs/nfs/nfs3acl.c
+++ b/fs/nfs/nfs3acl.c
@@ -262,6 +262,7 @@ nfs3_list_one_acl(struct inode *inode, int type, const char *name, void *data,
 	posix_acl_release(acl);
 
 	*result += strlen(name);
+	*result += 1;
 	if (!size)
 		return 0;
 	if (*result > size)

Re: Phantom ACL-related xattrs on 3.14.4 NFS client

[Philippe Troin]

Hi Christopher,

On Wed, 2014-06-11 at 09:22 -0700, Christoph Hellwig wrote:
> On Wed, Jun 11, 2014 at 09:15:18AM -0700, Philippe Troin wrote:
> > So, the only regression remaining between 3.13.11 and 3.14.6 + your
> > patch is the one where listxattr(2) and friends do not NUL-terminate the
> > xattr names they return.  This is detailed in
> > <1402435203.24047.9.camel@ceramic.home.fifi.org> I sent yesterday.
> 
> Oh, that's a bug in my patch.  The following incremental patch should
> fix it:
> 
> diff --git a/fs/nfs/nfs3acl.c b/fs/nfs/nfs3acl.c
> index e083827..8f854dd 100644
> --- a/fs/nfs/nfs3acl.c
> +++ b/fs/nfs/nfs3acl.c
> @@ -262,6 +262,7 @@ nfs3_list_one_acl(struct inode *inode, int type, const char *name, void *data,
>  	posix_acl_release(acl);
>  
>  	*result += strlen(name);
> +	*result += 1;
>  	if (!size)
>  		return 0;
>  	if (*result > size)


I'm belatedly confirming that both patches applied together in
<20140607140414.GA26534@infradead.org> and
<20140611162238.GA28340@infradead.org> fix the problem I was seeing with
NFSv3 client mounts on 3.6.14.x.

I've tried vanilla 3.6.14.6 + both patches and the regression is gone.
Remains the issue where on a NFSv3 client.

Phil.