linux-ext4 - Re: [PATCH 1/2] ext4: always allocate blocks only from groups inode can use

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <e472kuaklq556gbycq2h4e34jomct7aysjvkdcgwmhl4elwpci@u5abofkkhg4h>
Date: Wed, 14 Jan 2026 18:26:07 +0100
From: Jan Kara <jack@...e.cz>
To: Pedro Falcato <pfalcato@...e.de>
Cc: Jan Kara <jack@...e.cz>, Ted Tso <tytso@....edu>, 
	linux-ext4@...r.kernel.org, Baokun Li <libaokun1@...wei.com>
Subject: Re: [PATCH 1/2] ext4: always allocate blocks only from groups inode
 can use

On Tue 13-01-26 16:28:07, Pedro Falcato wrote:
> On Fri, Jan 09, 2026 at 11:53:37AM +0100, Jan Kara wrote:
> > For filesystems with more than 2^32 blocks inodes using indirect block
> > based format cannot use blocks beyond the 32-bit limit.
> > ext4_mb_scan_groups_linear() takes care to not select these unsupported
> > groups for such inodes however other functions selecting groups for
> > allocation don't. So far this is harmless because the other selection
> > functions are used only with mb_optimize_scan and this is currently
> > disabled for inodes with indirect blocks however in the following patch
> > we want to enable mb_optimize_scan regardless of inode format.
> > 
> > Signed-off-by: Jan Kara <jack@...e.cz>
> > ---
> >  fs/ext4/mballoc.c | 26 +++++++++++++++++---------
> >  1 file changed, 17 insertions(+), 9 deletions(-)
> > 
> > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> > index 56d50fd3310b..f0e07bf11a93 100644
> > --- a/fs/ext4/mballoc.c
> > +++ b/fs/ext4/mballoc.c
> > @@ -892,6 +892,18 @@ mb_update_avg_fragment_size(struct super_block *sb, struct ext4_group_info *grp)
> >  	}
> >  }
> >  
> > +static ext4_group_t ext4_get_allocation_groups_count(
> > +				struct ext4_allocation_context *ac)
> > +{
> > +	ext4_group_t ngroups = ext4_get_groups_count(ac->ac_sb);
> > +
> > +	/* non-extent files are limited to low blocks/groups */
> > +	if (!(ext4_test_inode_flag(ac->ac_inode, EXT4_INODE_EXTENTS)))
> > +		ngroups = EXT4_SB(ac->ac_sb)->s_blockfile_groups;
> > +
> > +	return ngroups;
> > +}
> 
> I know you're mostly only moving code around, but I think I see a problem here.
> Namely, we (probably?) need an smp_rmb() right after the s_blockfile_groups
> read to pair with the one in ext4_update_super(). The pre-existing smp_rmb()
> in ext4_get_groups_acount() after the s_groups_count load perhaps *incidentally*
> works here, but it seems to me like we need a new barrier. So fundamentally
> something like:
> 
> static ext4_group_t ext4_get_allocation_groups_count(...)
> {
> 	struct ext4_sb_info *sb = EXT4_SB(ac->ac_sb);
> 	ext4_group_t ngroups;
> 
> 	ngroups = sb->s_groups_count;
> 	if (!ext4_test_inode_flag(ac->ac_inode, EXT4_INODE_EXTENTS))
> 		ngroups = sb->s_blockfile_groups;
> 	/* pairs with ext4_group_add() logic */
> 	smp_rmb();
> 	return ngroups;
> }
> 
> and to be even more technically correct, we probably want READ_ONCE()
> and WRITE_ONCE() here as well.
> 
> Does this make sense?

I agree with both although I'd note this isn't strictly related to this
patch as the problem is already preexisting in the code. I think smp_rmb()
is good to add when we are touching the code, regarding READ_ONCE /
WRITE_ONCE, that will require modifying all the places touching
s_blockfile_groups / s_groups_count so I'd leave that for a separate series
as that's going to be more intrusive.

								Honza
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR