[PATCH] Revert "bcachefs: Add asserts to bch2_dev_btree_bitmap_marked_sectors()"

[PATCH] Revert "bcachefs: Add asserts to bch2_dev_btree_bitmap_marked_sectors()"

[Manas via B4 Relay]

From: Manas <manas18244@iiitd.ac.in>

This reverts commit 60f2b1bcf519416dbffee219132aa949d0c39d0e.

This syzbot bug[1] is triggered due to the BUG_ON assertions added in
__bch2_dev_btree_bitmap_mark. During runtime, m->btree_bitmap_shift is
63 '?'. This triggers both the assertions.

Reverting the commit does not reproduce the said bug.

[1] https://syzkaller.appspot.com/bug?extid=e8eff054face85d7ea41

Signed-off-by: Manas <manas18244@iiitd.ac.in>
Reported-by: syzbot+e8eff054face85d7ea41@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=e8eff054face85d7ea41
---
This syzbot bug[1] is triggered due to the BUG_ON assertions added in
__bch2_dev_btree_bitmap_mark. During runtime, m->btree_bitmap_shift is
63 '?'. This triggers both the assertions.

I am unfamiliar with the codebase, and there wasn't a lore discussion
about the assertions in the commit, so I am unsure about the relevance
of these assertions.

Reverting the commit does not reproduce the said bug.

[1] https://syzkaller.appspot.com/bug?extid=e8eff054face85d7ea41
---
 fs/bcachefs/sb-members.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/bcachefs/sb-members.c b/fs/bcachefs/sb-members.c
index fb08dd680dacf82bca414f424024e4a00bf432de..9790fd47338c46d2af30547e1f41a1e578b71aa4 100644
--- a/fs/bcachefs/sb-members.c
+++ b/fs/bcachefs/sb-members.c
@@ -450,9 +450,6 @@ static void __bch2_dev_btree_bitmap_mark(struct bch_sb_field_members_v2 *mi, uns
 		m->btree_bitmap_shift += resize;
 	}
 
-	BUG_ON(m->btree_bitmap_shift > 57);
-	BUG_ON(end > 64ULL << m->btree_bitmap_shift);
-
 	for (unsigned bit = start >> m->btree_bitmap_shift;
 	     (u64) bit << m->btree_bitmap_shift < end;
 	     bit++)

---
base-commit: 42f7652d3eb527d03665b09edac47f85fb600924
change-id: 20241021-revert-assert-bch2-25474fe809d1

Best regards,
-- 
Manas <manas18244@iiitd.ac.in>

Re: [PATCH] Revert "bcachefs: Add asserts to bch2_dev_btree_bitmap_marked_sectors()"

[Hongbo Li]

On 2024/10/22 0:48, Manas via B4 Relay wrote:
> From: Manas <manas18244@iiitd.ac.in>
> 
> This reverts commit 60f2b1bcf519416dbffee219132aa949d0c39d0e.
> 
> This syzbot bug[1] is triggered due to the BUG_ON assertions added in
> __bch2_dev_btree_bitmap_mark. During runtime, m->btree_bitmap_shift is
> 63 '?'. This triggers both the assertions.
> 
> Reverting the commit does not reproduce the said bug.
> 
> [1] https://syzkaller.appspot.com/bug?extid=e8eff054face85d7ea41
> 
> Signed-off-by: Manas <manas18244@iiitd.ac.in>
> Reported-by: syzbot+e8eff054face85d7ea41@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=e8eff054face85d7ea41
> ---
> This syzbot bug[1] is triggered due to the BUG_ON assertions added in
> __bch2_dev_btree_bitmap_mark. During runtime, m->btree_bitmap_shift is
> 63 '?'. This triggers both the assertions.
> 
> I am unfamiliar with the codebase, and there wasn't a lore discussion
> about the assertions in the commit, so I am unsure about the relevance
> of these assertions.
> 
> Reverting the commit does not reproduce the said bug.
> 
> [1] https://syzkaller.appspot.com/bug?extid=e8eff054face85d7ea41
> ---
>   fs/bcachefs/sb-members.c | 3 ---
>   1 file changed, 3 deletions(-)
> 
> diff --git a/fs/bcachefs/sb-members.c b/fs/bcachefs/sb-members.c
> index fb08dd680dacf82bca414f424024e4a00bf432de..9790fd47338c46d2af30547e1f41a1e578b71aa4 100644
> --- a/fs/bcachefs/sb-members.c
> +++ b/fs/bcachefs/sb-members.c
> @@ -450,9 +450,6 @@ static void __bch2_dev_btree_bitmap_mark(struct bch_sb_field_members_v2 *mi, uns
>   		m->btree_bitmap_shift += resize;
>   	}
>   
> -	BUG_ON(m->btree_bitmap_shift > 57);
> -	BUG_ON(end > 64ULL << m->btree_bitmap_shift);
> -
May be this is not good way by just removing the BUG_ON. In my humble 
opinion, the former code have checked m->btree_bitmap_shift in 
bch2_dev_btree_bitmap_marked_sectors. May be add the similar condition 
in this helper will be better.

