Message ID | 20210706133602.30706-2-colin.king@canonical.com |
---|---|
State | New |
Headers | show |
Series | [UBUNTU,SAUCE,SRU,B/F/G/H/I] ext4: fix directory index node split corruption | expand |
Hi, On Tue, Jul 6, 2021 at 2:36 PM Colin King <colin.king@canonical.com> wrote: > > From: Colin Ian King <colin.king@canonical.com> > > BugLink: https://bugs.launchpad.net/bugs/1933074 > > [ Note: This was originally sent upstream to linux-ext4@vger.kernel.org > and Ted Tso but didn't seem to make it to the list, presumably as it > was marked as SPAM > > See: http://www.voxelsoft.com/2021/ext4_large_dir_corruption.html ] > > Following commit b5776e7, a trivial sequential write of empty files to > an empty ext4 file system (with large_dir enabled) fails after just > over 26 million files. Depending on luck, file creation will give error > EEXIST or EUCLEAN. > > Commit b5776e7 fixed the no-restart condition so that > ext4_handle_dirty_dx_node is always called, but it also broke the > restart condition. This is because when restart=1, the original > implementation correctly skipped do_split() but b5776e7 clobbered the > "if(restart)goto journal_error;" logic. > Interesting how that was missed in reviews. It is not normal to drop goto without explanation, and that commit didn't explain anything wrt goto change. > This complementary change protects do_split() from restart condition, > making it safe from both current and future ordering of goto statements > in earlier sections of the code. > > Tested on 5.11.20 with handy testing script: > > i = 0 > while i <= 32000000: > print (i) > with open('tmpmnt/%d' % i, 'wb') as fout: > i += 1 > > Google-Bug-Id: 176345532 > Fixes: b5776e7 ("ext4: fix potential htree index checksum corruption") > Fixes: e08ac99 ("ext4: add largedir feature") > Signed-off-by: denis@voxelsoft.com > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > fs/ext4/namei.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > Acked-by: Dimitri John Ledkov <dimitri.ledkov@canonical.com> > diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c > index 4c37abe..9ad5cc6 100644 > --- a/fs/ext4/namei.c > +++ b/fs/ext4/namei.c > @@ -2432,13 +2432,15 @@ static int ext4_dx_add_entry(handle_t *handle, struct ext4_filename *fname, > goto journal_error; Just above this line there is "restart = 1;" which with this patch makes "goto journal_error" kind of redundant, since the code between `restart =1;` and journal_error is disabled by setting restart = 1. I would have dropped this goto journal_error, although in a way it documents what should happen next if any new code gets added below that is run with and without restart. > } > } > - de = do_split(handle, dir, &bh, frame, &fname->hinfo); > - if (IS_ERR(de)) { > - err = PTR_ERR(de); > + if (!restart) { > + de = do_split(handle, dir, &bh, frame, &fname->hinfo); > + if (IS_ERR(de)) { > + err = PTR_ERR(de); > + goto cleanup; > + } > + err = add_dirent_to_buf(handle, fname, dir, inode, de, bh); > goto cleanup; > } > - err = add_dirent_to_buf(handle, fname, dir, inode, de, bh); > - goto cleanup; > > journal_error: > ext4_std_error(dir->i_sb, err); /* this is a no-op if err == 0 */ > -- > 2.7.4
On 06.07.21 15:36, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > BugLink: https://bugs.launchpad.net/bugs/1933074 > > [ Note: This was originally sent upstream to linux-ext4@vger.kernel.org > and Ted Tso but didn't seem to make it to the list, presumably as it > was marked as SPAM > > See: http://www.voxelsoft.com/2021/ext4_large_dir_corruption.html ] > > Following commit b5776e7, a trivial sequential write of empty files to > an empty ext4 file system (with large_dir enabled) fails after just > over 26 million files. Depending on luck, file creation will give error > EEXIST or EUCLEAN. > > Commit b5776e7 fixed the no-restart condition so that > ext4_handle_dirty_dx_node is always called, but it also broke the > restart condition. This is because when restart=1, the original > implementation correctly skipped do_split() but b5776e7 clobbered the > "if(restart)goto journal_error;" logic. > > This complementary change protects do_split() from restart condition, > making it safe from both current and future ordering of goto statements > in earlier sections of the code. > > Tested on 5.11.20 with handy testing script: > > i = 0 > while i <= 32000000: > print (i) > with open('tmpmnt/%d' % i, 'wb') as fout: > i += 1 > > Google-Bug-Id: 176345532 > Fixes: b5776e7 ("ext4: fix potential htree index checksum corruption") > Fixes: e08ac99 ("ext4: add largedir feature") > Signed-off-by: denis@voxelsoft.com > Signed-off-by: Colin Ian King <colin.king@canonical.com> Acked-by: Stefan Bader <stefan.bader@canonical.com> > --- As this is not anywhere upstream, yet, we would need to prefix this with "UBUNTU: SAUCE" when we apply it. The change does what it explains but more than that there is good testing with this. -Stefan > fs/ext4/namei.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c > index 4c37abe..9ad5cc6 100644 > --- a/fs/ext4/namei.c > +++ b/fs/ext4/namei.c > @@ -2432,13 +2432,15 @@ static int ext4_dx_add_entry(handle_t *handle, struct ext4_filename *fname, > goto journal_error; > } > } > - de = do_split(handle, dir, &bh, frame, &fname->hinfo); > - if (IS_ERR(de)) { > - err = PTR_ERR(de); > + if (!restart) { > + de = do_split(handle, dir, &bh, frame, &fname->hinfo); > + if (IS_ERR(de)) { > + err = PTR_ERR(de); > + goto cleanup; > + } > + err = add_dirent_to_buf(handle, fname, dir, inode, de, bh); > goto cleanup; > } > - err = add_dirent_to_buf(handle, fname, dir, inode, de, bh); > - goto cleanup; > > journal_error: > ext4_std_error(dir->i_sb, err); /* this is a no-op if err == 0 */ >
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index 4c37abe..9ad5cc6 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -2432,13 +2432,15 @@ static int ext4_dx_add_entry(handle_t *handle, struct ext4_filename *fname, goto journal_error; } } - de = do_split(handle, dir, &bh, frame, &fname->hinfo); - if (IS_ERR(de)) { - err = PTR_ERR(de); + if (!restart) { + de = do_split(handle, dir, &bh, frame, &fname->hinfo); + if (IS_ERR(de)) { + err = PTR_ERR(de); + goto cleanup; + } + err = add_dirent_to_buf(handle, fname, dir, inode, de, bh); goto cleanup; } - err = add_dirent_to_buf(handle, fname, dir, inode, de, bh); - goto cleanup; journal_error: ext4_std_error(dir->i_sb, err); /* this is a no-op if err == 0 */