| Message ID | 20140607140414.GA26534@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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,
--
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
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 -- 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
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. -- 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
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. -- 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
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? -- 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
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. -- 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
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,