Thanks,
Hongbo

>   	for (unsigned bit = start >> m->btree_bitmap_shift;
>   	     (u64) bit << m->btree_bitmap_shift < end;
>   	     bit++)
> 
> ---
> base-commit: 42f7652d3eb527d03665b09edac47f85fb600924
> change-id: 20241021-revert-assert-bch2-25474fe809d1
> 
> Best regards,

Re: [PATCH] Revert "bcachefs: Add asserts to bch2_dev_btree_bitmap_marked_sectors()"

[Manas]

On 22.10.2024 09:43, Hongbo Li wrote:
>On 2024/10/22 0:48, Manas via B4 Relay wrote:
>>From: Manas <manas18244@iiitd.ac.in>
>>
>>This reverts commit 60f2b1bcf519416dbffee219132aa949d0c39d0e.
>>
>>This syzbot bug[1] is triggered due to the BUG_ON assertions added in
>>__bch2_dev_btree_bitmap_mark. During runtime, m->btree_bitmap_shift is
>>63 '?'. This triggers both the assertions.
>>
>>Reverting the commit does not reproduce the said bug.
>>
>>[1] https://syzkaller.appspot.com/bug?extid=e8eff054face85d7ea41
>>
>>Signed-off-by: Manas <manas18244@iiitd.ac.in>
>>Reported-by: syzbot+e8eff054face85d7ea41@syzkaller.appspotmail.com
>>Closes: https://syzkaller.appspot.com/bug?extid=e8eff054face85d7ea41
>>---
>>This syzbot bug[1] is triggered due to the BUG_ON assertions added in
>>__bch2_dev_btree_bitmap_mark. During runtime, m->btree_bitmap_shift is
>>63 '?'. This triggers both the assertions.
>>
>>I am unfamiliar with the codebase, and there wasn't a lore discussion
>>about the assertions in the commit, so I am unsure about the relevance
>>of these assertions.
>>
>>Reverting the commit does not reproduce the said bug.
>>
>>[1] https://syzkaller.appspot.com/bug?extid=e8eff054face85d7ea41
>>---
>>  fs/bcachefs/sb-members.c | 3 ---
>>  1 file changed, 3 deletions(-)
>>
>>diff --git a/fs/bcachefs/sb-members.c b/fs/bcachefs/sb-members.c
>>index fb08dd680dacf82bca414f424024e4a00bf432de..9790fd47338c46d2af30547e1f41a1e578b71aa4 100644
>>--- a/fs/bcachefs/sb-members.c
>>+++ b/fs/bcachefs/sb-members.c
>>@@ -450,9 +450,6 @@ static void __bch2_dev_btree_bitmap_mark(struct bch_sb_field_members_v2 *mi, uns
>>  		m->btree_bitmap_shift += resize;
>>  	}
>>-	BUG_ON(m->btree_bitmap_shift > 57);
>>-	BUG_ON(end > 64ULL << m->btree_bitmap_shift);
>>-
>May be this is not good way by just removing the BUG_ON. In my humble 
>opinion, the former code have checked m->btree_bitmap_shift in 
>bch2_dev_btree_bitmap_marked_sectors. May be add the similar condition 
>in this helper will be better.
>
Hi Hongbo, thanks for reviewing this. I was unsure about the fix so I decided to
initiate the conversation by reverting. Yes, that makes sense. I am sending a
fresh patch adding that condition.

