[PATCH DON'T MERGE] btrfs-progs: crash if eb has leaked for debug builds

[PATCH DON'T MERGE] btrfs-progs: crash if eb has leaked for debug builds

[Qu Wenruo]

!!! DON'T MERGE !!!

Currently if we leaked some extent buffers, btrfs-progs can still work
fine, and will only output a not-that-obvious message like:

 extent buffer leak: start 30572544 len 16384

This is pretty hard to catch and test cases will not be able to catch
such regression.

This patch will add a new default debug cflags,
-DDEBUG_ABORT_ON_EB_LEAK, and in extent_io_tree_cleanup(), if that
debug flag is enabled, we will report all the leaked eb first, then
crash to make users and test cases to catch this problem.

Unfortunately the eb leakage is a big problem, fsck tests can only reach
002 before crashing at that test image.

If someone can help fixing all the eb leakage it would be great.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 Makefile                  | 2 +-
 kernel-shared/extent_io.c | 8 +++++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 2a37d1c6b5eb..beaa31d36f0e 100644
--- a/Makefile
+++ b/Makefile
@@ -65,7 +65,7 @@ include Makefile.extrawarn
 EXTRA_CFLAGS :=
 EXTRA_LDFLAGS :=
 
-DEBUG_CFLAGS_DEFAULT = -O0 -U_FORTIFY_SOURCE -ggdb3
+DEBUG_CFLAGS_DEFAULT = -O0 -U_FORTIFY_SOURCE -ggdb3 -DDEBUG_ABORT_ON_EB_LEAK
 DEBUG_CFLAGS_INTERNAL =
 DEBUG_CFLAGS :=
 
diff --git a/kernel-shared/extent_io.c b/kernel-shared/extent_io.c
index 48bcf2cf2f96..6def70a3fca4 100644
--- a/kernel-shared/extent_io.c
+++ b/kernel-shared/extent_io.c
@@ -84,6 +84,7 @@ static void free_extent_buffer_final(struct extent_buffer *eb);
 void extent_io_tree_cleanup(struct extent_io_tree *tree)
 {
 	struct extent_buffer *eb;
+	bool leaked = false;
 
 	while(!list_empty(&tree->lru)) {
 		eb = list_entry(tree->lru.next, struct extent_buffer, lru);
@@ -92,11 +93,16 @@ void extent_io_tree_cleanup(struct extent_io_tree *tree)
 				"extent buffer leak: start %llu len %u\n",
 				(unsigned long long)eb->start, eb->len);
 			free_extent_buffer_nocache(eb);
+			leaked = true;
 		} else {
 			free_extent_buffer_final(eb);
 		}
 	}
-
+	if (leaked) {
+#ifdef DEBUG_ABORT_ON_EB_LEAK
+		abort();
+#endif
+	}
 	cache_tree_free_extents(&tree->state, free_extent_state_func);
 }
 
-- 
2.37.2

Re: [PATCH DON'T MERGE] btrfs-progs: crash if eb has leaked for debug builds

[David Sterba]

On Tue, Aug 30, 2022 at 01:10:29PM +0800, Qu Wenruo wrote:
> !!! DON'T MERGE !!!
> 
> Currently if we leaked some extent buffers, btrfs-progs can still work
> fine, and will only output a not-that-obvious message like:
> 
>  extent buffer leak: start 30572544 len 16384
> 
> This is pretty hard to catch and test cases will not be able to catch
> such regression.
> 
> This patch will add a new default debug cflags,
> -DDEBUG_ABORT_ON_EB_LEAK, and in extent_io_tree_cleanup(), if that
> debug flag is enabled, we will report all the leaked eb first, then
> crash to make users and test cases to catch this problem.
> 
> Unfortunately the eb leakage is a big problem, fsck tests can only reach
> 002 before crashing at that test image.
> 
> If someone can help fixing all the eb leakage it would be great.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  Makefile                  | 2 +-
>  kernel-shared/extent_io.c | 8 +++++++-
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 2a37d1c6b5eb..beaa31d36f0e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -65,7 +65,7 @@ include Makefile.extrawarn
>  EXTRA_CFLAGS :=
>  EXTRA_LDFLAGS :=
>  
> -DEBUG_CFLAGS_DEFAULT = -O0 -U_FORTIFY_SOURCE -ggdb3
> +DEBUG_CFLAGS_DEFAULT = -O0 -U_FORTIFY_SOURCE -ggdb3 -DDEBUG_ABORT_ON_EB_LEAK
>  DEBUG_CFLAGS_INTERNAL =

We have the tests/scan-results.sh script with patterns to look for, the
extent buffer leak is not there but it could be:

+                       *extent\ buffer\ leak*) echo "EXTENT BUFFER LEAK: $last" ;;

The standard assert.h and assert can be used for the leak check but it's
also in the release build I think so we may use a separate macro for
that, or maybe add another class for leak checks.

Re: [PATCH DON'T MERGE] btrfs-progs: crash if eb has leaked for debug builds

[Qu Wenruo]

On 2022/8/31 01:57, David Sterba wrote:
> On Tue, Aug 30, 2022 at 01:10:29PM +0800, Qu Wenruo wrote:
>> !!! DON'T MERGE !!!
>>
>> Currently if we leaked some extent buffers, btrfs-progs can still work
>> fine, and will only output a not-that-obvious message like:
>>
>>   extent buffer leak: start 30572544 len 16384
>>
>> This is pretty hard to catch and test cases will not be able to catch
>> such regression.
>>
>> This patch will add a new default debug cflags,
>> -DDEBUG_ABORT_ON_EB_LEAK, and in extent_io_tree_cleanup(), if that
>> debug flag is enabled, we will report all the leaked eb first, then
>> crash to make users and test cases to catch this problem.
>>
>> Unfortunately the eb leakage is a big problem, fsck tests can only reach
>> 002 before crashing at that test image.
>>
>> If someone can help fixing all the eb leakage it would be great.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   Makefile                  | 2 +-
>>   kernel-shared/extent_io.c | 8 +++++++-
>>   2 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/Makefile b/Makefile
>> index 2a37d1c6b5eb..beaa31d36f0e 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -65,7 +65,7 @@ include Makefile.extrawarn
>>   EXTRA_CFLAGS :=
>>   EXTRA_LDFLAGS :=
>>   
>> -DEBUG_CFLAGS_DEFAULT = -O0 -U_FORTIFY_SOURCE -ggdb3
>> +DEBUG_CFLAGS_DEFAULT = -O0 -U_FORTIFY_SOURCE -ggdb3 -DDEBUG_ABORT_ON_EB_LEAK
>>   DEBUG_CFLAGS_INTERNAL =
> 
> We have the tests/scan-results.sh script with patterns to look for, the
> extent buffer leak is not there but it could be:

No wonder it's not detected during selftest.

But it's still not as noisy as what we'd like, nor it's not even enabled 
by default.
At least a quick grep "scan-results" inside tests directory only got one 
hit in 'testsuite-files'.

> 
> +                       *extent\ buffer\ leak*) echo "EXTENT BUFFER LEAK: $last" ;;
> 
> The standard assert.h and assert can be used for the leak check but it's
> also in the release build I think so we may use a separate macro for
> that, or maybe add another class for leak checks.

That's why I want to use the new debug macro, mostly for the following 
two reasons:

- Avoid crash in release build
   This leakage won't hurt any end users except our pride and code
   correctness.

- Be included for any debug build
   We have D=* settings, but I doubt if most developers would use
   anything other than D=1 for most of their development.

   I can go something like D=eb_leak_detect, but at least myself won't
   use it in my daily development.

Any better ideas on this?

Thanks,
Qu