lkml.org 
[lkml]   [2022]   [Aug]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 5.10 58/65] xfs: hold buffer across unpin and potential shutdown processing
    Date
    From: Brian Foster <bfoster@redhat.com>

    commit 84d8949e770745b16a7e8a68dcb1d0f3687bdee9 upstream.

    The special processing used to simulate a buffer I/O failure on fs
    shutdown has a difficult to reproduce race that can result in a use
    after free of the associated buffer. Consider a buffer that has been
    committed to the on-disk log and thus is AIL resident. The buffer
    lands on the writeback delwri queue, but is subsequently locked,
    committed and pinned by another transaction before submitted for
    I/O. At this point, the buffer is stuck on the delwri queue as it
    cannot be submitted for I/O until it is unpinned. A log checkpoint
    I/O failure occurs sometime later, which aborts the bli. The unpin
    handler is called with the aborted log item, drops the bli reference
    count, the pin count, and falls into the I/O failure simulation
    path.

    The potential problem here is that once the pin count falls to zero
    in ->iop_unpin(), xfsaild is free to retry delwri submission of the
    buffer at any time, before the unpin handler even completes. If
    delwri queue submission wins the race to the buffer lock, it
    observes the shutdown state and simulates the I/O failure itself.
    This releases both the bli and delwri queue holds and frees the
    buffer while xfs_buf_item_unpin() sits on xfs_buf_lock() waiting to
    run through the same failure sequence. This problem is rare and
    requires many iterations of fstest generic/019 (which simulates disk
    I/O failures) to reproduce.

    To avoid this problem, grab a hold on the buffer before the log item
    is unpinned if the associated item has been aborted and will require
    a simulated I/O failure. The hold is already required for the
    simulated I/O failure, so the ordering simply guarantees the unpin
    handler access to the buffer before it is unpinned and thus
    processed by the AIL. This particular ordering is required so long
    as the AIL does not acquire a reference on the bli, which is the
    long term solution to this problem.

    Signed-off-by: Brian Foster <bfoster@redhat.com>
    Reviewed-by: Darrick J. Wong <djwong@kernel.org>
    Signed-off-by: Darrick J. Wong <djwong@kernel.org>
    Signed-off-by: Amir Goldstein <amir73il@gmail.com>
    Acked-by: Darrick J. Wong <djwong@kernel.org>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    ---
    fs/xfs/xfs_buf_item.c | 37 +++++++++++++++++++++----------------
    1 file changed, 21 insertions(+), 16 deletions(-)

    --- a/fs/xfs/xfs_buf_item.c
    +++ b/fs/xfs/xfs_buf_item.c
    @@ -393,17 +393,8 @@ xfs_buf_item_pin(
    }

    /*
    - * This is called to unpin the buffer associated with the buf log
    - * item which was previously pinned with a call to xfs_buf_item_pin().
    - *
    - * Also drop the reference to the buf item for the current transaction.
    - * If the XFS_BLI_STALE flag is set and we are the last reference,
    - * then free up the buf log item and unlock the buffer.
    - *
    - * If the remove flag is set we are called from uncommit in the
    - * forced-shutdown path. If that is true and the reference count on
    - * the log item is going to drop to zero we need to free the item's
    - * descriptor in the transaction.
    + * This is called to unpin the buffer associated with the buf log item which
    + * was previously pinned with a call to xfs_buf_item_pin().
    */
    STATIC void
    xfs_buf_item_unpin(
    @@ -420,12 +411,26 @@ xfs_buf_item_unpin(

    trace_xfs_buf_item_unpin(bip);

    + /*
    + * Drop the bli ref associated with the pin and grab the hold required
    + * for the I/O simulation failure in the abort case. We have to do this
    + * before the pin count drops because the AIL doesn't acquire a bli
    + * reference. Therefore if the refcount drops to zero, the bli could
    + * still be AIL resident and the buffer submitted for I/O (and freed on
    + * completion) at any point before we return. This can be removed once
    + * the AIL properly holds a reference on the bli.
    + */
    freed = atomic_dec_and_test(&bip->bli_refcount);
    -
    + if (freed && !stale && remove)
    + xfs_buf_hold(bp);
    if (atomic_dec_and_test(&bp->b_pin_count))
    wake_up_all(&bp->b_waiters);

    - if (freed && stale) {
    + /* nothing to do but drop the pin count if the bli is active */
    + if (!freed)
    + return;
    +
    + if (stale) {
    ASSERT(bip->bli_flags & XFS_BLI_STALE);
    ASSERT(xfs_buf_islocked(bp));
    ASSERT(bp->b_flags & XBF_STALE);
    @@ -468,13 +473,13 @@ xfs_buf_item_unpin(
    ASSERT(bp->b_log_item == NULL);
    }
    xfs_buf_relse(bp);
    - } else if (freed && remove) {
    + } else if (remove) {
    /*
    * The buffer must be locked and held by the caller to simulate
    - * an async I/O failure.
    + * an async I/O failure. We acquired the hold for this case
    + * before the buffer was unpinned.
    */
    xfs_buf_lock(bp);
    - xfs_buf_hold(bp);
    bp->b_flags |= XBF_ASYNC;
    xfs_buf_ioend_fail(bp);
    }

    \
     
     \ /
      Last update: 2022-08-01 13:58    [W:3.178 / U:0.120 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site