-- 
Manas

Re: [PATCH] Revert "bcachefs: Add asserts to bch2_dev_btree_bitmap_marked_sectors()"

[Kent Overstreet]

On Mon, Oct 21, 2024 at 10:18:57PM +0530, Manas via B4 Relay wrote:
> From: Manas <manas18244@iiitd.ac.in>
> 
> This reverts commit 60f2b1bcf519416dbffee219132aa949d0c39d0e.
> 
> This syzbot bug[1] is triggered due to the BUG_ON assertions added in
> __bch2_dev_btree_bitmap_mark. During runtime, m->btree_bitmap_shift is
> 63 '?'. This triggers both the assertions.

The BUG_ON() doesn't need to be deleted; we just need to fix the
validation so it doesn't fire (it doesn't particularly matter if it's
removed or not, ubsan would catch it without the BUG_ON()).

I believe 57 is correct, 64 - (2^6, which is 64, i.e. size of the btree
bitmap), but >= 58 would have been better.

> 
> Reverting the commit does not reproduce the said bug.
> 
> [1] https://syzkaller.appspot.com/bug?extid=e8eff054face85d7ea41
> 
> Signed-off-by: Manas <manas18244@iiitd.ac.in>
> Reported-by: syzbot+e8eff054face85d7ea41@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=e8eff054face85d7ea41
> ---
> This syzbot bug[1] is triggered due to the BUG_ON assertions added in
> __bch2_dev_btree_bitmap_mark. During runtime, m->btree_bitmap_shift is
> 63 '?'. This triggers both the assertions.
> 
> I am unfamiliar with the codebase, and there wasn't a lore discussion
> about the assertions in the commit, so I am unsure about the relevance
> of these assertions.
> 
> Reverting the commit does not reproduce the said bug.
> 
> [1] https://syzkaller.appspot.com/bug?extid=e8eff054face85d7ea41
> ---
>  fs/bcachefs/sb-members.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/fs/bcachefs/sb-members.c b/fs/bcachefs/sb-members.c
> index fb08dd680dacf82bca414f424024e4a00bf432de..9790fd47338c46d2af30547e1f41a1e578b71aa4 100644
> --- a/fs/bcachefs/sb-members.c
> +++ b/fs/bcachefs/sb-members.c
> @@ -450,9 +450,6 @@ static void __bch2_dev_btree_bitmap_mark(struct bch_sb_field_members_v2 *mi, uns
>  		m->btree_bitmap_shift += resize;
>  	}
>  
> -	BUG_ON(m->btree_bitmap_shift > 57);
> -	BUG_ON(end > 64ULL << m->btree_bitmap_shift);
> -
>  	for (unsigned bit = start >> m->btree_bitmap_shift;
>  	     (u64) bit << m->btree_bitmap_shift < end;
>  	     bit++)
> 
> ---
> base-commit: 42f7652d3eb527d03665b09edac47f85fb600924
> change-id: 20241021-revert-assert-bch2-25474fe809d1
> 
> Best regards,
> -- 
> Manas <manas18244@iiitd.ac.in>
> 
> 

Re: [PATCH] Revert "bcachefs: Add asserts to bch2_dev_btree_bitmap_marked_sectors()"

[John Stoffel]

>>>>> "Kent" == Kent Overstreet <kent.overstreet@linux.dev> writes:

