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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
