Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 17 Sep 2018 08:01:30 -0600 (Mountain Daylight Time)
From:   =?UTF-8?Q?Marc_Aur=C3=A8le_La_France?= <tsi@tuyoix.net>
To:     =?UTF-8?Q?Andreas_Gr=C3=BCnbacher?= <andreas.gruenbacher@gmail.com>
cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Tejun Heo <tj@kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Al Viro <viro@zeniv.linux.org.uk>,
        Linux FS-devel Mailing List <linux-fsdevel@vger.kernel.org>,
        Andreas Gruenbacher <agruenba@redhat.com>
Subject: Re: sysfs: Do not return POSIX ACL xattrs via listxattr()
In-Reply-To: <alpine.WNT.2.20.1809101709260.3100@CLUIJ>
Message-ID: <alpine.WNT.2.20.1809170801030.4228@CLUIJ>
References: <alpine.LNX.2.20.1809091544070.15534@fanir.tuyoix.net> <CAHpGcMJ9jB+Ekfvf6HUvWUr8ipoSJkS+U5arShpMwyjuc4qftg@mail.gmail.com> <alpine.WNT.2.20.1809101709260.3100@CLUIJ>
User-Agent: Alpine 2.20 (WNT 67 2015-01-07)
MIME-Version: 1.0
Content-Type: multipart/mixed; BOUNDARY="230644-14400-1537192892=:4228"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--230644-14400-1537192892=:4228
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8BIT

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@tuyoix.net>

>>> --- 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@tuyoix.net>

Anything more on this?

Thanks and have a great day.

Marc.
--230644-14400-1537192892=:4228--