> On Mon, Oct 21, 2024 at 10:18:57PM +0530, Manas via B4 Relay wrote:
>> From: Manas <manas18244@iiitd.ac.in>
>> 
>> This reverts commit 60f2b1bcf519416dbffee219132aa949d0c39d0e.
>> 
>> This syzbot bug[1] is triggered due to the BUG_ON assertions added in
>> __bch2_dev_btree_bitmap_mark. During runtime, m->btree_bitmap_shift is
>> 63 '?'. This triggers both the assertions.

> The BUG_ON() doesn't need to be deleted; we just need to fix the
> validation so it doesn't fire (it doesn't particularly matter if it's
> removed or not, ubsan would catch it without the BUG_ON()).

Shouldn't the BUG_ON() be replaced with making the filesystem readonly
instead if you're going to keep it?  I'd rather have the filesystem
still be mounted and able to be read, but not writeable, instead of
having my system crash before I can do anything.  

John

Re: [PATCH] Revert "bcachefs: Add asserts to bch2_dev_btree_bitmap_marked_sectors()"

[Kent Overstreet]

On Thu, Oct 24, 2024 at 02:30:34PM -0400, John Stoffel wrote:
> >>>>> "Kent" == Kent Overstreet <kent.overstreet@linux.dev> writes:
> 
> > On Mon, Oct 21, 2024 at 10:18:57PM +0530, Manas via B4 Relay wrote:
> >> From: Manas <manas18244@iiitd.ac.in>
> >> 
> >> This reverts commit 60f2b1bcf519416dbffee219132aa949d0c39d0e.
> >> 
> >> This syzbot bug[1] is triggered due to the BUG_ON assertions added in
> >> __bch2_dev_btree_bitmap_mark. During runtime, m->btree_bitmap_shift is
> >> 63 '?'. This triggers both the assertions.
> 
> > The BUG_ON() doesn't need to be deleted; we just need to fix the
> > validation so it doesn't fire (it doesn't particularly matter if it's
> > removed or not, ubsan would catch it without the BUG_ON()).
> 
> Shouldn't the BUG_ON() be replaced with making the filesystem readonly
> instead if you're going to keep it?  I'd rather have the filesystem
> still be mounted and able to be read, but not writeable, instead of
> having my system crash before I can do anything.  

Not in this case. In general, if there's a chance of the BUG_ON()
hitting in the wild after the code has passed testing then it shouldn't
be a BUG_ON(), but this is low level validation that we're relying on.

In general higher level code absolutely requires that the low level
validation is correct, because if it's not that will trigger all sorts
of undefined behaviour in the upper layers.

By "low level validation" I mean _only_ the validate functions that
check simple invariants within a single data type that is read or
written atomically to disk - "is data type garbage or not".

If it's an invariant that involves multiple objects - "does this extent
refer to a valid device/snapshot ID" - that's not something we can check
in validate, because then an extent or what have you would become
invalid depending on what happens in the rest of the filesystem. Those
sorts of checks do in fact need proper error paths.

Re: [PATCH] Revert "bcachefs: Add asserts to bch2_dev_btree_bitmap_marked_sectors()"

[John Stoffel]

>>>>> "Kent" == Kent Overstreet <kent.overstreet@linux.dev> writes:

> On Thu, Oct 24, 2024 at 02:30:34PM -0400, John Stoffel wrote:
>> >>>>> "Kent" == Kent Overstreet <kent.overstreet@linux.dev> writes:
>> 
>> > On Mon, Oct 21, 2024 at 10:18:57PM +0530, Manas via B4 Relay wrote:
>> >> From: Manas <manas18244@iiitd.ac.in>
>> >> 
>> >> This reverts commit 60f2b1bcf519416dbffee219132aa949d0c39d0e.
>> >> 
>> >> This syzbot bug[1] is triggered due to the BUG_ON assertions added in
>> >> __bch2_dev_btree_bitmap_mark. During runtime, m->btree_bitmap_shift is
>> >> 63 '?'. This triggers both the assertions.
>> 
>> > The BUG_ON() doesn't need to be deleted; we just need to fix the
>> > validation so it doesn't fire (it doesn't particularly matter if it's
>> > removed or not, ubsan would catch it without the BUG_ON()).
>> 
>> Shouldn't the BUG_ON() be replaced with making the filesystem readonly
>> instead if you're going to keep it?  I'd rather have the filesystem
>> still be mounted and able to be read, but not writeable, instead of
>> having my system crash before I can do anything.  

