On 1/14/21 12:24 PM, Brian Foster wrote:

On Thu, Jan 14, 2021 at 12:29:28PM -0500, Brian Foster wrote:

quoted

On Thu, Jan 14, 2021 at 05:20:29AM -0500, Yumei Huang wrote:

quoted

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