On 1/14/21 12:24 PM, Brian Foster wrote:
> On Thu, Jan 14, 2021 at 12:29:28PM -0500, Brian Foster wrote:
>> On Thu, Jan 14, 2021 at 05:20:29AM -0500, Yumei Huang wrote:
>>> Hit the issue when doing syzkaller test with kernel 5.11.0-rc3(65f0d241). The C reproducer is attached.
>>>
>>> Steps to Reproduce:
>>> 1. # gcc -pthread -o reproducer reproducer.c
>>> 2. # ./reproducer
>>>
>>>
>>> Test results:
>>> [ 131.726790] XFS: Assertion failed: (iattr->ia_valid & (ATTR_UID|ATTR_GID|ATTR_ATIME|ATTR_ATIME_SET| ATTR_MTIME_SET|ATTR_KILL_PRIV|ATTR_TIMES_SET)) == 0, file: fs/xfs/xfs_iops.c, line: 849
>>> [ 131.743687] ------------[ cut here ]------------
>>
>> Some quick initial analysis from a run of the reproducer... It looks
>> like it calls into xfs_setattr_size() with ATTR_KILL_PRIV set in
>> ->ia_valid. This appears to originate in the VFS via handle_truncate()
>> -> do_truncate() -> dentry_needs_remove_privs().
>>
>> An strace of the reproducer shows the following calls:
>>
>> ...
>> [pid 1524] creat("./file0", 010) = 3
>> ...
>> [pid 1524] fsetxattr(3, "security.capability", "\0\0\0\3b\27\0\0\10\0\0\0\2\0\0\0\377\377\377\377\0\356\0", 24, 0 <unfinished ...>
>> ...
>> [pid 1524] creat("./file0", 010 <unfinished ...>
>> ...
>>
>> So I'm guessing there's an attempt to open this file with O_TRUNC with
>> this particular xattr set (unexpectedly?). Indeed, after the reproducer
>> leaves file01 around with the xattr, a subsequent xfs_io -c "open -t
>> ..." attempt triggers the assert again, and then the xattr disappears.
>> I'd have to dig more into the associated vfs code to grok the expected
>> behavior and whether there's a problem here..
>>
>
> The reproducer seems to boil down to this:
>
> touch <file>
> setfattr -n security.capability -v 0sAAAAA2IXAAAIAAAAAgAAAP////8A7gAA <file>
> xfs_io -c "open -t <file>"
>
> ... and afaict, the behavior is as expected. do_truncate() sets
> ATTR_KILL_PRIV via dentry_needs_remove_privs() and calls into
> notify_change(). That eventually gets to xfs_vn_setattr_size(), which
> calls xfs_vn_change_ok() -> setattr_prepare(). setattr_prepare() handles
> ATTR_KILL_PRIV (which remains set in ->ia_valid), and then we return,
> fall into xfs_setattr_size() and that triggers the assert failure. ISTM
> we should probably just drop ATTR_KILL_PRIV from the assert.
I dumped the ia_valid value, and it's got these bits set:
3 ATTR_SIZE
5 ATTR_MTIME
6 ATTR_CTIME
9 ATTR_FORCE
13 ATTR_FILE
14 ATTR_KILL_PRIV
15 ATTR_OPEN
so you are right about ATTR_KILL_PRIV
It's been in the assert forever, though, which is interesting?
-Eric
