Return-Path: linux-nfs-owner@vger.kernel.org Received: from fieldses.org ([174.143.236.118]:60687 "EHLO fieldses.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933421AbaFIOqw (ORCPT ); Mon, 9 Jun 2014 10:46:52 -0400 Date: Mon, 9 Jun 2014 10:46:45 -0400 To: Philippe Troin Cc: Christoph Hellwig , Trond Myklebust , Linux NFS Mailing List , Linux Kernel mailing list Subject: Re: Phantom ACL-related xattrs on 3.14.4 NFS client Message-ID: <20140609144645.GB8390@fieldses.org> References: <1402082953.28220.7.camel@ceramic.home.fifi.org> <20140607140414.GA26534@infradead.org> <1402195701.12743.18.camel@ceramic.home.fifi.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: <1402195701.12743.18.camel@ceramic.home.fifi.org> From: "J. Bruce Fields" Sender: linux-nfs-owner@vger.kernel.org List-ID: 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