On 2024/8/24 0:05, Theodore Ts'o wrote:
On Fri, Aug 23, 2024 at 10:22:19AM +0800, Baokun Li wrote:
I think this patch is wrong and it will hide the real problem.
The maximum length of a filename is 255 and the minimum block size is 1024,
so it is always guaranteed that the number of entries is greater than or
equal to 2 when do_split() is called.
The problem reported by syzbot was actually caused by a missing check in
make_indexed_dir(). The issue has been fixed:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=50ea741def58
So unless ext4_dx_add_entry() and make_indexed_dir(), or some other function
has a bug, 'split == 0' will not occur.
If we want to defend against future changes that introduce bugs, I think
it's better to add a WARN_ON_ONCE to make sure that the problem isn't hidden
and that it doesn't trigger serious bugs like out-of-bounds access.
I agree that given your patch (50ea741def58: "ext4: check dot and
dotdot of dx_root before making dir indexed") split should never be
zero. (Although there are two ways this could happen --- either count
could be 0, or count == max). But this patch isn't wrong per se
because in the case where split == 0, we do want to prevent the
out-of-bounds memory access bug.
Agreed, it is correct to avoid serious problems by judging the split,
I was thinking that it is wrong to report no error or hint when split == 0.
That being said; adding a WARN_ON_ONCE(split == 0) might be a good
idea, although I'd probably also print more debugging information so
we can take a look at the file system and understand what might have
happened. Maybe something like this?
if (WARN_ON_ONCE(split == 0)) {
/* should never happen, but... */
ext4_error_inode_block(dir, (*bh)->b_blocknr, 0,
"bad indexed directory? hash=%08x:%08x "
"count=%d move=%u", hinfo->hash, hinfo->minor_hash,
count, move);
brelse(*bh);
brelse(bh2);
*bh = 0;
return ERR_PTR(-EFSCORRUPTED);
}
I haven't checked to make sure all of the error code paths / error
handling right, but something like this might be useful for debugging
purposes --- if the file system developer could get access to the file
system moment the error is logged. If the data center automation
causes the file system to get fsck'ed or reformatted right away (which
is the only scalable thing to do if there are millions of file systems
in production :-), something like this is probably not going to help
all that much. Still, it certainly wouldn't hurt.
Totally agree! These printouts are very useful for debugging.
The modification above looks good. I tested it and it works fine.
But I think we could reuse the error handling code like this:
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index e6769b97a970..0187910108c4 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -1997,6 +1997,15 @@ static struct ext4_dir_entry_2 *do_split(handle_t
*handle, struct inode *dir,
else
split = count/2;
+ if (WARN_ON_ONCE(split == 0)) {
+ /* should never happen, but... */
+ ext4_error_inode_block(dir, (*bh)->b_blocknr, 0,
+ "bad indexed directory? hash=%08x:%08x
count=%d move=%u",
+ hinfo->hash, hinfo->minor_hash, count,
move);
+ err = -EFSCORRUPTED;
+ goto out;
+ }
+
hash2 = map[split].hash;
continued = hash2 == map[split - 1].hash;
dxtrace(printk(KERN_INFO "Split block %lu at %x, %i/%i\n",
@@ -2040,10 +2049,11 @@ static struct ext4_dir_entry_2
*do_split(handle_t *handle, struct inode *dir,
return de;
journal_error:
+ ext4_std_error(dir->i_sb, err);
+out:
brelse(*bh);
brelse(bh2);
*bh = NULL;
- ext4_std_error(dir->i_sb, err);
return ERR_PTR(err);
}
If someone does think this would be helpful for them, I wouldn't
object to adding a patch something like this.
Cheers,
- Ted
I think it's very helpful.
Thank you very much for your detailed explanation!
Cheers,
Baokun
