| Message ID | tencent_BE7AEE6C7C2D216CB8949CE8E6EE7ECC2C0A@qq.com |
|---|---|
| State | Awaiting Upstream |
| Headers | show |
| Series | ext4: No need to continue when the number of entries is 1 | expand |
On Mon, 01 Jul 2024 22:25:03 +0800, Edward Adam Davis wrote: > When the number of entries mapped is 1, there is no need to split it. > > Applied, thanks! [1/1] ext4: No need to continue when the number of entries is 1 commit: b2b81e122b5616890ba6657adeb8aa5ca1f05fe2 Best regards,
On 2024/8/22 23:00, Theodore Ts'o wrote: > On Mon, 01 Jul 2024 22:25:03 +0800, Edward Adam Davis wrote: >> When the number of entries mapped is 1, there is no need to split it. >> >> > Applied, thanks! > > [1/1] ext4: No need to continue when the number of entries is 1 > commit: b2b81e122b5616890ba6657adeb8aa5ca1f05fe2 > > Best regards, Hi Ted, 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. continued = WARN_ON_ONCE(split == 0) ? 0 : hash2 == map[split - 1].hash;
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
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
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index a630b27a4cc6..0a111274dc4a 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -2043,7 +2043,7 @@ static struct ext4_dir_entry_2 *do_split(handle_t *handle, struct inode *dir, split = count/2; hash2 = map[split].hash; - continued = hash2 == map[split - 1].hash; + continued = split > 0 ? hash2 == map[split - 1].hash : 0; dxtrace(printk(KERN_INFO "Split block %lu at %x, %i/%i\n", (unsigned long)dx_get_block(frame->at), hash2, split, count-split));
When the number of entries mapped is 1, there is no need to split it. Fixes: ac27a0ec112a ("[PATCH] ext4: initial copy of files from ext3") Reported-by: syzbot+ae688d469e36fb5138d0@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=ae688d469e36fb5138d0 Signed-off-by: Edward Adam Davis <eadavis@qq.com> --- fs/ext4/namei.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)