On Sep 20, 2024, at 9:00 AM, Jeongjun Park <aha310510@xxxxxxxxx> wrote:
>
> Currently, data-race like [1] occur in fs/ext4/ialloc.c
>
> find_group_other() and find_group_orlov() read multiple ext4_groups but
> do not protect them with locks, which causes data-race. I think it would
> be appropriate to add ext4_lock_group() at an appropriate location to solve
> this.
>
> [1]
>
> ==================================================================
> BUG: KCSAN: data-race in ext4_free_inodes_count / ext4_free_inodes_set
>
> write to 0xffff88810404300e of 2 bytes by task 6254 on cpu 1:
> ext4_free_inodes_set+0x1f/0x80 fs/ext4/super.c:405
> __ext4_new_inode+0x15ca/0x2200 fs/ext4/ialloc.c:1216
> ext4_symlink+0x242/0x5a0 fs/ext4/namei.c:3391
> vfs_symlink+0xca/0x1d0 fs/namei.c:4615
> do_symlinkat+0xe3/0x340 fs/namei.c:4641
> __do_sys_symlinkat fs/namei.c:4657 [inline]
> __se_sys_symlinkat fs/namei.c:4654 [inline]
> __x64_sys_symlinkat+0x5e/0x70 fs/namei.c:4654
> x64_sys_call+0x1dda/0x2d60 arch/x86/include/generated/asm/syscalls_64.h:267
> do_syscall_x64 arch/x86/entry/common.c:52 [inline]
> do_syscall_64+0x54/0x120 arch/x86/entry/common.c:83
> entry_SYSCALL_64_after_hwframe+0x76/0x7e
>
> read to 0xffff88810404300e of 2 bytes by task 6257 on cpu 0:
> ext4_free_inodes_count+0x1c/0x80 fs/ext4/super.c:349
> find_group_other fs/ext4/ialloc.c:594 [inline]
> __ext4_new_inode+0x6ec/0x2200 fs/ext4/ialloc.c:1017
> ext4_symlink+0x242/0x5a0 fs/ext4/namei.c:3391
> vfs_symlink+0xca/0x1d0 fs/namei.c:4615
> do_symlinkat+0xe3/0x340 fs/namei.c:4641
> __do_sys_symlinkat fs/namei.c:4657 [inline]
> __se_sys_symlinkat fs/namei.c:4654 [inline]
> __x64_sys_symlinkat+0x5e/0x70 fs/namei.c:4654
> x64_sys_call+0x1dda/0x2d60 arch/x86/include/generated/asm/syscalls_64.h:267
> do_syscall_x64 arch/x86/entry/common.c:52 [inline]
> do_syscall_64+0x54/0x120 arch/x86/entry/common.c:83
> entry_SYSCALL_64_after_hwframe+0x76/0x7e
>
> value changed: 0x185c -> 0x185b
I see now after you sent this patch that all of these cases are for
read-only access to the free inodes count, which doesn't really
matter if the value is racy. These values are only used in heuristics
for block group selection, and if the value is wrong then creating a
new subdirectory may be in a different group, but that doesn't make
much difference.
It looks like the write side of all these accesses are already under
ext4_group_lock(), so the code is actually correct and not in danger
of two threads updating bg_free_inodes_count_lo/hi inconsistently.
We probably *do not* want locking in the read case, as it will cause
unnecessary lock contention scanning groups for subdirectory allocation.
My suggestion at this point would be to go back to using READ_ONCE() and
WRITE_ONCE() in ext4_free_inodes_count()/ext4_free_inodes_set() like in
your original patch. but *only* for functions used by find_group_*(). We
want to be warned by KASAN if any of the other fields are accessed without
a proper ext4_group_lock(), since READ_ONCE()/WRITE_ONCE() does not fix
_lo/_hi tearing.
It probably also makes sense to add comments to all of these functions
that they should hold ext4_group_lock() for access/updates, *except*
ext4_free_inodes_count() can be called to read the inode count without it
if the result does not need to be totally accurate.
Cheers, Andreas
> Fixes: ac27a0ec112a ("[PATCH] ext4: initial copy of files from ext3")
> Signed-off-by: Jeongjun Park <aha310510@xxxxxxxxx>
> ---
> fs/ext4/ialloc.c | 27 ++++++++++++++++++++++++---
> 1 file changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
> index 9dfd768ed9f8..5cae247ff21f 100644
> --- a/fs/ext4/ialloc.c
> +++ b/fs/ext4/ialloc.c
> @@ -500,11 +500,14 @@ static int find_group_orlov(struct super_block *sb, struct inode *parent,
> for (i = 0; i < flex_size; i++) {
> if (grp+i >= real_ngroups)
> break;
> + ext4_lock_group(sb, grp+i);
> desc = ext4_get_group_desc(sb, grp+i, NULL);
> if (desc && ext4_free_inodes_count(sb, desc)) {
> *group = grp+i;
> + ext4_unlock_group(sb, grp+i);
> return 0;
> }
> + ext4_unlock_group(sb, grp+i);
> }
> goto fallback;
> }
> @@ -544,14 +547,17 @@ static int find_group_orlov(struct super_block *sb, struct inode *parent,
> parent_group = EXT4_I(parent)->i_block_group;
> for (i = 0; i < ngroups; i++) {
> grp = (parent_group + i) % ngroups;
> + ext4_lock_group(sb, grp);
> desc = ext4_get_group_desc(sb, grp, NULL);
> if (desc) {
> grp_free = ext4_free_inodes_count(sb, desc);
> if (grp_free && grp_free >= avefreei) {
> *group = grp;
> + ext4_unlock_group(sb, grp);
> return 0;
> }
> }
> + ext4_unlock_group(sb, grp);
> }
>
> if (avefreei) {
> @@ -590,11 +596,14 @@ static int find_group_other(struct super_block *sb, struct inode *parent,
> if (last > ngroups)
> last = ngroups;
> for (i = parent_group; i < last; i++) {
> + ext4_lock_group(sb, i);
> desc = ext4_get_group_desc(sb, i, NULL);
> if (desc && ext4_free_inodes_count(sb, desc)) {
> *group = i;
> + ext4_unlock_group(sb, i);
> return 0;
> }
> + ext4_unlock_group(sb, i);
> }
> if (!retry && EXT4_I(parent)->i_last_alloc_group != ~0) {
> retry = 1;
> @@ -616,10 +625,14 @@ static int find_group_other(struct super_block *sb, struct inode *parent,
> * Try to place the inode in its parent directory
> */
> *group = parent_group;
> + ext4_lock_group(sb, *group);
> desc = ext4_get_group_desc(sb, *group, NULL);
> if (desc && ext4_free_inodes_count(sb, desc) &&
> - ext4_free_group_clusters(sb, desc))
> + ext4_free_group_clusters(sb, desc)) {
> + ext4_unlock_group(sb, *group);
> return 0;
> + }
> + ext4_unlock_group(sb, *group);
>
> /*
> * We're going to place this inode in a different blockgroup from its
> @@ -640,10 +653,14 @@ static int find_group_other(struct super_block *sb, struct inode *parent,
> *group += i;
> if (*group >= ngroups)
> *group -= ngroups;
> + ext4_lock_group(sb, *group);
> desc = ext4_get_group_desc(sb, *group, NULL);
> if (desc && ext4_free_inodes_count(sb, desc) &&
> - ext4_free_group_clusters(sb, desc))
> + ext4_free_group_clusters(sb, desc)) {
> + ext4_unlock_group(sb, *group);
> return 0;
> + }
> + ext4_unlock_group(sb, *group);
> }
>
> /*
> @@ -654,9 +671,13 @@ static int find_group_other(struct super_block *sb, struct inode *parent,
> for (i = 0; i < ngroups; i++) {
> if (++*group >= ngroups)
> *group = 0;
> + ext4_lock_group(sb, *group);
> desc = ext4_get_group_desc(sb, *group, NULL);
> - if (desc && ext4_free_inodes_count(sb, desc))
> + if (desc && ext4_free_inodes_count(sb, desc)) {
> + ext4_unlock_group(sb, *group);
> return 0;
> + }
> + ext4_unlock_group(sb, *group);
> }
>
> return -1;
> --
Cheers, Andreas
Attachment:
signature.asc
Description: Message signed with OpenPGP
