diff mbox series

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

Message ID 20240920150013.2447-1-aha310510@gmail.com
State Superseded
Headers show
Series [v2] ext4: prevent data-race that occur when read/write ext4_group_desc structure members | expand

Commit Message

Jeongjun Park Sept. 20, 2024, 3 p.m. UTC 
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 <aha310510@gmail.com>
---
 fs/ext4/ialloc.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

--

Comments

Andreas Dilger Sept. 30, 2024, 8:54 p.m. UTC | #1 
On Sep 20, 2024, at 9:00 AM, Jeongjun Park <aha310510@gmail.com> 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@gmail.com>
> ---
> 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
Jeongjun Park Oct. 1, 2024, 12:26 p.m. UTC | #2 
Andreas Dilger <adilger@dilger.ca> wrote:
>
> On Sep 20, 2024, at 9:00 AM, Jeongjun Park <aha310510@gmail.com> 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.

Thanks for the comment! Then it seems appropriate to annotate
ext4_free_inodes_count() called in find_group_* so that KCSAN does
not report it. I will write a v3 patch that implements this and send it to
you right away.

Regards,
Jeongjun Park

>
> Cheers, Andreas
>
> > Fixes: ac27a0ec112a ("[PATCH] ext4: initial copy of files from ext3")
> > Signed-off-by: Jeongjun Park <aha310510@gmail.com>
> > ---
> > 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
>
>
>
>
>
diff mbox series

Patch

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;