[PATCH] btrfs: fix extent_state leak in btrfs_lock_and_flush_ordered_range

[PATCH] btrfs: fix extent_state leak in btrfs_lock_and_flush_ordered_range

[Naohiro Aota]

btrfs_lock_and_flush_ordered_range() loads given "*cached_state" into
cachedp, which, in general, is NULL. Then, lock_extent_bits() updates
"cachedp", but it never goes backs to the caller. Thus the caller still
see its "cached_state" to be NULL and never free the state allocated
under btrfs_lock_and_flush_ordered_range(). As a result, we will
see massive state leak with e.g. fstests btrfs/005. Fix this bug by
properly handling the pointers.

Fixes: bd80d94efb83 ("btrfs: Always use a cached extent_state in btrfs_lock_and_flush_ordered_range")
Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
---
 fs/btrfs/ordered-data.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
index df02ed25b7db..ab31b1a1b624 100644
--- a/fs/btrfs/ordered-data.c
+++ b/fs/btrfs/ordered-data.c
@@ -982,13 +982,14 @@ void btrfs_lock_and_flush_ordered_range(struct extent_io_tree *tree,
 					struct extent_state **cached_state)
 {
 	struct btrfs_ordered_extent *ordered;
-	struct extent_state *cachedp = NULL;
+	struct extent_state *cache = NULL;
+	struct extent_state **cachedp = &cache;
 
 	if (cached_state)
-		cachedp = *cached_state;
+		cachedp = cached_state;
 
 	while (1) {
-		lock_extent_bits(tree, start, end, &cachedp);
+		lock_extent_bits(tree, start, end, cachedp);
 		ordered = btrfs_lookup_ordered_range(inode, start,
 						     end - start + 1);
 		if (!ordered) {
@@ -998,10 +999,10 @@ void btrfs_lock_and_flush_ordered_range(struct extent_io_tree *tree,
 			 * aren't exposing it outside of this function
 			 */
 			if (!cached_state)
-				refcount_dec(&cachedp->refs);
+				refcount_dec(&cache->refs);
 			break;
 		}
-		unlock_extent_cached(tree, start, end, &cachedp);
+		unlock_extent_cached(tree, start, end, cachedp);
 		btrfs_start_ordered_extent(&inode->vfs_inode, ordered, 1);
 		btrfs_put_ordered_extent(ordered);
 	}
-- 
2.22.0

Re: [PATCH] btrfs: fix extent_state leak in btrfs_lock_and_flush_ordered_range

[Nikolay Borisov]

On 26.07.19 г. 10:47 ч., Naohiro Aota wrote:
> btrfs_lock_and_flush_ordered_range() loads given "*cached_state" into
> cachedp, which, in general, is NULL. Then, lock_extent_bits() updates
> "cachedp", but it never goes backs to the caller. Thus the caller still
> see its "cached_state" to be NULL and never free the state allocated
> under btrfs_lock_and_flush_ordered_range(). As a result, we will
> see massive state leak with e.g. fstests btrfs/005. Fix this bug by
> properly handling the pointers.
> 
> Fixes: bd80d94efb83 ("btrfs: Always use a cached extent_state in btrfs_lock_and_flush_ordered_range")
> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
>  fs/btrfs/ordered-data.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
> index df02ed25b7db..ab31b1a1b624 100644
> --- a/fs/btrfs/ordered-data.c
> +++ b/fs/btrfs/ordered-data.c
> @@ -982,13 +982,14 @@ void btrfs_lock_and_flush_ordered_range(struct extent_io_tree *tree,
>  					struct extent_state **cached_state)
>  {
>  	struct btrfs_ordered_extent *ordered;
> -	struct extent_state *cachedp = NULL;
> +	struct extent_state *cache = NULL;
> +	struct extent_state **cachedp = &cache;
>  
>  	if (cached_state)
> -		cachedp = *cached_state;
> +		cachedp = cached_state;
>  
>  	while (1) {
> -		lock_extent_bits(tree, start, end, &cachedp);
> +		lock_extent_bits(tree, start, end, cachedp);
>  		ordered = btrfs_lookup_ordered_range(inode, start,
>  						     end - start + 1);
>  		if (!ordered) {
> @@ -998,10 +999,10 @@ void btrfs_lock_and_flush_ordered_range(struct extent_io_tree *tree,
>  			 * aren't exposing it outside of this function
>  			 */
>  			if (!cached_state)
> -				refcount_dec(&cachedp->refs);
> +				refcount_dec(&cache->refs);
>  			break;
>  		}
> -		unlock_extent_cached(tree, start, end, &cachedp);
> +		unlock_extent_cached(tree, start, end, cachedp);
>  		btrfs_start_ordered_extent(&inode->vfs_inode, ordered, 1);
>  		btrfs_put_ordered_extent(ordered);
>  	}
> 

Re: [PATCH] btrfs: fix extent_state leak in btrfs_lock_and_flush_ordered_range

[David Sterba]

On Fri, Jul 26, 2019 at 04:47:05PM +0900, Naohiro Aota wrote:
> btrfs_lock_and_flush_ordered_range() loads given "*cached_state" into
> cachedp, which, in general, is NULL. Then, lock_extent_bits() updates
> "cachedp", but it never goes backs to the caller. Thus the caller still
> see its "cached_state" to be NULL and never free the state allocated
> under btrfs_lock_and_flush_ordered_range(). As a result, we will
> see massive state leak with e.g. fstests btrfs/005. Fix this bug by
> properly handling the pointers.
> 
> Fixes: bd80d94efb83 ("btrfs: Always use a cached extent_state in btrfs_lock_and_flush_ordered_range")
> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>

Queued for 5.3, thanks.