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.
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
> > <[email protected]> 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
<[email protected]> and
<[email protected]> 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.
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,
On Fri, Jun 6, 2014 at 3:29 PM, Philippe Troin <[email protected]> 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
[email protected]
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
> <[email protected]> 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)
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?
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.
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 [email protected]
> 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
<[email protected]> I sent yesterday.
Phil.
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,
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.