[PATCH] bcachefs: Use uncompressed_size as the operand

[PATCH] bcachefs: Use uncompressed_size as the operand

[Alan Huang]

Since we don't merge compressed extents, uncompressed_size should be
equal to compressed_size, use uncompressed_size to clarify the intention.

Signed-off-by: Alan Huang <mmpgouride@gmail.com>
---
 fs/bcachefs/extents.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/bcachefs/extents.c b/fs/bcachefs/extents.c
index 05d5f71a7ca9..eeea23d09b9a 100644
--- a/fs/bcachefs/extents.c
+++ b/fs/bcachefs/extents.c
@@ -326,7 +326,7 @@ bool bch2_extent_merge(struct bch_fs *c, struct bkey_s l, struct bkey_s_c r)
 			    !bch2_checksum_mergeable(lp.crc.csum_type))
 				return false;
 
-			if (lp.crc.offset + lp.crc.live_size != lp.crc.compressed_size ||
+			if (lp.crc.offset + lp.crc.live_size != lp.crc.uncompressed_size ||
 			    rp.crc.offset)
 				return false;
 
-- 
2.47.0

[PATCH] bcachefs: Fix use after free

[Alan Huang]

acc->k.data should be used with the lock hold:

00221 ========= TEST   generic/187
00221        run fstests generic/187 at 2025-02-09 21:08:10
00221 spectre-v4 mitigation disabled by command-line option
00222 bcachefs (vdc): starting version 1.20: directory_size opts=errors=ro
00222 bcachefs (vdc): initializing new filesystem
00222 bcachefs (vdc): going read-write
00222 bcachefs (vdc): marking superblocks
00222 bcachefs (vdc): initializing freespace
00222 bcachefs (vdc): done initializing freespace
00222 bcachefs (vdc): reading snapshots table
00222 bcachefs (vdc): reading snapshots done
00222 bcachefs (vdc): done starting filesystem
00222 bcachefs (vdc): shutting down
00222 bcachefs (vdc): going read-only
00222 bcachefs (vdc): finished waiting for writes to stop
00223 bcachefs (vdc): flushing journal and stopping allocators, journal seq 6
00223 bcachefs (vdc): flushing journal and stopping allocators complete, journal seq 8
00223 bcachefs (vdc): clean shutdown complete, journal seq 9
00223 bcachefs (vdc): marking filesystem clean
00223 bcachefs (vdc): shutdown complete
00223 bcachefs (vdc): starting version 1.20: directory_size opts=errors=ro
00223 bcachefs (vdc): initializing new filesystem
00223 bcachefs (vdc): going read-write
00223 bcachefs (vdc): marking superblocks
00223 bcachefs (vdc): initializing freespace
00223 bcachefs (vdc): done initializing freespace
00223 bcachefs (vdc): reading snapshots table
00223 bcachefs (vdc): reading snapshots done
00223 bcachefs (vdc): done starting filesystem
00244 hrtimer: interrupt took 123350440 ns
00264 bcachefs (vdc): shutting down
00264 bcachefs (vdc): going read-only
00264 bcachefs (vdc): finished waiting for writes to stop
00264 bcachefs (vdc): flushing journal and stopping allocators, journal seq 97
00265 bcachefs (vdc): flushing journal and stopping allocators complete, journal seq 101
00265 bcachefs (vdc): clean shutdown complete, journal seq 102
00265 bcachefs (vdc): marking filesystem clean
00265 bcachefs (vdc): shutdown complete
00265 bcachefs (vdc): starting version 1.20: directory_size opts=errors=ro
00265 bcachefs (vdc): recovering from clean shutdown, journal seq 102
00265 bcachefs (vdc): accounting_read...
00265 ==================================================================
00265  done
00265 BUG: KASAN: slab-use-after-free in bch2_fs_to_text+0x12b4/0x1728
00265 bcachefs (vdc): alloc_read... done
00265 bcachefs (vdc): stripes_read... done
00265 Read of size 4 at addr ffffff80c57eac00 by task cat/7531
00265 bcachefs (vdc): snapshots_read... done
00265
00265 CPU: 6 UID: 0 PID: 7531 Comm: cat Not tainted 6.13.0-rc3-ktest-g16fc6fa3819d #14103
00265 Hardware name: linux,dummy-virt (DT)
00265 Call trace:
00265  show_stack+0x1c/0x30 (C)
00265  dump_stack_lvl+0x6c/0x80
00265  print_report+0xf8/0x5d8
00265  kasan_report+0x90/0xd0
00265  __asan_report_load4_noabort+0x1c/0x28
00265  bch2_fs_to_text+0x12b4/0x1728
00265  bch2_fs_show+0x94/0x188
00265  sysfs_kf_seq_show+0x1a4/0x348
00265  kernfs_seq_show+0x12c/0x198
00265  seq_read_iter+0x27c/0xfd0
00265  kernfs_fop_read_iter+0x390/0x4f8
00265  vfs_read+0x480/0x7f0
00265  ksys_read+0xe0/0x1e8
00265  __arm64_sys_read+0x70/0xa8
00265  invoke_syscall.constprop.0+0x74/0x1e8
00265  do_el0_svc+0xc8/0x1c8
00265  el0_svc+0x20/0x60
00265  el0t_64_sync_handler+0x104/0x130
00265  el0t_64_sync+0x154/0x158
00265
00265 Allocated by task 7510:
00265  kasan_save_stack+0x28/0x50
00265  kasan_save_track+0x1c/0x38
00265  kasan_save_alloc_info+0x3c/0x50
00265  __kasan_kmalloc+0xac/0xb0
00265  __kmalloc_node_noprof+0x168/0x348
00265  __kvmalloc_node_noprof+0x20/0x140
00265  __bch2_darray_resize_noprof+0x90/0x1b0
00265  __bch2_accounting_mem_insert+0x76c/0xb08
00265  bch2_accounting_mem_insert+0x224/0x3b8
00265  bch2_accounting_mem_mod_locked+0x480/0xc58
00265  bch2_accounting_read+0xa94/0x3eb8
00265  bch2_run_recovery_pass+0x80/0x178
00265  bch2_run_recovery_passes+0x340/0x698
00265  bch2_fs_recovery+0x1c98/0x2bd8
00265  bch2_fs_start+0x240/0x490
00265  bch2_fs_get_tree+0xe1c/0x1458
00265  vfs_get_tree+0x7c/0x250
00265  path_mount+0xe24/0x1648
00265  __arm64_sys_mount+0x240/0x438
00265  invoke_syscall.constprop.0+0x74/0x1e8
00265  do_el0_svc+0xc8/0x1c8
00265  el0_svc+0x20/0x60
00265  el0t_64_sync_handler+0x104/0x130
00265  el0t_64_sync+0x154/0x158
00265
00265 Freed by task 7510:
00265  kasan_save_stack+0x28/0x50
00265  kasan_save_track+0x1c/0x38
00265  kasan_save_free_info+0x48/0x88
00265  __kasan_slab_free+0x48/0x60
00265  kfree+0x188/0x408
00265  kvfree+0x3c/0x50
00265  __bch2_darray_resize_noprof+0xe0/0x1b0
00265  __bch2_accounting_mem_insert+0x76c/0xb08
00265  bch2_accounting_mem_insert+0x224/0x3b8
00265  bch2_accounting_mem_mod_locked+0x480/0xc58
00265  bch2_accounting_read+0xa94/0x3eb8
00265  bch2_run_recovery_pass+0x80/0x178
00265  bch2_run_recovery_passes+0x340/0x698
00265  bch2_fs_recovery+0x1c98/0x2bd8
00265  bch2_fs_start+0x240/0x490
00265  bch2_fs_get_tree+0xe1c/0x1458
00265  vfs_get_tree+0x7c/0x250
00265  path_mount+0xe24/0x1648
00265 bcachefs (vdc): going read-write
00265  __arm64_sys_mount+0x240/0x438
00265  invoke_syscall.constprop.0+0x74/0x1e8
00265  do_el0_svc+0xc8/0x1c8
00265  el0_svc+0x20/0x60
00265  el0t_64_sync_handler+0x104/0x130
00265  el0t_64_sync+0x154/0x158
00265
00265 The buggy address belongs to the object at ffffff80c57eac00
00265  which belongs to the cache kmalloc-128 of size 128
00265 The buggy address is located 0 bytes inside of
00265  freed 128-byte region [ffffff80c57eac00, ffffff80c57eac80)
00265
00265 The buggy address belongs to the physical page:
00265 page: refcount:1 mapcount:0 mapping:0000000000000000 index:0x0 pfn:0x1057ea
00265 head: order:1 mapcount:0 entire_mapcount:0 nr_pages_mapped:0 pincount:0
00265 flags: 0x8000000000000040(head|zone=2)
00265 page_type: f5(slab)
00265 raw: 8000000000000040 ffffff80c0002800 dead000000000100 dead000000000122
00265 raw: 0000000000000000 0000000000200020 00000001f5000000 ffffff80c57a6400
00265 head: 8000000000000040 ffffff80c0002800 dead000000000100 dead000000000122
00265 head: 0000000000000000 0000000000200020 00000001f5000000 ffffff80c57a6400
00265 head: 8000000000000001 fffffffec315fa81 ffffffffffffffff 0000000000000000
00265 head: 0000000000000002 0000000000000000 00000000ffffffff 0000000000000000
00265 page dumped because: kasan: bad access detected
00265
00265 Memory state around the buggy address:
00265  ffffff80c57eab00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00265  ffffff80c57eab80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
00265 >ffffff80c57eac00: fa fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
00265                    ^
00265  ffffff80c57eac80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
00265  ffffff80c57ead00: 00 00 00 00 00 00 00 00 00 00 00 00 00 fc fc fc
00265 ==================================================================
00265 Kernel panic - not syncing: kasan.fault=panic set ...
00265 CPU: 6 UID: 0 PID: 7531 Comm: cat Not tainted 6.13.0-rc3-ktest-g16fc6fa3819d #14103
00265 Hardware name: linux,dummy-virt (DT)
00265 Call trace:
00265  show_stack+0x1c/0x30 (C)
00265  dump_stack_lvl+0x30/0x80
00265  dump_stack+0x18/0x20
00265  panic+0x4d4/0x518
00265  start_report.constprop.0+0x0/0x90
00265  kasan_report+0xa0/0xd0
00265  __asan_report_load4_noabort+0x1c/0x28
00265  bch2_fs_to_text+0x12b4/0x1728
00265  bch2_fs_show+0x94/0x188
00265  sysfs_kf_seq_show+0x1a4/0x348
00265  kernfs_seq_show+0x12c/0x198
00265  seq_read_iter+0x27c/0xfd0
00265  kernfs_fop_read_iter+0x390/0x4f8
00265  vfs_read+0x480/0x7f0
00265  ksys_read+0xe0/0x1e8
00265  __arm64_sys_read+0x70/0xa8
00265  invoke_syscall.constprop.0+0x74/0x1e8
00265  do_el0_svc+0xc8/0x1c8
00265  el0_svc+0x20/0x60
00265  el0t_64_sync_handler+0x104/0x130
00265  el0t_64_sync+0x154/0x158
00265 SMP: stopping secondary CPUs
00265 Kernel Offset: disabled
00265 CPU features: 0x000,00000070,00000010,8240500b
00265 Memory Limit: none
00265 ---[ end Kernel panic - not syncing: kasan.fault=panic set ... ]---
00270 ========= FAILED TIMEOUT generic.187 in 1200s

Signed-off-by: Alan Huang <mmpgouride@gmail.com>
---
 fs/bcachefs/disk_accounting.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/bcachefs/disk_accounting.h b/fs/bcachefs/disk_accounting.h
index 5360cbb3ec29..f4372cafea2e 100644
--- a/fs/bcachefs/disk_accounting.h
+++ b/fs/bcachefs/disk_accounting.h
@@ -210,11 +210,13 @@ static inline void bch2_accounting_mem_read_counters(struct bch_accounting_mem *
 static inline void bch2_accounting_mem_read(struct bch_fs *c, struct bpos p,
 					    u64 *v, unsigned nr)
 {
+	percpu_down_read(&c->mark_lock);
 	struct bch_accounting_mem *acc = &c->accounting;
 	unsigned idx = eytzinger0_find(acc->k.data, acc->k.nr, sizeof(acc->k.data[0]),
 				       accounting_pos_cmp, &p);
 
 	bch2_accounting_mem_read_counters(acc, idx, v, nr, false);
+	percpu_up_read(&c->mark_lock);
 }
 
 static inline struct bversion journal_pos_to_bversion(struct journal_res *res, unsigned offset)
-- 
2.47.0

Re: [PATCH] bcachefs: Fix use after free

[Kent Overstreet]

On Mon, Feb 10, 2025 at 11:04:22AM +0800, Alan Huang wrote:
> acc->k.data should be used with the lock hold:

<snipped>

> 
> Signed-off-by: Alan Huang <mmpgouride@gmail.com>


Test this with lockdep (you know how to edit the lists of tests the CI
runs, right?

mark_lock usage is a giant mess, and calling percpu_down_read()
recursively by accident will mostly work, until it doesn't.

> ---
>  fs/bcachefs/disk_accounting.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/bcachefs/disk_accounting.h b/fs/bcachefs/disk_accounting.h
> index 5360cbb3ec29..f4372cafea2e 100644
> --- a/fs/bcachefs/disk_accounting.h
> +++ b/fs/bcachefs/disk_accounting.h
> @@ -210,11 +210,13 @@ static inline void bch2_accounting_mem_read_counters(struct bch_accounting_mem *
>  static inline void bch2_accounting_mem_read(struct bch_fs *c, struct bpos p,
>  					    u64 *v, unsigned nr)
>  {
> +	percpu_down_read(&c->mark_lock);
>  	struct bch_accounting_mem *acc = &c->accounting;
>  	unsigned idx = eytzinger0_find(acc->k.data, acc->k.nr, sizeof(acc->k.data[0]),
>  				       accounting_pos_cmp, &p);
>  
>  	bch2_accounting_mem_read_counters(acc, idx, v, nr, false);
> +	percpu_up_read(&c->mark_lock);
>  }
>  
>  static inline struct bversion journal_pos_to_bversion(struct journal_res *res, unsigned offset)
> -- 
> 2.47.0
> 

Re: [PATCH] bcachefs: Fix use after free

[Alan Huang]

On Feb 11, 2025, at 00:28, Kent Overstreet <kent.overstreet@linux.dev> wrote:
> 
> On Mon, Feb 10, 2025 at 11:04:22AM +0800, Alan Huang wrote:
>> acc->k.data should be used with the lock hold:
> 
> <snipped>
> 
>> 
>> Signed-off-by: Alan Huang <mmpgouride@gmail.com>
> 
> 
> Test this with lockdep (you know how to edit the lists of tests the CI
> runs, right?

The lockdep test group is in the tests lists.

Or you mean add tests in the lockdep group ? 

> 
> mark_lock usage is a giant mess, and calling percpu_down_read()
> recursively by accident will mostly work, until it doesn't.
> 
>> ---
>> fs/bcachefs/disk_accounting.h | 2 ++
>> 1 file changed, 2 insertions(+)
>> 
>> diff --git a/fs/bcachefs/disk_accounting.h b/fs/bcachefs/disk_accounting.h
>> index 5360cbb3ec29..f4372cafea2e 100644
>> --- a/fs/bcachefs/disk_accounting.h
>> +++ b/fs/bcachefs/disk_accounting.h
>> @@ -210,11 +210,13 @@ static inline void bch2_accounting_mem_read_counters(struct bch_accounting_mem *
>> static inline void bch2_accounting_mem_read(struct bch_fs *c, struct bpos p,
>>    u64 *v, unsigned nr)
>> {
>> + percpu_down_read(&c->mark_lock);
>> struct bch_accounting_mem *acc = &c->accounting;
>> unsigned idx = eytzinger0_find(acc->k.data, acc->k.nr, sizeof(acc->k.data[0]),
>>       accounting_pos_cmp, &p);
>> 
>> bch2_accounting_mem_read_counters(acc, idx, v, nr, false);
>> + percpu_up_read(&c->mark_lock);
>> }
>> 
>> static inline struct bversion journal_pos_to_bversion(struct journal_res *res, unsigned offset)
>> -- 
>> 2.47.0
>> 

Re: [PATCH] bcachefs: Fix use after free

[Kent Overstreet]

On Tue, Feb 11, 2025 at 12:54:29AM +0800, Alan Huang wrote:
> On Feb 11, 2025, at 00:28, Kent Overstreet <kent.overstreet@linux.dev> wrote:
> > 
> > On Mon, Feb 10, 2025 at 11:04:22AM +0800, Alan Huang wrote:
> >> acc->k.data should be used with the lock hold:
> > 
> > <snipped>
> > 
> >> 
> >> Signed-off-by: Alan Huang <mmpgouride@gmail.com>
> > 
> > 
> > Test this with lockdep (you know how to edit the lists of tests the CI
> > runs, right?
> 
> The lockdep test group is in the tests lists.
> 
> Or you mean add tests in the lockdep group ? 

Ah ok, you're already running them - I'm applying this.

Re: [PATCH] bcachefs: Use uncompressed_size as the operand

[Kent Overstreet]

On Mon, Feb 10, 2025 at 11:04:21AM +0800, Alan Huang wrote:
> Since we don't merge compressed extents, uncompressed_size should be
> equal to compressed_size, use uncompressed_size to clarify the intention.

compressed_size actually describes the intention better here, it refers
to size ondisk, and we're checking if the extents are contiguous on disk

> 
> Signed-off-by: Alan Huang <mmpgouride@gmail.com>
> ---
>  fs/bcachefs/extents.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/bcachefs/extents.c b/fs/bcachefs/extents.c
> index 05d5f71a7ca9..eeea23d09b9a 100644
> --- a/fs/bcachefs/extents.c
> +++ b/fs/bcachefs/extents.c
> @@ -326,7 +326,7 @@ bool bch2_extent_merge(struct bch_fs *c, struct bkey_s l, struct bkey_s_c r)
>  			    !bch2_checksum_mergeable(lp.crc.csum_type))
>  				return false;
>  
> -			if (lp.crc.offset + lp.crc.live_size != lp.crc.compressed_size ||
> +			if (lp.crc.offset + lp.crc.live_size != lp.crc.uncompressed_size ||
>  			    rp.crc.offset)
>  				return false;
>  
> -- 
> 2.47.0
> 

Re: [PATCH] bcachefs: Use uncompressed_size as the operand

[Alan Huang]

On Feb 11, 2025, at 00:25, Kent Overstreet <kent.overstreet@linux.dev> wrote:
> 
> On Mon, Feb 10, 2025 at 11:04:21AM +0800, Alan Huang wrote:
>> Since we don't merge compressed extents, uncompressed_size should be
>> equal to compressed_size, use uncompressed_size to clarify the intention.
> 
> compressed_size actually describes the intention better here, it refers
> to size ondisk, and we're checking if the extents are contiguous on disk

But we don’t merge compressed extents.

There is is_compressed check above (below compressed_size check in the past as the history says)

Not sure if it’s my problem, but I spent hours before figuring out that we're checking if the extents are contiguous on disk.

> 
>> 
>> Signed-off-by: Alan Huang <mmpgouride@gmail.com>
>> ---
>> fs/bcachefs/extents.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/fs/bcachefs/extents.c b/fs/bcachefs/extents.c
>> index 05d5f71a7ca9..eeea23d09b9a 100644
>> --- a/fs/bcachefs/extents.c
>> +++ b/fs/bcachefs/extents.c
>> @@ -326,7 +326,7 @@ bool bch2_extent_merge(struct bch_fs *c, struct bkey_s l, struct bkey_s_c r)
>>    !bch2_checksum_mergeable(lp.crc.csum_type))
>> return false;
>> 
>> - if (lp.crc.offset + lp.crc.live_size != lp.crc.compressed_size ||
>> + if (lp.crc.offset + lp.crc.live_size != lp.crc.uncompressed_size ||
>>    rp.crc.offset)
>> return false;
>> 
>> -- 
>> 2.47.0
>> 

Re: [PATCH] bcachefs: Use uncompressed_size as the operand

[Kent Overstreet]

On Tue, Feb 11, 2025 at 01:02:28AM +0800, Alan Huang wrote:
> On Feb 11, 2025, at 00:25, Kent Overstreet <kent.overstreet@linux.dev> wrote:
> > 
> > On Mon, Feb 10, 2025 at 11:04:21AM +0800, Alan Huang wrote:
> >> Since we don't merge compressed extents, uncompressed_size should be
> >> equal to compressed_size, use uncompressed_size to clarify the intention.
> > 
> > compressed_size actually describes the intention better here, it refers
> > to size ondisk, and we're checking if the extents are contiguous on disk
> 
> But we don’t merge compressed extents.
> 
> There is is_compressed check above (below compressed_size check in the past as the history says)
> 
> Not sure if it’s my problem, but I spent hours before figuring out that we're checking if the extents are contiguous on disk.

You're also checking p.ptr.offset, which is the offset on disk...