[PATCH v2] ext4: fix bug for rename with RENAME_WHITEOUT

[PATCH v2] ext4: fix bug for rename with RENAME_WHITEOUT

[yangerkun]

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(-)

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;
-- 
2.25.4

Re: [PATCH v2] ext4: fix bug for rename with RENAME_WHITEOUT

[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!!

      	  	   		    - Ted

Re: [PATCH v2] ext4: fix bug for rename with RENAME_WHITEOUT

[yangerkun]

在 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
> .
> 

Re: [PATCH v2] ext4: fix bug for rename with RENAME_WHITEOUT

[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...

									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
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2] ext4: fix bug for rename with RENAME_WHITEOUT

[yangerkun]

在 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
>>