[PATCH] bcachefs: WARNING in bch2_fs_journal_stop

[PATCH] bcachefs: WARNING in bch2_fs_journal_stop

[Camila Alvarez]

last_seq and last_empty_seq suffered from an off by one error when the
journal has no entries.

The indexes were fixed and an assertion is added to check that the
last_empty_seq is always kept under the next valid seq number.

Reported-by: syzbot+4093905737cf289b6b38@syzkaller.appspotmail.com
Signed-off-by: Camila Alvarez <cam.alvarez.i@gmail.com>
---
 fs/bcachefs/journal.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/fs/bcachefs/journal.c b/fs/bcachefs/journal.c
index 10b19791ec98..7bbbf4b149e9 100644
--- a/fs/bcachefs/journal.c
+++ b/fs/bcachefs/journal.c
@@ -1201,7 +1201,7 @@ int bch2_fs_journal_start(struct journal *j, u64 cur_seq)
 	struct journal_replay *i, **_i;
 	struct genradix_iter iter;
 	bool had_entries = false;
-	u64 last_seq = cur_seq, nr, seq;
+        u64 last_written_seq = cur_seq - 1, last_seq = cur_seq - 1, nr, seq;
 
 	genradix_for_each_reverse(&c->journal_entries, iter, _i) {
 		i = *_i;
@@ -1209,11 +1209,11 @@ int bch2_fs_journal_start(struct journal *j, u64 cur_seq)
 		if (journal_replay_ignore(i))
 			continue;
 
-		last_seq = le64_to_cpu(i->j.last_seq);
+		last_written_seq = le64_to_cpu(i->j.last_seq);
 		break;
 	}
 
-	nr = cur_seq - last_seq;
+	nr = cur_seq - last_written_seq;
 
 	if (nr + 1 > j->pin.size) {
 		free_fifo(&j->pin);
@@ -1224,14 +1224,14 @@ int bch2_fs_journal_start(struct journal *j, u64 cur_seq)
 		}
 	}
 
-	j->replay_journal_seq	= last_seq;
+	j->replay_journal_seq	= last_written_seq;
 	j->replay_journal_seq_end = cur_seq;
-	j->last_seq_ondisk	= last_seq;
-	j->flushed_seq_ondisk	= cur_seq - 1;
-	j->seq_ondisk		= cur_seq - 1;
-	j->pin.front		= last_seq;
+	j->last_seq_ondisk	= last_written_seq;
+	j->flushed_seq_ondisk	= last_seq;
+	j->seq_ondisk		= last_seq;
+	j->pin.front		= last_written_seq;
 	j->pin.back		= cur_seq;
-	atomic64_set(&j->seq, cur_seq - 1);
+	atomic64_set(&j->seq, last_seq);
 
 	fifo_for_each_entry_ptr(p, &j->pin, seq)
 		journal_pin_list_init(p, 1);
@@ -1261,7 +1261,10 @@ int bch2_fs_journal_start(struct journal *j, u64 cur_seq)
 	}
 
 	if (!had_entries)
-		j->last_empty_seq = cur_seq;
+		j->last_empty_seq = last_seq;
+
+	WARN(j->last_empty_seq >= cur_seq, "journal startup error: last empty seq %llu is higher or equal than the next seq number to be used (%llu)",
+	      j->last_empty_seq, cur_seq);
 
 	spin_lock(&j->lock);
 
-- 
2.34.1

Re: [PATCH] bcachefs: WARNING in bch2_fs_journal_stop

[AP]

On Wed, Jul 17, 2024 at 06:02:39PM -0400, Camila Alvarez wrote:
> diff --git a/fs/bcachefs/journal.c b/fs/bcachefs/journal.c
> index 10b19791ec98..7bbbf4b149e9 100644
> --- a/fs/bcachefs/journal.c
> +++ b/fs/bcachefs/journal.c
> @@ -1201,7 +1201,7 @@ int bch2_fs_journal_start(struct journal *j, u64 cur_seq)
>  	struct journal_replay *i, **_i;
>  	struct genradix_iter iter;
>  	bool had_entries = false;
> -	u64 last_seq = cur_seq, nr, seq;
> +        u64 last_written_seq = cur_seq - 1, last_seq = cur_seq - 1, nr, seq;

