[PATCH] bcache: fix memleak when error occurred before journal replay

[PATCH] bcache: fix memleak when error occurred before journal replay

[Guoju Fang]

A list of struct journal_replay is allocated when register cache device
and will be freed when journal replay complete. It will cause memory
leaks if some error occurred before journal replay.

Signed-off-by: Guoju Fang <fangguoju@gmail.com>
---
 drivers/md/bcache/super.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
index a697a3a..e4289291 100644
--- a/drivers/md/bcache/super.c
+++ b/drivers/md/bcache/super.c
@@ -1782,6 +1782,7 @@ static void run_cache_set(struct cache_set *c)
 	struct cache *ca;
 	struct closure cl;
 	unsigned int i;
+	LIST_HEAD(journal);
 
 	closure_init_stack(&cl);
 
@@ -1790,7 +1791,6 @@ static void run_cache_set(struct cache_set *c)
 	set_gc_sectors(c);
 
 	if (CACHE_SYNC(&c->sb)) {
-		LIST_HEAD(journal);
 		struct bkey *k;
 		struct jset *j;
 
@@ -1820,25 +1820,25 @@ static void run_cache_set(struct cache_set *c)
 
 		err = "bad btree root";
 		if (__bch_btree_ptr_invalid(c, k))
-			goto err;
+			goto free_journal;
 
 		err = "error reading btree root";
 		c->root = bch_btree_node_get(c, NULL, k,
 					     j->btree_level,
 					     true, NULL);
 		if (IS_ERR_OR_NULL(c->root))
-			goto err;
+			goto free_journal;
 
 		list_del_init(&c->root->list);
 		rw_unlock(true, c->root);
 
 		err = uuid_read(c, j, &cl);
 		if (err)
-			goto err;
+			goto free_journal;
 
 		err = "error in recovery";
 		if (bch_btree_check(c))
-			goto err;
+			goto free_journal;
 
 		bch_journal_mark(c, &journal);
 		bch_initial_gc_finish(c);
@@ -1854,7 +1854,7 @@ static void run_cache_set(struct cache_set *c)
 		err = "error starting allocator thread";
 		for_each_cache(ca, c, i)
 			if (bch_cache_allocator_start(ca))
-				goto err;
+				goto free_journal;
 
 		/*
 		 * First place it's safe to allocate: btree_check() and
@@ -1938,6 +1938,14 @@ static void run_cache_set(struct cache_set *c)
 
 	set_bit(CACHE_SET_RUNNING, &c->flags);
 	return;
+
+free_journal:
+	while (!list_empty(&journal)) {
+		struct journal_replay *jr = list_first_entry(&journal,
+			    struct journal_replay, list);
+		list_del(&jr->list);
+		kfree(jr);
+	}
 err:
 	closure_sync(&cl);
 	/* XXX: test this, it's broken */
-- 
1.8.3.1

Re: [PATCH] bcache: fix memleak when error occurred before journal replay

[guoju fang]

Oh, Shenghui submitted a patch to fix this bug days ago, so please 
ignore this.

On 2019/4/29 20:30, Guoju Fang wrote:
> A list of struct journal_replay is allocated when register cache device
> and will be freed when journal replay complete. It will cause memory
> leaks if some error occurred before journal replay.
>
> Signed-off-by: Guoju Fang <fangguoju@gmail.com>
> ---
>   drivers/md/bcache/super.c | 20 ++++++++++++++------
>   1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index a697a3a..e4289291 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -1782,6 +1782,7 @@ static void run_cache_set(struct cache_set *c)
>   	struct cache *ca;
>   	struct closure cl;
>   	unsigned int i;
> +	LIST_HEAD(journal);
>   
>   	closure_init_stack(&cl);
>   
> @@ -1790,7 +1791,6 @@ static void run_cache_set(struct cache_set *c)
>   	set_gc_sectors(c);
>   
>   	if (CACHE_SYNC(&c->sb)) {
> -		LIST_HEAD(journal);
>   		struct bkey *k;
>   		struct jset *j;
>   
> @@ -1820,25 +1820,25 @@ static void run_cache_set(struct cache_set *c)
>   
>   		err = "bad btree root";
>   		if (__bch_btree_ptr_invalid(c, k))
> -			goto err;
> +			goto free_journal;
>   
>   		err = "error reading btree root";
>   		c->root = bch_btree_node_get(c, NULL, k,
>   					     j->btree_level,
>   					     true, NULL);
>   		if (IS_ERR_OR_NULL(c->root))
> -			goto err;
> +			goto free_journal;
>   
>   		list_del_init(&c->root->list);
>   		rw_unlock(true, c->root);
>   
>   		err = uuid_read(c, j, &cl);
>   		if (err)
> -			goto err;
> +			goto free_journal;
>   
>   		err = "error in recovery";
>   		if (bch_btree_check(c))
> -			goto err;
> +			goto free_journal;
>   
>   		bch_journal_mark(c, &journal);
>   		bch_initial_gc_finish(c);
> @@ -1854,7 +1854,7 @@ static void run_cache_set(struct cache_set *c)
>   		err = "error starting allocator thread";
>   		for_each_cache(ca, c, i)
>   			if (bch_cache_allocator_start(ca))
> -				goto err;
> +				goto free_journal;
>   
>   		/*
>   		 * First place it's safe to allocate: btree_check() and
> @@ -1938,6 +1938,14 @@ static void run_cache_set(struct cache_set *c)
>   
>   	set_bit(CACHE_SET_RUNNING, &c->flags);
>   	return;
> +
> +free_journal:
> +	while (!list_empty(&journal)) {
> +		struct journal_replay *jr = list_first_entry(&journal,
> +			    struct journal_replay, list);
> +		list_del(&jr->list);
> +		kfree(jr);
> +	}
>   err:
>   	closure_sync(&cl);
>   	/* XXX: test this, it's broken */