[PATCH] btrfs: don't BUG_ON() in btrfs_drop_extents()

[PATCH] btrfs: don't BUG_ON() in btrfs_drop_extents()

[Johannes Thumshirn]

From: Johannes Thumshirn <johannes.thumshirn@wdc.com>

btrfs_drop_extents() calls BUG_ON() in case the counter of to be deleted
extents is greater than 0 in two code paths. But both of these code paths
can handle errors, so there's no need to crash the kernel, but gracefully
bail out.

For developer builds with CONFIG_BTRFS_ASSERT turned on, ASSERT() that
this conditon is never met.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/btrfs/file.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index fbb753300071..33f0de10df5b 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -245,7 +245,11 @@ int btrfs_drop_extents(struct btrfs_trans_handle *trans,
 next_slot:
 		leaf = path->nodes[0];
 		if (path->slots[0] >= btrfs_header_nritems(leaf)) {
-			BUG_ON(del_nr > 0);
+			ASSERT(del_nr == 0);
+			if (del_nr > 0) {
+				ret = -EINVAL;
+				break;
+			}
 			ret = btrfs_next_leaf(root, path);
 			if (ret < 0)
 				break;
@@ -321,7 +325,11 @@ int btrfs_drop_extents(struct btrfs_trans_handle *trans,
 		 *  | -------- extent -------- |
 		 */
 		if (args->start > key.offset && args->end < extent_end) {
-			BUG_ON(del_nr > 0);
+			ASSERT(del_nr == 0);
+			if (del_nr > 0) {
+				ret = -EINVAL;
+				break;
+			}
 			if (extent_type == BTRFS_FILE_EXTENT_INLINE) {
 				ret = -EOPNOTSUPP;
 				break;
-- 
2.47.0

Re: [PATCH] btrfs: don't BUG_ON() in btrfs_drop_extents()

[Filipe Manana]

On Thu, Nov 28, 2024 at 9:34 AM Johannes Thumshirn <jth@kernel.org> wrote:
>
> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>
> btrfs_drop_extents() calls BUG_ON() in case the counter of to be deleted
> extents is greater than 0 in two code paths. But both of these code paths
> can handle errors, so there's no need to crash the kernel, but gracefully
> bail out.
>
> For developer builds with CONFIG_BTRFS_ASSERT turned on, ASSERT() that
> this conditon is never met.
>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> ---
>  fs/btrfs/file.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index fbb753300071..33f0de10df5b 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -245,7 +245,11 @@ int btrfs_drop_extents(struct btrfs_trans_handle *trans,
>  next_slot:
>                 leaf = path->nodes[0];
>                 if (path->slots[0] >= btrfs_header_nritems(leaf)) {
> -                       BUG_ON(del_nr > 0);
> +                       ASSERT(del_nr == 0);
> +                       if (del_nr > 0) {
> +                               ret = -EINVAL;
> +                               break;
> +                       }
>                         ret = btrfs_next_leaf(root, path);
>                         if (ret < 0)
>                                 break;
> @@ -321,7 +325,11 @@ int btrfs_drop_extents(struct btrfs_trans_handle *trans,
>                  *  | -------- extent -------- |
>                  */
>                 if (args->start > key.offset && args->end < extent_end) {
> -                       BUG_ON(del_nr > 0);
> +                       ASSERT(del_nr == 0);
> +                       if (del_nr > 0) {
> +                               ret = -EINVAL;
> +                               break;
> +                       }

Why only these 2 BUG_ON()'s?

There's another BUG_ON(del_nr > 0) below.

There's also a similar one further below:

BUG_ON(del_slot + del_nr != path->slots[0]);

Also, I'd rather have a WARN_ON() in the if statements, so that in
case assertions are disabled (and they are disabled on some distros by
default),
we get better chances of the issue getting noticed and reported by users.

Thanks.


>                         if (extent_type == BTRFS_FILE_EXTENT_INLINE) {
>                                 ret = -EOPNOTSUPP;
>                                 break;
> --
> 2.47.0
>
>

Re: [PATCH] btrfs: don't BUG_ON() in btrfs_drop_extents()

[Johannes Thumshirn]

On 28.11.24 13:38, Filipe Manana wrote:
> On Thu, Nov 28, 2024 at 9:34 AM Johannes Thumshirn <jth@kernel.org> wrote:
>>
>> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>>
>> btrfs_drop_extents() calls BUG_ON() in case the counter of to be deleted
>> extents is greater than 0 in two code paths. But both of these code paths
>> can handle errors, so there's no need to crash the kernel, but gracefully
>> bail out.
>>
>> For developer builds with CONFIG_BTRFS_ASSERT turned on, ASSERT() that
>> this conditon is never met.
>>
>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>> ---
>>   fs/btrfs/file.c | 12 ++++++++++--
>>   1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
>> index fbb753300071..33f0de10df5b 100644
>> --- a/fs/btrfs/file.c
>> +++ b/fs/btrfs/file.c
>> @@ -245,7 +245,11 @@ int btrfs_drop_extents(struct btrfs_trans_handle *trans,
>>   next_slot:
>>                  leaf = path->nodes[0];
>>                  if (path->slots[0] >= btrfs_header_nritems(leaf)) {
>> -                       BUG_ON(del_nr > 0);
>> +                       ASSERT(del_nr == 0);
>> +                       if (del_nr > 0) {
>> +                               ret = -EINVAL;
>> +                               break;
>> +                       }
>>                          ret = btrfs_next_leaf(root, path);
>>                          if (ret < 0)
>>                                  break;
>> @@ -321,7 +325,11 @@ int btrfs_drop_extents(struct btrfs_trans_handle *trans,
>>                   *  | -------- extent -------- |
>>                   */
>>                  if (args->start > key.offset && args->end < extent_end) {
>> -                       BUG_ON(del_nr > 0);
>> +                       ASSERT(del_nr == 0);
>> +                       if (del_nr > 0) {
>> +                               ret = -EINVAL;
>> +                               break;
>> +                       }
> 
> Why only these 2 BUG_ON()'s?
> 
> There's another BUG_ON(del_nr > 0) below.
> 
> There's also a similar one further below:
> 
> BUG_ON(del_slot + del_nr != path->slots[0]);

Plain oversight. I just came across this becasuse I needed an example 
for a similar "punch hole" problematic in RST.

> Also, I'd rather have a WARN_ON() in the if statements, so that in
> case assertions are disabled (and they are disabled on some distros by
> default),
> we get better chances of the issue getting noticed and reported by users.

Sure I'll change it and also include the other BUG_ON()s.


> Thanks.
> 
> 
>>                          if (extent_type == BTRFS_FILE_EXTENT_INLINE) {
>>                                  ret = -EOPNOTSUPP;
>>                                  break;
>> --
>> 2.47.0
>>
>>
>