Spaces not tabs. I think consistency would be wanted so should be a tab.

AP

Re: [PATCH] bcachefs: WARNING in bch2_fs_journal_stop

[Kent Overstreet]

On Wed, Jul 17, 2024 at 06:02:39PM GMT, Camila Alvarez wrote:
> last_seq and last_empty_seq suffered from an off by one error when the
> journal has no entries.
> 
> The indexes were fixed and an assertion is added to check that the
> last_empty_seq is always kept under the next valid seq number.

oh nice. I'm going to need to stare at this some more, I still feel like
this code needs to be clearer and less fiddly.

> 
> Reported-by: syzbot+4093905737cf289b6b38@syzkaller.appspotmail.com
> Signed-off-by: Camila Alvarez <cam.alvarez.i@gmail.com>
> ---
>  fs/bcachefs/journal.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/bcachefs/journal.c b/fs/bcachefs/journal.c
> index 10b19791ec98..7bbbf4b149e9 100644
> --- a/fs/bcachefs/journal.c
> +++ b/fs/bcachefs/journal.c
> @@ -1201,7 +1201,7 @@ int bch2_fs_journal_start(struct journal *j, u64 cur_seq)
>  	struct journal_replay *i, **_i;
>  	struct genradix_iter iter;
>  	bool had_entries = false;
> -	u64 last_seq = cur_seq, nr, seq;
> +        u64 last_written_seq = cur_seq - 1, last_seq = cur_seq - 1, nr, seq;
>  
>  	genradix_for_each_reverse(&c->journal_entries, iter, _i) {
>  		i = *_i;
> @@ -1209,11 +1209,11 @@ int bch2_fs_journal_start(struct journal *j, u64 cur_seq)
>  		if (journal_replay_ignore(i))
>  			continue;
>  
> -		last_seq = le64_to_cpu(i->j.last_seq);
> +		last_written_seq = le64_to_cpu(i->j.last_seq);
>  		break;
>  	}
>  
> -	nr = cur_seq - last_seq;
> +	nr = cur_seq - last_written_seq;
>  
>  	if (nr + 1 > j->pin.size) {
>  		free_fifo(&j->pin);
> @@ -1224,14 +1224,14 @@ int bch2_fs_journal_start(struct journal *j, u64 cur_seq)
>  		}
>  	}
>  
> -	j->replay_journal_seq	= last_seq;
> +	j->replay_journal_seq	= last_written_seq;
>  	j->replay_journal_seq_end = cur_seq;
> -	j->last_seq_ondisk	= last_seq;
> -	j->flushed_seq_ondisk	= cur_seq - 1;
> -	j->seq_ondisk		= cur_seq - 1;
> -	j->pin.front		= last_seq;
> +	j->last_seq_ondisk	= last_written_seq;
> +	j->flushed_seq_ondisk	= last_seq;
> +	j->seq_ondisk		= last_seq;
> +	j->pin.front		= last_written_seq;
>  	j->pin.back		= cur_seq;
> -	atomic64_set(&j->seq, cur_seq - 1);
> +	atomic64_set(&j->seq, last_seq);
>  
>  	fifo_for_each_entry_ptr(p, &j->pin, seq)
>  		journal_pin_list_init(p, 1);
> @@ -1261,7 +1261,10 @@ int bch2_fs_journal_start(struct journal *j, u64 cur_seq)
>  	}
>  
>  	if (!had_entries)
> -		j->last_empty_seq = cur_seq;
> +		j->last_empty_seq = last_seq;
> +
> +	WARN(j->last_empty_seq >= cur_seq, "journal startup error: last empty seq %llu is higher or equal than the next seq number to be used (%llu)",
> +	      j->last_empty_seq, cur_seq);
>  
>  	spin_lock(&j->lock);
>  
> -- 
> 2.34.1
> 

