Re: [PATCH] ext4: No need to continue when the number of entries is 1

["Theodore Ts'o" <tytso@mit.edu>]

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.

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.

If someone does think this would be helpful for them, I wouldn't
object to adding a patch something like this.

Cheers,

						- Ted