Re: [PATCH] xfs: fix a buffer lookup against removal race

[Carlos Maiolino <cem@kernel.org>]

On Fri, May 15, 2026 at 03:31:58PM +0200, Christoph Hellwig wrote:
> When a buffer is freed either by LRU eviction or because it is unset,
> the lockref is marked as dead instantly, which prevents the buffer from
> being used after finding it in the buffer hash in xfs_buf_lookup and
> xfs_buf_find_insert.  But the latter will then not add the new buffer to
> the hash because it already found an existing buffer.
> 
> Fix this using in two places:  Remove the buffer from the hash before
> marking the lockref dead so that that no buffer with a dead lockref can
> be found in the hash, but if we find one in xfs_buf_find_insert due to
> store reordering, handle this case correctly instead of returning an
> unhashed buffer.
> 
> Fixes: 67fe4303972e ("xfs: don't keep a reference for buffers on the LRU")
> Reported-by: Andrey Albershteyn <aalbersh@redhat.com>
> Reported-by: Carlos Maiolino <cem@kernel.org>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

>  fs/xfs/xfs_buf.c | 33 +++++++++++++++++++++++----------
>  1 file changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 580d40a5ee57..a095a5c0a01f 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -472,6 +472,7 @@ xfs_buf_find_insert(
>  	/* The new buffer keeps the perag reference until it is freed. */
>  	new_bp->b_pag = pag;
>  
> +retry:
>  	rcu_read_lock();
>  	bp = rhashtable_lookup_get_insert_fast(&btp->bt_hash,
>  			&new_bp->b_rhash_head, xfs_buf_hash_params);
> @@ -480,8 +481,15 @@ xfs_buf_find_insert(
>  		error = PTR_ERR(bp);
>  		goto out_free_buf;
>  	}
> -	if (bp && lockref_get_not_dead(&bp->b_lockref)) {
> -		/* found an existing buffer */
> +	if (bp) {
> +		/*
> +		 * If there is an existing buffer with a dead lockref, retry
> +		 * until the new buffer is added or usable buffer is found.
> +		 */
> +		if (!lockref_get_not_dead(&bp->b_lockref)) {
> +			rcu_read_unlock();
> +			goto retry;
> +		}
>  		rcu_read_unlock();
>  		error = xfs_buf_find_lock(bp, flags);
>  		if (error)
> @@ -820,15 +828,20 @@ xfs_buf_destroy(
>  	ASSERT(__lockref_is_dead(&bp->b_lockref));
>  	ASSERT(!(bp->b_flags & _XBF_DELWRI_Q));
>  
> +	if (bp->b_pag)
> +		xfs_perag_put(bp->b_pag);
> +	xfs_buf_free(bp);
> +}
> +
> +static inline void
> +xfs_buf_kill(
> +	struct xfs_buf		*bp)
> +{
>  	if (!xfs_buf_is_uncached(bp)) {
>  		rhashtable_remove_fast(&bp->b_target->bt_hash,
>  				&bp->b_rhash_head, xfs_buf_hash_params);
> -
> -		if (bp->b_pag)
> -			xfs_perag_put(bp->b_pag);
>  	}
> -
> -	xfs_buf_free(bp);
> +	lockref_mark_dead(&bp->b_lockref);
>  }
>  
>  /*
> @@ -851,7 +864,7 @@ xfs_buf_rele(
>  	return;
>  
>  kill:
> -	lockref_mark_dead(&bp->b_lockref);
> +	xfs_buf_kill(bp);
>  	list_lru_del_obj(&bp->b_target->bt_lru, &bp->b_lru);
>  	spin_unlock(&bp->b_lockref.lock);
>  
> @@ -1433,7 +1446,7 @@ xfs_buftarg_drain_rele(
>  		return LRU_SKIP;
>  	}
>  
> -	lockref_mark_dead(&bp->b_lockref);
> +	xfs_buf_kill(bp);
>  	list_lru_isolate_move(lru, item, dispose);
>  	spin_unlock(&bp->b_lockref.lock);
>  	return LRU_REMOVED;
> @@ -1545,7 +1558,7 @@ xfs_buftarg_isolate(
>  		return LRU_ROTATE;
>  	}
>  
> -	lockref_mark_dead(&bp->b_lockref);
> +	xfs_buf_kill(bp);
>  	list_lru_isolate_move(lru, item, dispose);
>  	spin_unlock(&bp->b_lockref.lock);
>  	return LRU_REMOVED;
> -- 
> 2.53.0
>