Re: [PATCH] bcachefs: WARNING in bch2_fs_journal_stop

[Camila Alvarez Inostroza]

On Thu, 18 Jul 2024, Kent Overstreet wrote:

> On Wed, Jul 17, 2024 at 06:02:39PM GMT, Camila Alvarez wrote:
>> last_seq and last_empty_seq suffered from an off by one error when the
>> journal has no entries.
>>
>> The indexes were fixed and an assertion is added to check that the
>> last_empty_seq is always kept under the next valid seq number.
>
> oh nice. I'm going to need to stare at this some more, I still feel like
> this code needs to be clearer and less fiddly.
>
Sure! Please let me know if there's anything I can do to help.
>>
>> Reported-by: syzbot+4093905737cf289b6b38@syzkaller.appspotmail.com
>> Signed-off-by: Camila Alvarez <cam.alvarez.i@gmail.com>
>> ---
>>  fs/bcachefs/journal.c | 23 +++++++++++++----------
>>  1 file changed, 13 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/bcachefs/journal.c b/fs/bcachefs/journal.c
>> index 10b19791ec98..7bbbf4b149e9 100644
>> --- a/fs/bcachefs/journal.c
>> +++ b/fs/bcachefs/journal.c
>> @@ -1201,7 +1201,7 @@ int bch2_fs_journal_start(struct journal *j, u64 cur_seq)
>>  	struct journal_replay *i, **_i;
>>  	struct genradix_iter iter;
>>  	bool had_entries = false;
>> -	u64 last_seq = cur_seq, nr, seq;
>> +        u64 last_written_seq = cur_seq - 1, last_seq = cur_seq - 1, nr, seq;
>>
>>  	genradix_for_each_reverse(&c->journal_entries, iter, _i) {
>>  		i = *_i;
>> @@ -1209,11 +1209,11 @@ int bch2_fs_journal_start(struct journal *j, u64 cur_seq)
>>  		if (journal_replay_ignore(i))
>>  			continue;
>>
>> -		last_seq = le64_to_cpu(i->j.last_seq);
>> +		last_written_seq = le64_to_cpu(i->j.last_seq);
>>  		break;
>>  	}
>>
>> -	nr = cur_seq - last_seq;
>> +	nr = cur_seq - last_written_seq;
>>
>>  	if (nr + 1 > j->pin.size) {
>>  		free_fifo(&j->pin);
>> @@ -1224,14 +1224,14 @@ int bch2_fs_journal_start(struct journal *j, u64 cur_seq)
>>  		}
>>  	}
>>
>> -	j->replay_journal_seq	= last_seq;
>> +	j->replay_journal_seq	= last_written_seq;
>>  	j->replay_journal_seq_end = cur_seq;
>> -	j->last_seq_ondisk	= last_seq;
>> -	j->flushed_seq_ondisk	= cur_seq - 1;
>> -	j->seq_ondisk		= cur_seq - 1;
>> -	j->pin.front		= last_seq;
>> +	j->last_seq_ondisk	= last_written_seq;
>> +	j->flushed_seq_ondisk	= last_seq;
>> +	j->seq_ondisk		= last_seq;
>> +	j->pin.front		= last_written_seq;
>>  	j->pin.back		= cur_seq;
>> -	atomic64_set(&j->seq, cur_seq - 1);
>> +	atomic64_set(&j->seq, last_seq);
>>
>>  	fifo_for_each_entry_ptr(p, &j->pin, seq)
>>  		journal_pin_list_init(p, 1);
>> @@ -1261,7 +1261,10 @@ int bch2_fs_journal_start(struct journal *j, u64 cur_seq)
>>  	}
>>
>>  	if (!had_entries)
>> -		j->last_empty_seq = cur_seq;
>> +		j->last_empty_seq = last_seq;
>> +
>> +	WARN(j->last_empty_seq >= cur_seq, "journal startup error: last empty seq %llu is higher or equal than the next seq number to be used (%llu)",
>> +	      j->last_empty_seq, cur_seq);
>>
>>  	spin_lock(&j->lock);
>>
>> --
>> 2.34.1
>>
>

