| Message ID | cd9ae501762221ffca5408ffb59f1a3b990de14e.1708339010.git.dsterba@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Static inline cleanups | expand |
On Mon, Feb 19, 2024 at 11:14 AM David Sterba <dsterba@suse.com> wrote: > > The helper is simple enough for inlining, we can further simplify it by > removing the check for map pointer validity. After this patch all > callers always pass a valid pointer. So by making btrfs_free_chunk_map() to not ignore a NULL pointer, we are adding rather surprising behaviour and inconsistency. Most free functions ignore a NULL pointer, take the example of the kfree() family and even free() family in the standard C library, as well as most of the free functions we have in btrfs as well, which are modeled on that common pattern. Ignoring NULL makes error handling simpler, by not having the need to take special care to call the free function with a non-NULL pointer. Besides that, this change doesn't seem to improve anything. Thanks. > > The changes to achieve this: > > - in verify_one_dev_extent() return and don't jump to the out label > that could potentially pass a NULL pointer to btrfs_free_chunk_map > > - in btrfs_load_block_group_zone_info() add a label that specifically > clears the map and does not go through label out that could encounter > a NULL pointer in cache->physical_map > > Signed-off-by: David Sterba <dsterba@suse.com> > --- > fs/btrfs/volumes.c | 3 +-- > fs/btrfs/volumes.h | 2 +- > fs/btrfs/zoned.c | 3 ++- > 3 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c > index 752144a31d79..55b91807aba4 100644 > --- a/fs/btrfs/volumes.c > +++ b/fs/btrfs/volumes.c > @@ -7979,8 +7979,7 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info, > btrfs_err(fs_info, > "dev extent physical offset %llu on devid %llu doesn't have corresponding chunk", > physical_offset, devid); > - ret = -EUCLEAN; > - goto out; > + return -EUCLEAN; > } > > stripe_len = btrfs_calc_stripe_length(map); > diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h > index 21d4de0e3f1f..ce1aa7684c74 100644 > --- a/fs/btrfs/volumes.h > +++ b/fs/btrfs/volumes.h > @@ -566,7 +566,7 @@ struct btrfs_chunk_map { > > static inline void btrfs_free_chunk_map(struct btrfs_chunk_map *map) > { > - if (map && refcount_dec_and_test(&map->refs)) { > + if (refcount_dec_and_test(&map->refs)) { > ASSERT(RB_EMPTY_NODE(&map->rb_node)); > kfree(map); > } > diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c > index 3317bebfca95..b9346ca82c47 100644 > --- a/fs/btrfs/zoned.c > +++ b/fs/btrfs/zoned.c > @@ -1564,7 +1564,7 @@ int btrfs_load_block_group_zone_info(struct btrfs_block_group *cache, bool new) > cache->physical_map = btrfs_clone_chunk_map(map, GFP_NOFS); > if (!cache->physical_map) { > ret = -ENOMEM; > - goto out; > + goto out_free_map; > } > > zone_info = kcalloc(map->num_stripes, sizeof(*zone_info), GFP_NOFS); > @@ -1677,6 +1677,7 @@ int btrfs_load_block_group_zone_info(struct btrfs_block_group *cache, bool new) > } > bitmap_free(active); > kfree(zone_info); > +out_free_map: > btrfs_free_chunk_map(map); > > return ret; > -- > 2.42.1 > >
On Mon, Feb 19, 2024 at 12:27:44PM +0000, Filipe Manana wrote: > On Mon, Feb 19, 2024 at 11:14 AM David Sterba <dsterba@suse.com> wrote: > > > > The helper is simple enough for inlining, we can further simplify it by > > removing the check for map pointer validity. After this patch all > > callers always pass a valid pointer. > > So by making btrfs_free_chunk_map() to not ignore a NULL pointer, we > are adding rather > surprising behaviour and inconsistency. > > Most free functions ignore a NULL pointer, take the example of the > kfree() family and even free() family in the standard C library, > as well as most of the free functions we have in btrfs as well, which > are modeled on that common pattern. > > Ignoring NULL makes error handling simpler, by not having the need to > take special care to call the free function with a non-NULL pointer. > > Besides that, this change doesn't seem to improve anything. The goal is to reduce a static inline function size as its code is duplicated many times (in this case 36x) so anything that does not need to be there is removed. The improvement is smaller code size, one less condition to check. OTOH that it's a freeing function and would not accept a NULL pointer is indeed inconsistent and potentially problematic so I'll drop the patch.
Hello,
kernel test robot noticed "dmesg.BUG:KASAN:null-ptr-deref_in_btrfs_put_block_group" on:
commit: 1511810d056bc04fc0aed7a2b20d09b170da3e86 ("[PATCH 08/10] btrfs: simplify conditions in btrfs_free_chunk_map()")
url: https://github.com/intel-lab-lkp/linux/commits/David-Sterba/btrfs-move-balance-args-conversion-helpers-to-volumes-c/20240219-191714
base: https://git.kernel.org/cgit/linux/kernel/git/kdave/linux.git for-next
patch link: https://lore.kernel.org/all/cd9ae501762221ffca5408ffb59f1a3b990de14e.1708339010.git.dsterba@suse.com/
patch subject: [PATCH 08/10] btrfs: simplify conditions in btrfs_free_chunk_map()
in testcase: xfstests
version: xfstests-x86_64-c46ca4d1-1_20240205
with following parameters:
disk: 4HDD
fs: btrfs
test: generic-group-34
compiler: gcc-12
test machine: 4 threads Intel(R) Core(TM) i5-6500 CPU @ 3.20GHz (Skylake) with 32G memory
(please refer to attached dmesg/kmsg for entire log/backtrace)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202402261652.bcd6d27d-lkp@intel.com
[ 55.292606][ T1454] BTRFS info (device sda1): last unmount of filesystem b71ba2d6-b44f-48b2-b855-8d320c026d64
[ 55.376758][ T1454] ==================================================================
[ 55.384644][ T1454] BUG: KASAN: null-ptr-deref in btrfs_put_block_group+0x15a/0x2c0 [btrfs]
[ 55.393037][ T1454] Write of size 4 at addr 000000000000001c by task umount/1454
[ 55.400400][ T1454]
[ 55.402575][ T1454] CPU: 1 PID: 1454 Comm: umount Tainted: G S I 6.8.0-rc4-00127-g1511810d056b #1
[ 55.412614][ T1454] Hardware name: Dell Inc. OptiPlex 7040/0Y7WYT, BIOS 1.1.1 10/07/2015
[ 55.420665][ T1454] Call Trace:
[ 55.423806][ T1454] <TASK>
[ 55.426586][ T1454] dump_stack_lvl+0x36/0x50
[ 55.430927][ T1454] kasan_report+0xc7/0x100
[ 55.435178][ T1454] ? btrfs_put_block_group+0x15a/0x2c0 [btrfs]
[ 55.441237][ T1454] kasan_check_range+0xfc/0x1a0
[ 55.445928][ T1454] btrfs_put_block_group+0x15a/0x2c0 [btrfs]
[ 55.451831][ T1454] btrfs_free_block_groups+0x7fd/0x10f0 [btrfs]
[ 55.457992][ T1454] ? free_root_pointers+0x759/0xa10 [btrfs]
[ 55.463785][ T1454] close_ctree+0x87c/0xcf0 [btrfs]
[ 55.468842][ T1454] ? _btrfs_printk+0x1e8/0x430 [btrfs]
[ 55.474214][ T1454] ? preempt_notifier_dec+0x20/0x20
[ 55.479245][ T1454] ? btrfs_cleanup_transaction+0xae0/0xae0 [btrfs]
[ 55.486236][ T1454] ? fsnotify_sb_delete+0x2ab/0x420
[ 55.491265][ T1454] ? fsnotify+0x14d0/0x1550
[ 55.495604][ T1454] ? dispose_list+0x1b0/0x1b0
[ 55.500118][ T1454] generic_shutdown_super+0x13f/0x370
[ 55.505320][ T1454] kill_anon_super+0x3a/0x90
[ 55.509745][ T1454] btrfs_kill_super+0x3b/0x50 [btrfs]
[ 55.515033][ T1454] deactivate_locked_super+0xa2/0x190
[ 55.520235][ T1454] cleanup_mnt+0x1e5/0x3f0
[ 55.524487][ T1454] task_work_run+0x119/0x200
[ 55.528911][ T1454] ? task_work_cancel+0x20/0x20
[ 55.533592][ T1454] ? __x64_sys_umount+0x119/0x140
[ 55.538447][ T1454] ? __ia32_sys_oldumount+0xf0/0xf0
[ 55.543475][ T1454] syscall_exit_to_user_mode+0x1fa/0x200
[ 55.548936][ T1454] do_syscall_64+0x6f/0x170
[ 55.553272][ T1454] entry_SYSCALL_64_after_hwframe+0x63/0x6b
[ 55.558994][ T1454] RIP: 0033:0x7fcb7e405a67
[ 55.563244][ T1454] Code: 24 0d 00 f7 d8 64 89 01 48 83 c8 ff c3 66 0f 1f 44 00 00 31 f6 e9 09 00 00 00 66 0f 1f 84 00 00 00 00 00 b8 a6 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d f9 23 0d 00 f7 d8 64 89 01 48
[ 55.582617][ T1454] RSP: 002b:00007ffd2ff1b1d8 EFLAGS: 00000246 ORIG_RAX: 00000000000000a6
[ 55.590846][ T1454] RAX: 0000000000000000 RBX: 00007fcb7e53a264 RCX: 00007fcb7e405a67
[ 55.598639][ T1454] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000564579779b90
[ 55.606431][ T1454] RBP: 0000564579779960 R08: 0000000000000000 R09: 00007ffd2ff19f80
[ 55.614225][ T1454] R10: 00007fcb7e498fc0 R11: 0000000000000246 R12: 0000000000000000
[ 55.622019][ T1454] R13: 0000564579779b90 R14: 0000564579779a70 R15: 0000000000000000
[ 55.629829][ T1454] </TASK>
[ 55.632697][ T1454] ==================================================================
[ 55.640659][ T1454] Disabling lock debugging due to kernel taint
[ 55.646643][ T1454] BUG: kernel NULL pointer dereference, address: 000000000000001c
[ 55.654266][ T1454] #PF: supervisor write access in kernel mode
[ 55.660162][ T1454] #PF: error_code(0x0002) - not-present page
[ 55.665971][ T1454] PGD 0 P4D 0
[ 55.669189][ T1454] Oops: 0002 [#1] PREEMPT SMP KASAN PTI
[ 55.674565][ T1454] CPU: 1 PID: 1454 Comm: umount Tainted: G S B I 6.8.0-rc4-00127-g1511810d056b #1
[ 55.684619][ T1454] Hardware name: Dell Inc. OptiPlex 7040/0Y7WYT, BIOS 1.1.1 10/07/2015
[ 55.692684][ T1454] RIP: 0010:btrfs_put_block_group+0x15f/0x2c0 [btrfs]
[ 55.699363][ T1454] Code: c1 ea 03 80 3c 02 00 0f 85 31 01 00 00 48 8b ab 28 02 00 00 be 04 00 00 00 4c 8d 65 1c 4c 89 e7 e8 86 cc e6 bf b8 ff ff ff ff <f0> 0f c1 45 1c 83 f8 01 74 7e 85 c0 0f 8e 9b 00 00 00 48 89 df 5b
[ 55.718754][ T1454] RSP: 0018:ffffc90001317b78 EFLAGS: 00010246
[ 55.724647][ T1454] RAX: 00000000ffffffff RBX: ffff8881eae12000 RCX: 0000000000000001
[ 55.732455][ T1454] RDX: fffffbfff0c59f01 RSI: 0000000000000008 RDI: ffffffff862cf800
[ 55.740260][ T1454] RBP: 0000000000000000 R08: 0000000000000001 R09: fffffbfff0c59f00
[ 55.748054][ T1454] R10: ffffffff862cf807 R11: 0000000000000001 R12: 000000000000001c
[ 55.755848][ T1454] R13: ffff88818c5da090 R14: ffff8881eae12100 R15: ffff8881eae120d8
[ 55.763642][ T1454] FS: 00007fcb7e1c8840(0000) GS:ffff8887ee280000(0000) knlGS:0000000000000000
[ 55.772406][ T1454] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 55.778826][ T1454] CR2: 000000000000001c CR3: 00000001e5a68006 CR4: 00000000003706f0
[ 55.786620][ T1454] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 55.794416][ T1454] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 55.802223][ T1454] Call Trace:
[ 55.805352][ T1454] <TASK>
[ 55.808132][ T1454] ? __die+0x23/0x70
[ 55.811871][ T1454] ? page_fault_oops+0x136/0x240
[ 55.816655][ T1454] ? show_fault_oops+0x780/0x780
[ 55.821426][ T1454] ? exc_page_fault+0x5c/0xc0
[ 55.825938][ T1454] ? asm_exc_page_fault+0x26/0x30
[ 55.830797][ T1454] ? btrfs_put_block_group+0x15f/0x2c0 [btrfs]
[ 55.836867][ T1454] ? btrfs_put_block_group+0x15a/0x2c0 [btrfs]
[ 55.842937][ T1454] btrfs_free_block_groups+0x7fd/0x10f0 [btrfs]
[ 55.849077][ T1454] ? free_root_pointers+0x759/0xa10 [btrfs]
[ 55.854884][ T1454] close_ctree+0x87c/0xcf0 [btrfs]
[ 55.859891][ T1454] ? _btrfs_printk+0x1e8/0x430 [btrfs]
[ 55.865252][ T1454] ? preempt_notifier_dec+0x20/0x20
[ 55.870283][ T1454] ? btrfs_cleanup_transaction+0xae0/0xae0 [btrfs]
[ 55.877277][ T1454] ? fsnotify_sb_delete+0x2ab/0x420
[ 55.882308][ T1454] ? fsnotify+0x14d0/0x1550
[ 55.886645][ T1454] ? dispose_list+0x1b0/0x1b0
[ 55.891156][ T1454] generic_shutdown_super+0x13f/0x370
[ 55.896358][ T1454] kill_anon_super+0x3a/0x90
[ 55.900785][ T1454] btrfs_kill_super+0x3b/0x50 [btrfs]
[ 55.906047][ T1454] deactivate_locked_super+0xa2/0x190
[ 55.911249][ T1454] cleanup_mnt+0x1e5/0x3f0
[ 55.915516][ T1454] task_work_run+0x119/0x200
[ 55.919957][ T1454] ? task_work_cancel+0x20/0x20
[ 55.924651][ T1454] ? __x64_sys_umount+0x119/0x140
[ 55.929522][ T1454] ? __ia32_sys_oldumount+0xf0/0xf0
[ 55.934575][ T1454] syscall_exit_to_user_mode+0x1fa/0x200
[ 55.940044][ T1454] do_syscall_64+0x6f/0x170
[ 55.944391][ T1454] entry_SYSCALL_64_after_hwframe+0x63/0x6b
[ 55.950115][ T1454] RIP: 0033:0x7fcb7e405a67
[ 55.954369][ T1454] Code: 24 0d 00 f7 d8 64 89 01 48 83 c8 ff c3 66 0f 1f 44 00 00 31 f6 e9 09 00 00 00 66 0f 1f 84 00 00 00 00 00 b8 a6 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d f9 23 0d 00 f7 d8 64 89 01 48
[ 55.973758][ T1454] RSP: 002b:00007ffd2ff1b1d8 EFLAGS: 00000246 ORIG_RAX: 00000000000000a6
[ 55.981985][ T1454] RAX: 0000000000000000 RBX: 00007fcb7e53a264 RCX: 00007fcb7e405a67
[ 55.989779][ T1454] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000564579779b90
[ 55.997574][ T1454] RBP: 0000564579779960 R08: 0000000000000000 R09: 00007ffd2ff19f80
[ 56.005367][ T1454] R10: 00007fcb7e498fc0 R11: 0000000000000246 R12: 0000000000000000
[ 56.013160][ T1454] R13: 0000564579779b90 R14: 0000564579779a70 R15: 0000000000000000
[ 56.020959][ T1454] </TASK>
[ 56.023826][ T1454] Modules linked in: dm_mod btrfs blake2b_generic xor raid6_pq zstd_compress intel_rapl_msr libcrc32c intel_rapl_common x86_pkg_temp_thermal intel_powerclamp coretemp kvm_intel sd_mod t10_pi crc64_rocksoft_generic kvm crc64_rocksoft crc64 irqbypass crct10dif_pclmul sg crc32_pclmul crc32c_intel ipmi_devintf ipmi_msghandler ghash_clmulni_intel sha512_ssse3 i915 mei_wdt rapl ahci wmi_bmof intel_cstate drm_buddy intel_gtt drm_display_helper libahci intel_uncore ttm libata mei_me mei drm_kms_helper intel_pch_thermal video acpi_pad wmi drm fuse ip_tables
[ 56.073766][ T1454] CR2: 000000000000001c
[ 56.077761][ T1454] ---[ end trace 0000000000000000 ]---
[ 56.083048][ T1454] RIP: 0010:btrfs_put_block_group+0x15f/0x2c0 [btrfs]
[ 56.089714][ T1454] Code: c1 ea 03 80 3c 02 00 0f 85 31 01 00 00 48 8b ab 28 02 00 00 be 04 00 00 00 4c 8d 65 1c 4c 89 e7 e8 86 cc e6 bf b8 ff ff ff ff <f0> 0f c1 45 1c 83 f8 01 74 7e 85 c0 0f 8e 9b 00 00 00 48 89 df 5b
[ 56.109089][ T1454] RSP: 0018:ffffc90001317b78 EFLAGS: 00010246
[ 56.114983][ T1454] RAX: 00000000ffffffff RBX: ffff8881eae12000 RCX: 0000000000000001
[ 56.122778][ T1454] RDX: fffffbfff0c59f01 RSI: 0000000000000008 RDI: ffffffff862cf800
[ 56.130571][ T1454] RBP: 0000000000000000 R08: 0000000000000001 R09: fffffbfff0c59f00
[ 56.138364][ T1454] R10: ffffffff862cf807 R11: 0000000000000001 R12: 000000000000001c
[ 56.146158][ T1454] R13: ffff88818c5da090 R14: ffff8881eae12100 R15: ffff8881eae120d8
[ 56.153955][ T1454] FS: 00007fcb7e1c8840(0000) GS:ffff8887ee280000(0000) knlGS:0000000000000000
[ 56.162715][ T1454] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 56.169149][ T1454] CR2: 000000000000001c CR3: 00000001e5a68006 CR4: 00000000003706f0
[ 56.175890][ T271] result_service: raw_upload, RESULT_MNT: /internal-lkp-server/result, RESULT_ROOT: /internal-lkp-server/result/xfstests/4HDD-btrfs-generic-group-34/lkp-skl-d02/debian-11.1-x86_64-20220510.cgz/x86_64-rhel-8.3-func/gcc-12/1511810d056bc04fc0aed7a2b20d09b170da3e86/3, TMP_RESULT_ROOT: /tmp/lkp/result
[ 56.176952][ T1454] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 56.176953][ T1454] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 56.176955][ T1454] Kernel panic - not syncing: Fatal exception
[ 56.204637][ T1454] Kernel Offset: disabled
The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20240226/202402261652.bcd6d27d-lkp@intel.com
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 752144a31d79..55b91807aba4 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -7979,8 +7979,7 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info, btrfs_err(fs_info, "dev extent physical offset %llu on devid %llu doesn't have corresponding chunk", physical_offset, devid); - ret = -EUCLEAN; - goto out; + return -EUCLEAN; } stripe_len = btrfs_calc_stripe_length(map); diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h index 21d4de0e3f1f..ce1aa7684c74 100644 --- a/fs/btrfs/volumes.h +++ b/fs/btrfs/volumes.h @@ -566,7 +566,7 @@ struct btrfs_chunk_map { static inline void btrfs_free_chunk_map(struct btrfs_chunk_map *map) { - if (map && refcount_dec_and_test(&map->refs)) { + if (refcount_dec_and_test(&map->refs)) { ASSERT(RB_EMPTY_NODE(&map->rb_node)); kfree(map); } diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c index 3317bebfca95..b9346ca82c47 100644 --- a/fs/btrfs/zoned.c +++ b/fs/btrfs/zoned.c @@ -1564,7 +1564,7 @@ int btrfs_load_block_group_zone_info(struct btrfs_block_group *cache, bool new) cache->physical_map = btrfs_clone_chunk_map(map, GFP_NOFS); if (!cache->physical_map) { ret = -ENOMEM; - goto out; + goto out_free_map; } zone_info = kcalloc(map->num_stripes, sizeof(*zone_info), GFP_NOFS); @@ -1677,6 +1677,7 @@ int btrfs_load_block_group_zone_info(struct btrfs_block_group *cache, bool new) } bitmap_free(active); kfree(zone_info); +out_free_map: btrfs_free_chunk_map(map); return ret;
The helper is simple enough for inlining, we can further simplify it by removing the check for map pointer validity. After this patch all callers always pass a valid pointer. The changes to achieve this: - in verify_one_dev_extent() return and don't jump to the out label that could potentially pass a NULL pointer to btrfs_free_chunk_map - in btrfs_load_block_group_zone_info() add a label that specifically clears the map and does not go through label out that could encounter a NULL pointer in cache->physical_map Signed-off-by: David Sterba <dsterba@suse.com> --- fs/btrfs/volumes.c | 3 +-- fs/btrfs/volumes.h | 2 +- fs/btrfs/zoned.c | 3 ++- 3 files changed, 4 insertions(+), 4 deletions(-)