ext4: prevent data-race that occur when read/write ext4_group_desc structure members

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

Fixes ac27a0ec112a ("[PATCH] ext4: initial copy of files from ext3")

Signed-off-by Jeongjun Park <[email protected]>

---
fs/ext4/ialloc.c | 27 ++++++++++++++++++++++++---

1 file changed, 24 insertions(+), 3 deletions(-)

fs/ext4/ialloc.c +20 -2

index 9dfd768ed9f8..5cae247ff21f 100644
@@ -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);

Quoted 42 lines · Jeongjun Park

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

Quoted 1 line

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

Quoted 96 lines

Fixes ac27a0ec112a ("[PATCH] ext4: initial copy of files from ext3")

Signed-off-by Jeongjun Park <[email protected]>

---
fs/ext4/ialloc.c | 27 ++++++++++++++++++++++++---

1 file changed, 24 insertions(+), 3 deletions(-)

fs/ext4/ialloc.c +20 -2

index 9dfd768ed9f8..5cae247ff21f 100644
@@ -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);

fs/ext4/ialloc.c static int find_group_other(struct super_block *sb, struct inode *parent,

Quoted 10 lines

fs/ext4/ialloc.c +4 -2

-             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