[PATCH 50/50] xfs: use reference counts to free clean buffer items

[Mark Tinguely]

On 08/12/13 05:50, Dave Chinner wrote:
> From: Dave Chinner<dchinner at redhat.com>
>
> When a transaction is cancelled and the buffer log item is clean in
> the transaction, the buffer log item is unconditionally freed. If
> the log item is in the AIL, however, this leads to a use after free
> condition as the item still has other users.
>
> In this case, xfs_buf_item_relse() should only be called on clean
> buffer items if the reference count has dropped to zero. This
> ensures only the last user frees the item.
>
> Signed-off-by: Dave Chinner<dchinner at redhat.com>
> ---
>   fs/xfs/xfs_buf_item.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 9358504..3a944b1 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -613,11 +613,9 @@ xfs_buf_item_unlock(
>   			}
>   		}
>   	}
> -	if (clean)
> -		xfs_buf_item_relse(bp);
> -	else if (aborted) {
> +	if (clean || aborted) {
>   		if (atomic_dec_and_test(&bip->bli_refcount)) {
> -			ASSERT(XFS_FORCED_SHUTDOWN(lip->li_mountp));
> +			ASSERT(!aborted || XFS_FORCED_SHUTDOWN(lip->li_mountp));
>   			xfs_buf_item_relse(bp);
>   		}
>   	} else

why is a clean buffer on the AIL? Racing with a completion handler?

rather than this:

	if (clean || aborted) {
		if (atomic_dec_and_test(&bip->bli_refcount)) {
			ASSERT(!aborted || XFS_FORCED_SHUTDOWN(lip->li_mountp));
			xfs_buf_item_relse(bp);
		}
	} else
		atomic_dec(&bip->bli_refcount);

why not fold it into this:

	if (atomic_dec_and_test(&bip->bli_refcount)) {
		ASSERT(!aborted || XFS_FORCED_SHUTDOWN(lip->li_mountp));
		xfs_buf_item_relse(bp);
	}

--Mark.