Re: sysfs: Do not return POSIX ACL xattrs via listxattr()

[Marc AurÃle La France]

On Mon, 10 Sep 2018, Marc AurÃle La France wrote:
> On Mon, 10 Sep 2018, Andreas GrÃnbacher wrote:
> > Am Mo., 10. Sep. 2018 schrieb Marc AurÃle La France:

> > > Commit 786534b92f3ce68f4afc8a761c80b76887797b0a "tmpfs: listxattr
> > > should include POSIX ACL xattrs", which first appeared in 4.5 kernels,
> > > introduced a regression whereby listxattr() syscalls on anything in
> > > sysfs, or its mountpoint, return the name of the two POSIX ACL xattrs,
> > > but attempts to retrieve these values are denied with EOPNOTSUP.  For
> > > example ...

> > >         # getfattr -d --match=- /sys
> > >         /sys: system.posix_acl_access: Operation not supported
> > >         /sys: system.posix_acl_default: Operation not supported
> > >         #

> > I can confirm this regression.

> > > This inconsistent behaviour confuses rsync(1) (among others) when it
> > > runs into a sysfs mountpoint, even when told to not descend into it.
> > > This issue occurs because simple_xattr_list() does not correctly deal
> > > with cached ACLs.

> > > The suggested fix below was developed with a 4.18.7 kernel, but should
> > > apply, modulo patch fuzz, to any kernel >= 4.7.  A fix for 4.5 <=
> > > kernels < 4.7 is trivially different, but I won't bother given such
> > > kernels are no longer maintained.

> > > Note that the only other simple_xattr_list() caller, shmem, avoids
> > > this glitch by previously calling cache_no_acl() on all inodes it
> > > creates, so perhaps sysfs/kernfs should do the same.

> > > Signed-off-by: Marc AurÃle La France <tsi@xxxxxxxxxx>

> > > --- a/fs/xattr.c
> > > +++ b/fs/xattr.c
> > > @@ -949,13 +949,13 @@ ssize_t simple_xattr_list(struct inode *inode,
> > >         int err = 0;
> > >
> > >  #ifdef CONFIG_FS_POSIX_ACL
> > > -       if (inode->i_acl) {
> > > +       if (inode->i_acl && !is_uncached_acl(inode->i_acl)) {
> > >                 err = xattr_list_one(&buffer, &remaining_size,
> > >                                      XATTR_NAME_POSIX_ACL_ACCESS);
> > >                 if (err)
> > >                         return err;
> > >         }
> > > -       if (inode->i_default_acl) {
> > > +       if (inode->i_default_acl && 
> > > !is_uncached_acl(inode->i_default_acl)) {
> > >                 err = xattr_list_one(&buffer, &remaining_size,
> > >                                      XATTR_NAME_POSIX_ACL_DEFAULT);
> > >                 if (err)

> > This seems to be a better fix, but I haven't fully verified it, yet:

> > --- a/fs/inode.c
> > +++ b/fs/inode.c
> > @@ -187,7 +187,8 @@ int inode_init_always(struct super_block *sb, struct
> >     inode->i_mapping = mapping;
> >     INIT_HLIST_HEAD(&inode->i_dentry);    /* buggered by rcu freeing */
> >  #ifdef CONFIG_FS_POSIX_ACL
> > -    inode->i_acl = inode->i_default_acl = ACL_NOT_CACHED;
> > +    inode->i_acl = inode->i_default_acl =
> > +           (sb->s_flags & SB_POSIXACL) ? ACL_NOT_CACHED : NULL;
> >  #endif
> >
> >  #ifdef CONFIG_FSNOTIFY

> Yes, that works too, and doesn't seem to cause other issues.

> Tested-by: Marc AurÃle La France <tsi@xxxxxxxxxx>

Anything more on this?

Thanks and have a great day.

Marc.