On Tue, Nov 20, 2018 at 08:04:55AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> When retrying a failed inode or dquot buffer,
> xfs_buf_resubmit_failed_buffers() clears all the failed flags from
> the inde/dquot log items. In doing so, it also drops all the
> reference counts on the buffer that the failed log items hold. This
> means it can drop all the active references on the buffer and hence
> free the buffer before it queues it for write again.
>
> Putting the buffer on the delwri queue takes a reference to the
> buffer (so that it hangs around until it has been written and
> completed), but this goes bang if the buffer has already been freed.
>
> Hence we need to add the buffer to the delwri queue before we remove
> the failed flags from the log items attached to the buffer to ensure
> it always remains referenced during the resubmit process.
>
> Reported-by: Josef Bacik <josef@xxxxxxxxxxxxxx>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
--D
> ---
> fs/xfs/xfs_buf_item.c | 28 +++++++++++++++++++++-------
> 1 file changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 12d8455bfbb2..010db5f8fb00 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -1233,9 +1233,23 @@ xfs_buf_iodone(
> }
>
> /*
> - * Requeue a failed buffer for writeback
> + * Requeue a failed buffer for writeback.
> *
> - * Return true if the buffer has been re-queued properly, false otherwise
> + * We clear the log item failed state here as well, but we have to be careful
> + * about reference counts because the only active reference counts on the buffer
> + * may be the failed log items. Hence if we clear the log item failed state
> + * before queuing the buffer for IO we can release all active references to
> + * the buffer and free it, leading to use after free problems in
> + * xfs_buf_delwri_queue. It makes no difference to the buffer or log items which
> + * order we process them in - the buffer is locked, and we own the buffer list
> + * so nothing on them is going to change while we are performing this action.
> + *
> + * Hence we can safely queue the buffer for IO before we clear the failed log
> + * item state, therefore always having an active reference to the buffer and
> + * avoiding the transient zero-reference state that leads to use-after-free.
> + *
> + * Return true if the buffer was added to the buffer list, false if it was
> + * already on the buffer list.
> */
> bool
> xfs_buf_resubmit_failed_buffers(
> @@ -1243,16 +1257,16 @@ xfs_buf_resubmit_failed_buffers(
> struct list_head *buffer_list)
> {
> struct xfs_log_item *lip;
> + bool ret;
> +
> + ret = xfs_buf_delwri_queue(bp, buffer_list);
>
> /*
> - * Clear XFS_LI_FAILED flag from all items before resubmit
> - *
> - * XFS_LI_FAILED set/clear is protected by ail_lock, caller this
> + * XFS_LI_FAILED set/clear is protected by ail_lock, caller of this
> * function already have it acquired
> */
> list_for_each_entry(lip, &bp->b_li_list, li_bio_list)
> xfs_clear_li_failed(lip);
>
> - /* Add this buffer back to the delayed write list */
> - return xfs_buf_delwri_queue(bp, buffer_list);
> + return ret;
> }
> --
> 2.19.1
>
