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