Re: [PATCH] bcachefs: WARNING in bch2_fs_journal_stop

[Kent Overstreet]

On Thu, Jul 18, 2024 at 06:42:55PM GMT, Camila Alvarez Inostroza wrote:
> 
> 
> On Thu, 18 Jul 2024, Kent Overstreet wrote:
> 
> > On Wed, Jul 17, 2024 at 06:02:39PM GMT, Camila Alvarez wrote:
> > > last_seq and last_empty_seq suffered from an off by one error when the
> > > journal has no entries.
> > > 
> > > The indexes were fixed and an assertion is added to check that the
> > > last_empty_seq is always kept under the next valid seq number.
> > 
> > oh nice. I'm going to need to stare at this some more, I still feel like
> > this code needs to be clearer and less fiddly.
> > 
> Sure! Please let me know if there's anything I can do to help.

I'm just applying it - the warning you added gave me warm fuzzies.

I'll let you know if anything pops up in the CI (and I should give you
an account so you can point it at your own branches; email me a username
and ssh key)

Re: [PATCH] bcachefs: WARNING in bch2_fs_journal_stop

[Camila Alvarez Inostroza]

On Thu, 18 Jul 2024, Kent Overstreet wrote:

> On Thu, Jul 18, 2024 at 06:42:55PM GMT, Camila Alvarez Inostroza wrote:
>>
>>
>> On Thu, 18 Jul 2024, Kent Overstreet wrote:
>>
>>> On Wed, Jul 17, 2024 at 06:02:39PM GMT, Camila Alvarez wrote:
>>>> last_seq and last_empty_seq suffered from an off by one error when the
>>>> journal has no entries.
>>>>
>>>> The indexes were fixed and an assertion is added to check that the
>>>> last_empty_seq is always kept under the next valid seq number.
>>>
>>> oh nice. I'm going to need to stare at this some more, I still feel like
>>> this code needs to be clearer and less fiddly.
>>>
>> Sure! Please let me know if there's anything I can do to help.
>
> I'm just applying it - the warning you added gave me warm fuzzies.
>
> I'll let you know if anything pops up in the CI (and I should give you
> an account so you can point it at your own branches; email me a username
> and ssh key)
>
Should I give you the info in this same thread or in a separate email?

Re: [PATCH] bcachefs: WARNING in bch2_fs_journal_stop

[Kent Overstreet]

On Thu, Jul 18, 2024 at 06:49:02PM GMT, Camila Alvarez Inostroza wrote:
> 
> 
> On Thu, 18 Jul 2024, Kent Overstreet wrote:
> 
> > On Thu, Jul 18, 2024 at 06:42:55PM GMT, Camila Alvarez Inostroza wrote:
> > > 
> > > 
> > > On Thu, 18 Jul 2024, Kent Overstreet wrote:
> > > 
> > > > On Wed, Jul 17, 2024 at 06:02:39PM GMT, Camila Alvarez wrote:
> > > > > last_seq and last_empty_seq suffered from an off by one error when the
> > > > > journal has no entries.
> > > > > 
> > > > > The indexes were fixed and an assertion is added to check that the
> > > > > last_empty_seq is always kept under the next valid seq number.
> > > > 
> > > > oh nice. I'm going to need to stare at this some more, I still feel like
> > > > this code needs to be clearer and less fiddly.
> > > > 
> > > Sure! Please let me know if there's anything I can do to help.
> > 
> > I'm just applying it - the warning you added gave me warm fuzzies.
> > 
> > I'll let you know if anything pops up in the CI (and I should give you
> > an account so you can point it at your own branches; email me a username
> > and ssh key)
> > 
> Should I give you the info in this same thread or in a separate email?

