Re: Phantom ACL-related xattrs on 3.14.4 NFS client
From: Philippe Troin
Date: Tue Jun 10 2014 - 17:20:14 EST
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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/