| Message ID | 20201229090208.1113218-1-yangerkun@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] ext4: fix bug for rename with RENAME_WHITEOUT | expand |
On Tue, Dec 29, 2020 at 05:02:08PM +0800, yangerkun wrote: > ext4_rename will create a special inode for whiteout and use this 'ino' > to replace the source file's dir entry 'ino'. Once error happens > latter(small ext4 img, and consume all space, so the rename with dst > path not exist will fail due to the ENOSPC return from ext4_add_entry in > ext4_rename), the cleanup do drop the nlink for whiteout, but forget to > restore 'ino' with source file. This will lead to "deleted inode > referenced". Could you sendhave instructions how to reproduce this failure? Many thanks!! - Ted
在 2020/12/30 6:24, Theodore Ts'o 写道: > On Tue, Dec 29, 2020 at 05:02:08PM +0800, yangerkun wrote: >> ext4_rename will create a special inode for whiteout and use this 'ino' >> to replace the source file's dir entry 'ino'. Once error happens >> latter(small ext4 img, and consume all space, so the rename with dst >> path not exist will fail due to the ENOSPC return from ext4_add_entry in >> ext4_rename), the cleanup do drop the nlink for whiteout, but forget to >> restore 'ino' with source file. This will lead to "deleted inode >> referenced". > > Could you sendhave instructions how to reproduce this failure? Many thanks!! Hi, Follow step will reproduce it easily! cd /dev/shm mkdir test/ fallocate -l 128M img mkfs.ext4 -b 1024 img mount img test/ dd if=/dev/zero of=test/foo bs=1M count=128 mkdir test/dir/ && cd test/dir/ for ((i=0;i<1000;i++)); do touch file$i; done # consume all block cd ~ && renameat2(AT_FDCWD, /dev/shm/test/dir/file1, AT_FDCWD, /dev/shm/test/dir/dst_file, RENAME_WHITEOUT) # ext4_add_entry in ext4_rename will return ENOSPC!! cd /dev/shm/ && mount img test/ && ls -li test/dir/file1 We will get the output: "ls: cannot access 'test/dir/file1': Structure needs cleaning" and the dmesg show: "EXT4-fs error (device loop0): ext4_lookup:1626: inode #2049: comm ls: deleted inode referenced: 139" static int ext4_rename(...) { ... whiteout = ext4_whiteout_for_rename(&old, credits, &handle); ... retval = ext4_setent(handle, &old, whiteout->i_ino, EXT4_FT_CHRDEV); // will replace dir entry with ... if (!new.bh) { retval = ext4_add_entry(handle, new.dentry, old.inode); // will fail with ENOSPC if (retval) goto end_rename; ... end_rename: ... if (whiteout) { // forget to restore the dir entry's ino if (retval) drop_nlink(whiteout); unlock_new_inode(whiteout); iput(whiteout); } ... } Thanks, Kun. > > - Ted > . >
On Tue 29-12-20 17:02:08, yangerkun wrote: > ext4_rename will create a special inode for whiteout and use this 'ino' > to replace the source file's dir entry 'ino'. Once error happens > latter(small ext4 img, and consume all space, so the rename with dst > path not exist will fail due to the ENOSPC return from ext4_add_entry in > ext4_rename), the cleanup do drop the nlink for whiteout, but forget to > restore 'ino' with source file. This will lead to "deleted inode > referenced". > > Signed-off-by: yangerkun <yangerkun@huawei.com> Thanks for the patch! It looks mostly good, just one comment below: > end_rename: > - brelse(old.dir_bh); > - brelse(old.bh); > - brelse(new.bh); > if (whiteout) { > + ext4_setent(handle, &old, > + old.inode->i_ino, old_file_type); I'm wondering here - how is it correct to reset the 'old' entry whenever whiteout != NULL? I'd expect this to be guarded by the if (retval) check... Honza > if (retval) > drop_nlink(whiteout); > unlock_new_inode(whiteout); > iput(whiteout); > } > + brelse(old.dir_bh); > + brelse(old.bh); > + brelse(new.bh); > if (handle) > ext4_journal_stop(handle); > return retval; > -- > 2.25.4 >
在 2021/1/4 22:19, Jan Kara 写道: > On Tue 29-12-20 17:02:08, yangerkun wrote: >> ext4_rename will create a special inode for whiteout and use this 'ino' >> to replace the source file's dir entry 'ino'. Once error happens >> latter(small ext4 img, and consume all space, so the rename with dst >> path not exist will fail due to the ENOSPC return from ext4_add_entry in >> ext4_rename), the cleanup do drop the nlink for whiteout, but forget to >> restore 'ino' with source file. This will lead to "deleted inode >> referenced". >> >> Signed-off-by: yangerkun <yangerkun@huawei.com> > > Thanks for the patch! It looks mostly good, just one comment below: > >> end_rename: >> - brelse(old.dir_bh); >> - brelse(old.bh); >> - brelse(new.bh); >> if (whiteout) { >> + ext4_setent(handle, &old, >> + old.inode->i_ino, old_file_type); > > I'm wondering here - how is it correct to reset the 'old' entry whenever > whiteout != NULL? I'd expect this to be guarded by the if (retval) check... Thanks a lot! This is actually a bug and sorry for that. We need check retval to prevent call for ext4_setent for the correct case. I will resend the patch! > > Honza > >> if (retval) >> drop_nlink(whiteout); >> unlock_new_inode(whiteout); >> iput(whiteout); >> } >> + brelse(old.dir_bh); >> + brelse(old.bh); >> + brelse(new.bh); >> if (handle) >> ext4_journal_stop(handle); >> return retval; >> -- >> 2.25.4 >>
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index b17a082b7db1..b3acfd384583 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -3593,9 +3593,6 @@ static int ext4_setent(handle_t *handle, struct ext4_renament *ent, return retval2; } } - brelse(ent->bh); - ent->bh = NULL; - return retval; } @@ -3794,6 +3791,7 @@ static int ext4_rename(struct inode *old_dir, struct dentry *old_dentry, } } + old_file_type = old.de->file_type; if (IS_DIRSYNC(old.dir) || IS_DIRSYNC(new.dir)) ext4_handle_sync(handle); @@ -3821,7 +3819,6 @@ static int ext4_rename(struct inode *old_dir, struct dentry *old_dentry, force_reread = (new.dir->i_ino == old.dir->i_ino && ext4_test_inode_flag(new.dir, EXT4_INODE_INLINE_DATA)); - old_file_type = old.de->file_type; if (whiteout) { /* * Do this before adding a new entry, so the old entry is sure @@ -3919,15 +3916,17 @@ static int ext4_rename(struct inode *old_dir, struct dentry *old_dentry, retval = 0; end_rename: - brelse(old.dir_bh); - brelse(old.bh); - brelse(new.bh); if (whiteout) { + ext4_setent(handle, &old, + old.inode->i_ino, old_file_type); if (retval) drop_nlink(whiteout); unlock_new_inode(whiteout); iput(whiteout); } + brelse(old.dir_bh); + brelse(old.bh); + brelse(new.bh); if (handle) ext4_journal_stop(handle); return retval;
ext4_rename will create a special inode for whiteout and use this 'ino' to replace the source file's dir entry 'ino'. Once error happens latter(small ext4 img, and consume all space, so the rename with dst path not exist will fail due to the ENOSPC return from ext4_add_entry in ext4_rename), the cleanup do drop the nlink for whiteout, but forget to restore 'ino' with source file. This will lead to "deleted inode referenced". Signed-off-by: yangerkun <yangerkun@huawei.com> --- fs/ext4/namei.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)