doesn't matter, ssh pubkeys aren't secrets

Re: [PATCH] bcachefs: WARNING in bch2_fs_journal_stop

[Camila Alvarez Inostroza]

[-- Attachment #1: Type: text/plain, Size: 1300 bytes --]



On Thu, 18 Jul 2024, Kent Overstreet wrote:

> On Thu, Jul 18, 2024 at 06:49:02PM GMT, Camila Alvarez Inostroza wrote:
>>
>>
>> On Thu, 18 Jul 2024, Kent Overstreet wrote:
>>
>>> On Thu, Jul 18, 2024 at 06:42:55PM GMT, Camila Alvarez Inostroza wrote:
>>>>
>>>>
>>>> On Thu, 18 Jul 2024, Kent Overstreet wrote:
>>>>
>>>>> On Wed, Jul 17, 2024 at 06:02:39PM GMT, Camila Alvarez wrote:
>>>>>> last_seq and last_empty_seq suffered from an off by one error when the
>>>>>> journal has no entries.
>>>>>>
>>>>>> The indexes were fixed and an assertion is added to check that the
>>>>>> last_empty_seq is always kept under the next valid seq number.
>>>>>
>>>>> oh nice. I'm going to need to stare at this some more, I still feel like
>>>>> this code needs to be clearer and less fiddly.
>>>>>
>>>> Sure! Please let me know if there's anything I can do to help.
>>>
>>> I'm just applying it - the warning you added gave me warm fuzzies.
>>>
>>> I'll let you know if anything pops up in the CI (and I should give you
>>> an account so you can point it at your own branches; email me a username
>>> and ssh key)
>>>
>> Should I give you the info in this same thread or in a separate email?
>
> doesn't matter, ssh pubkeys aren't secrets
>
I've attached my pubkey. You can use calvarez as my username. Thanks

[-- Attachment #2: Type: application/x-mspublisher, Size: 418 bytes --]

Re: [PATCH] bcachefs: WARNING in bch2_fs_journal_stop

[Camila Alvarez Inostroza]

On Thu, 18 Jul 2024, Camila Alvarez Inostroza wrote:

>
>
> On Thu, 18 Jul 2024, Kent Overstreet wrote:
>
>> On Thu, Jul 18, 2024 at 06:49:02PM GMT, Camila Alvarez Inostroza wrote:
>>> 
>>> 
>>> On Thu, 18 Jul 2024, Kent Overstreet wrote:
>>> 
>>>> On Thu, Jul 18, 2024 at 06:42:55PM GMT, Camila Alvarez Inostroza wrote:
>>>>> 
>>>>> 
>>>>> On Thu, 18 Jul 2024, Kent Overstreet wrote:
>>>>> 
>>>>>> On Wed, Jul 17, 2024 at 06:02:39PM GMT, Camila Alvarez wrote:
>>>>>>> last_seq and last_empty_seq suffered from an off by one error when the
>>>>>>> journal has no entries.
>>>>>>> 
>>>>>>> The indexes were fixed and an assertion is added to check that the
>>>>>>> last_empty_seq is always kept under the next valid seq number.
>>>>>> 
>>>>>> oh nice. I'm going to need to stare at this some more, I still feel 
>>>>>> like
>>>>>> this code needs to be clearer and less fiddly.
>>>>>> 
>>>>> Sure! Please let me know if there's anything I can do to help.
>>>> 
>>>> I'm just applying it - the warning you added gave me warm fuzzies.
>>>> 
>>>> I'll let you know if anything pops up in the CI (and I should give you
>>>> an account so you can point it at your own branches; email me a username
>>>> and ssh key)
>>>> 
>>> Should I give you the info in this same thread or in a separate email?
>> 
>> doesn't matter, ssh pubkeys aren't secrets
>> 
> I've attached my pubkey. You can use calvarez as my username. Thanks
Hi, just wondering if you had any time to take a look at this patch.
Please let me know if you have any comments!