> Not in this case. In general, if there's a chance of the BUG_ON()
> hitting in the wild after the code has passed testing then it
> shouldn't be a BUG_ON(), but this is low level validation that we're
> relying on.

So I'm having a hard time parsing this reply.  And I don't think you
make a good case for leaving or even having a BUG_ON() here at all.
If there's a chance of it hitting in the wild, it should be removed.
But you don't want to remove it because it will never hit?  That's
just lazy... :-)   I just don't see why a filesystem should have the
opportunity to kill an entire system because that one filesystem has
problems.  

> In general higher level code absolutely requires that the low level
> validation is correct, because if it's not that will trigger all sorts
> of undefined behaviour in the upper layers.

> By "low level validation" I mean _only_ the validate functions that
> check simple invariants within a single data type that is read or
> written atomically to disk - "is data type garbage or not".

Sure.

> If it's an invariant that involves multiple objects - "does this
> extent refer to a valid device/snapshot ID" - that's not something
> we can check in validate, because then an extent or what have you
> would become invalid depending on what happens in the rest of the
> filesystem. Those sorts of checks do in fact need proper error
> paths.

Right. 

Re: [PATCH] Revert "bcachefs: Add asserts to bch2_dev_btree_bitmap_marked_sectors()"

[Kent Overstreet]

On Fri, Oct 25, 2024 at 05:26:19PM -0400, John Stoffel wrote:
> >>>>> "Kent" == Kent Overstreet <kent.overstreet@linux.dev> writes:
> 
> > On Thu, Oct 24, 2024 at 02:30:34PM -0400, John Stoffel wrote:
> >> >>>>> "Kent" == Kent Overstreet <kent.overstreet@linux.dev> writes:
> >> 
> >> > On Mon, Oct 21, 2024 at 10:18:57PM +0530, Manas via B4 Relay wrote:
> >> >> From: Manas <manas18244@iiitd.ac.in>
> >> >> 
> >> >> This reverts commit 60f2b1bcf519416dbffee219132aa949d0c39d0e.
> >> >> 
> >> >> This syzbot bug[1] is triggered due to the BUG_ON assertions added in
> >> >> __bch2_dev_btree_bitmap_mark. During runtime, m->btree_bitmap_shift is
> >> >> 63 '?'. This triggers both the assertions.
> >> 
> >> > The BUG_ON() doesn't need to be deleted; we just need to fix the
> >> > validation so it doesn't fire (it doesn't particularly matter if it's
> >> > removed or not, ubsan would catch it without the BUG_ON()).
> >> 
> >> Shouldn't the BUG_ON() be replaced with making the filesystem readonly
> >> instead if you're going to keep it?  I'd rather have the filesystem
> >> still be mounted and able to be read, but not writeable, instead of
> >> having my system crash before I can do anything.  
> 
> > Not in this case. In general, if there's a chance of the BUG_ON()
> > hitting in the wild after the code has passed testing then it
> > shouldn't be a BUG_ON(), but this is low level validation that we're
> > relying on.
> 
> So I'm having a hard time parsing this reply.  And I don't think you
> make a good case for leaving or even having a BUG_ON() here at all.
> If there's a chance of it hitting in the wild, it should be removed.
> But you don't want to remove it because it will never hit?  That's
> just lazy... :-)   I just don't see why a filesystem should have the
> opportunity to kill an entire system because that one filesystem has
> problems.  

No, it's not lazy.

Code with assertions is much more robust and reliable, because bugs
shake out faster. An assertion pop is _much, much_ easier to debug than
wandering off into undefined behaviour land.

Assertions are also documentation that can't go out of date.

They make later refactoring much safer and easier, as well.