Re: [PATCH] xfs: fix slab-use-after-free in xrep_newbt_free()

[Zheng Shuhao <viennaoffh@gmail.com>]

Hi Carlos,

I have to admit I was not sure how to reply to your last message, or
whether I should reply at all. You said to wait until the patch gets
reviewed before sending a V2, so I did not want to push or send noise to
the list. On the other hand, it has now been a few weeks without any
further review or comment, and staying silent did not seem right either.
So I am sending this short status note; if you would rather I simply
wait, please say so and I will do that.

I also wanted to close the two items you asked about earlier, since I
promised them and then went quiet.

> Yes, sharing the reproducer (and potentially hooking it up into
> xfstests) is a good thing, specially giving the syzbot doesn't seem
> to have a reproducer for it yet.

The reproducer is a standalone harness, not a syzkaller program. It
mounts a scratch XFS with rmapbt and reflink, starts an online repair
that rebuilds a btree-format inode fork, and then issues
XFS_IOC_GOINGDOWN (LOGFLUSH and NOLOGFLUSH) from 1 to 8 helper threads
at 0-100 ms after the xfs_defer_item_pause tracepoint.

On the unpatched kernel this produces the reported KASAN
slab-use-after-free in xrep_newbt_free() -> xfs_defer_item_unpause()
on every run. With the patch applied, the scrub ioctl returns -EIO, the
shutdown path unwinds, the log replays on remount, the free block count
is unchanged, file data reads back intact, and xfs_repair -n is clean.
Healthy forced rebuilds still take the normal pause/unpause path.

I can send the harness and logs, but I did not want to attach a large
tarball to the list without asking first.

> I'm also interested if you actually passed your patch through xfstests
> before submitting it. There is no good fixing a bug if you create a
> handful of others. Specialy agains scrub, online_repair (and maybe
> fsstress_scrub) groups.

I ran the patched kernel against the scrub and online-repair fuzzer
cases, plus fsstress_scrub. To be precise about the coverage:

 - Completed with a real verdict: xfs/351, 353, 355, 357, 359, 361,
   363, 365, 367, 369, 371, 373, 375, 377, 379, 381, 383, 385, 399,
   401, 403, 405, 411, 413, 415, 417, 426, 428, 430, 497, 730, 733,
   736, 786.
 - xfs/387, 389, 391, 393, 395, 397 were started but hit my per-test
   timeout before reaching a verdict. They are long directory/xattr
   walks; I am not counting them as passes.

No KASAN report was produced by the patched kernel. The failures I did
see are all reproducible on the unpatched baseline as well, so they are
pre-existing and not introduced by this patch:

 - xfs_btree.c:1814 in xfs_btree_decrement (xfs/361, and also xfs/373)
 - xfs_buf.c:389 stale-buffer assertion (xfs/363, 365, 367)
 - xfs_log_cil.c:546 empty-transaction assertion (xfs/369)
 - xfs_attr_leaf.c:168 attribute-leaf assertion (xfs/401)
 - null-ptr-deref in xfs_btree_simple_query_range, reached from
   xrep_dinode_count_rtgroup_rmaps (xfs/375, 377, 379, 385, 399, 413,
   415, 417). This one reproduces identically on the baseline kernel.

> You'll need to send a V2. But wait until your patch gets reviewed, you
> don't need to send it right now.

Understood. I am holding the V2 until there is review feedback. When I
send it, it will use the correct identity:

    From: Zheng Shuhao <viennaoffh@gmail.com>
    Signed-off-by: Zheng Shuhao <viennaoffh@gmail.com>

Is there anything you would like changed in the patch itself, or any
additional data you want before I send the V2? If it is more useful, I
can also send the reproducer and the per-test logs now, separately from
the patch.

Thanks,
Zheng Shuhao