Hi all, Here's a proper v1 of the previously posted RFC to address a subtle buffer use after free in the unpin abort sequence for buffer log items. Dave had previously suggested that the underlying problem here is that bli's are effectively used by the AIL unreferenced. While this makes a lot of sense, this is a long standing design detail that subtly impacts code related to log item processing, AIL processing, buffer I/O, as well as potentially log recovery. In contrast, the immediate problem that leads to the use after free is lack of a buffer hold in a context that already explicitly acquires a hold for the problematic simulated I/O failure sequence. Given the significant cost/risk vs. benefit imbalance of a design rework, I've opted to to make the minimal change to fix this bug and defer broader rework to a standalone effort. Patch 1 basically reorders the preexisting buffer hold to accommodate the flaw that the AIL does not hold a reference to the bli (and thus does not maintain the associated buffer hold). This preserves the existing isolation logic and prevents the associated UAF. This survives an fstests run and is going on 6k iterations of generic/019 (which previously reproduced the problem in 2-3k iterations) without any explosions. Thoughts, reviews, flames appreciated. Brian v1: - Rework patch 1 to hold conditionally in the abort case and document the underlying design flaw. - Add patch 2 to remove some unused code. rfc: https://lore.kernel.org/linux-xfs/20210503121816.561340-1-bfoster@xxxxxxxxxx/ Brian Foster (2): xfs: hold buffer across unpin and potential shutdown processing xfs: remove dead stale buf unpin handling code fs/xfs/xfs_buf_item.c | 57 +++++++++++++++++-------------------------- 1 file changed, 22 insertions(+), 35 deletions(-) -- 2.26.3
