linux-kernel - Re: [PATCH 12/13] ext4: remove unnecessary check for avoiding multiple update_backups in ext4_flex_group

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230818160038.GA3515079@mit.edu>
Date:   Fri, 18 Aug 2023 12:00:38 -0400
From:   "Theodore Ts'o" <tytso@....edu>
To:     Kemeng Shi <shikemeng@...weicloud.com>
Cc:     adilger.kernel@...ger.ca, linux-ext4@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 12/13] ext4: remove unnecessary check for avoiding
 multiple update_backups in ext4_flex_group_add

On Fri, Aug 18, 2023 at 04:42:31PM +0800, Kemeng Shi wrote:
> > s_group_desc[] is initialized in ext4_group_desc_init() in
> > fs/ext4/super.c, and it is used in fs/ext4/balloc.c, and of course, it
> > is defined in fs/ext4.h.  
> I plan to add comment in fs/ext4.h as following:
> struct ext4_sb_info {
> 	...
> 	struct buffer_head * __rcu *s_group_desc; /* Primary gdb blocks of online groups */
> But I'm not sure it's proper now as you menthioned s_group_desc[] is to
> keep the buffer_heads for the block group descriptor blocks in memory
> and it contains primary gdb block is a coincidence that we only modify
> primary block in kernel.

In general, the terminology that ext4 developers have used is "block
group descriptors" and "backup block group descriptors".  The kernel
never *uses* the backup block group users; and with the sparse_super2
feature, the "backup superblocks" and "backup block group descriptors"
are optional.

They are used by e2fsck if we need to recover a trashed superblock and
block group descriptors, which is why code that is resizing the file
system, or updating the label or the UUID need to update the backup
superblocks and/or backup block group descriptors so that we can
better recover disaster.

So I'd just suggest changing the comment above to "array of bh's for
the block group descriptors".

Cheers,

							- Ted

> Besides, I plan to go through the resize code again in fulture and
> add some comments to make it easy for anyone starting read this
> or make it easy to maintain. Please let me if you disklike it.

P.S.

BTW, a useful test program to add is one that checks to make sure that
the "static" parts of the superblock and block group descriptors
(i.e., the parts that don't get changed under normal operation while
the file system is mounted when the kernel *isn't* trying to do a
resize or change the label, UUID, or in the future, the new ioctl's to
update the parts of the superblock that can get modified by tune2fs),
and to make sure that all of the backup superblock and block group
descriptors have gotten updated.

Some of the bugs that you found may have resulted in some of the
backup bg descriptors not getting updataed, which we wouldn't
necessarily notice unless we had a test program that explicitly
checked for them.

And truth to tell, the only backup superblock and block group
descriptor that actually gets used to recover the file system is the
first one, since that's the one e2fsck will fall back to
automatically.  An expert might try to use one of the other backup
block groups as a desperate measure, and there might be some automated
programs that might be smart enough to use the backup block groups
when trying to recover the location of the partition table when the
file system and partition table is very badly damaged --- so that's
one of the reasons why with sparse_super2, the number of backup block
group descriptors can be limited to (for example) one located in the
first block group, and one located in the very last block group.