[PATCH v1 1/2] ext4: find old entry again if failed to rename whiteout

[PATCH v1 1/2] ext4: find old entry again if failed to rename whiteout

[zhangyi (F)]

If we failed to add new entry on rename whiteout, we cannot reset the
old->de entry directly, because the old->de could have moved from under
us during make indexed dir. So find the old entry again before reset is
needed, otherwise it may corrupt the filesystem as below.

  /dev/sda: Entry '00000001' in ??? (12) has deleted/unused inode 15. CLEARED.
  /dev/sda: Unattached inode 75
  /dev/sda: UNEXPECTED INCONSISTENCY; RUN fsck MANUALLY.

Fixes: 6b4b8e6b4ad ("ext4: fix bug for rename with RENAME_WHITEOUT")
Cc: stable@vger.kernel.org
Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
---
 fs/ext4/namei.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index cf652ba3e74d..17d9400563fa 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -3602,6 +3602,31 @@ static int ext4_setent(handle_t *handle, struct ext4_renament *ent,
 	return retval;
 }
 
+static void ext4_resetent(handle_t *handle, struct ext4_renament *ent,
+			  unsigned ino, unsigned file_type)
+{
+	struct ext4_renament old = *ent;
+	int retval = 0;
+
+	/*
+	 * old->de could have moved from under us during make indexed dir,
+	 * so the old->de may no longer valid and need to find it again
+	 * before reset old inode info.
+	 */
+	old.bh = ext4_find_entry(old.dir, &old.dentry->d_name, &old.de, NULL);
+	if (IS_ERR(old.bh))
+		retval = PTR_ERR(old.bh);
+	if (!old.bh)
+		retval = -ENOENT;
+	if (retval) {
+		ext4_std_error(old.dir->i_sb, retval);
+		return;
+	}
+
+	ext4_setent(handle, &old, ino, file_type);
+	brelse(old.bh);
+}
+
 static int ext4_find_delete_entry(handle_t *handle, struct inode *dir,
 				  const struct qstr *d_name)
 {
@@ -3924,8 +3949,8 @@ static int ext4_rename(struct inode *old_dir, struct dentry *old_dentry,
 end_rename:
 	if (whiteout) {
 		if (retval) {
-			ext4_setent(handle, &old,
-				old.inode->i_ino, old_file_type);
+			ext4_resetent(handle, &old,
+				      old.inode->i_ino, old_file_type);
 			drop_nlink(whiteout);
 		}
 		unlock_new_inode(whiteout);
-- 
2.25.4

[PATCH v1 2/2] ext4: Do not iput inode under running transaction in ext4_rename()

[zhangyi (F)]

In ext4_rename(), when RENAME_WHITEOUT failed to add new entry into
directory, it ends up dropping new created whiteout inode under the
running transaction. After commit <9b88f9fb0d2> ("ext4: Do not iput inode
under running transaction"), we follow the assumptions that evict() does
not get called from a transaction context but in ext4_rename() it breaks
this suggestion. Although it's not a real problem, better to obey it, so
this patch add inode to orphan list and stop transaction before final
iput().

Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>
---
 fs/ext4/namei.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 17d9400563fa..defdbf80ebfa 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -3786,14 +3786,14 @@ static int ext4_rename(struct inode *old_dir, struct dentry *old_dentry,
 	 */
 	retval = -ENOENT;
 	if (!old.bh || le32_to_cpu(old.de->inode) != old.inode->i_ino)
-		goto end_rename;
+		goto release_bh;
 
 	new.bh = ext4_find_entry(new.dir, &new.dentry->d_name,
 				 &new.de, &new.inlined);
 	if (IS_ERR(new.bh)) {
 		retval = PTR_ERR(new.bh);
 		new.bh = NULL;
-		goto end_rename;
+		goto release_bh;
 	}
 	if (new.bh) {
 		if (!new.inode) {
@@ -3810,15 +3810,13 @@ static int ext4_rename(struct inode *old_dir, struct dentry *old_dentry,
 		handle = ext4_journal_start(old.dir, EXT4_HT_DIR, credits);
 		if (IS_ERR(handle)) {
 			retval = PTR_ERR(handle);
-			handle = NULL;
-			goto end_rename;
+			goto release_bh;
 		}
 	} else {
 		whiteout = ext4_whiteout_for_rename(&old, credits, &handle);
 		if (IS_ERR(whiteout)) {
 			retval = PTR_ERR(whiteout);
-			whiteout = NULL;
-			goto end_rename;
+			goto release_bh;
 		}
 	}
 
@@ -3952,16 +3950,18 @@ static int ext4_rename(struct inode *old_dir, struct dentry *old_dentry,
 			ext4_resetent(handle, &old,
 				      old.inode->i_ino, old_file_type);
 			drop_nlink(whiteout);
+			ext4_orphan_add(handle, whiteout);
 		}
 		unlock_new_inode(whiteout);
+		ext4_journal_stop(handle);
 		iput(whiteout);
-
+	} else {
+		ext4_journal_stop(handle);
 	}
+release_bh:
 	brelse(old.dir_bh);
 	brelse(old.bh);
 	brelse(new.bh);
-	if (handle)
-		ext4_journal_stop(handle);
 	return retval;
 }
 
-- 
2.25.4

Re: [PATCH v1 2/2] ext4: Do not iput inode under running transaction in ext4_rename()

[Theodore Ts'o]

On Wed, Mar 03, 2021 at 09:17:03PM +0800, zhangyi (F) wrote:
> In ext4_rename(), when RENAME_WHITEOUT failed to add new entry into
> directory, it ends up dropping new created whiteout inode under the
> running transaction. After commit <9b88f9fb0d2> ("ext4: Do not iput inode
> under running transaction"), we follow the assumptions that evict() does
> not get called from a transaction context but in ext4_rename() it breaks
> this suggestion. Although it's not a real problem, better to obey it, so
> this patch add inode to orphan list and stop transaction before final
> iput().
> 
> Signed-off-by: zhangyi (F) <yi.zhang@huawei.com>

Thanks, applied.

					- Ted

Re: [PATCH v1 1/2] ext4: find old entry again if failed to rename whiteout

[Theodore Ts'o]

On Wed, Mar 03, 2021 at 09:17:02PM +0800, zhangyi (F) wrote:
> If we failed to add new entry on rename whiteout, we cannot reset the
> old->de entry directly, because the old->de could have moved from under
> us during make indexed dir. So find the old entry again before reset is
> needed, otherwise it may corrupt the filesystem as below.
> 
>   /dev/sda: Entry '00000001' in ??? (12) has deleted/unused inode 15. CLEARED.
>   /dev/sda: Unattached inode 75
>   /dev/sda: UNEXPECTED INCONSISTENCY; RUN fsck MANUALLY.
>
>   ....
>
> +	/*
> +	 * old->de could have moved from under us during make indexed dir,
> +	 * so the old->de may no longer valid and need to find it again
> +	 * before reset old inode info.
> +	 */
> +	old.bh = ext4_find_entry(old.dir, &old.dentry->d_name, &old.de, NULL);
> +	if (IS_ERR(old.bh))
> +		retval = PTR_ERR(old.bh);
> +	if (!old.bh)
> +		retval = -ENOENT;
> +	if (retval) {
> +		ext4_std_error(old.dir->i_sb, retval);


So if the directory entry may have been deleted out from under us, an
ENOENT failure might happen under normal circumstances, shouldn't it?

In that case, ext4_std_error() will declare that the file system is
inconsistent, potentially resulting in the file system to be remounted
read-only, or causing the system to panic.  So calling
ext4_std_error() needs to be done carefully.

Are we sure that calling ext4_std_error() is the right thing to do in
the case where ext4_find_entry() returns ENOENT?

							- Ted

Re: [PATCH v1 1/2] ext4: find old entry again if failed to rename whiteout

[zhangyi (F)]

On 2021/3/11 23:43, Theodore Ts'o wrote:
> On Wed, Mar 03, 2021 at 09:17:02PM +0800, zhangyi (F) wrote:
>> If we failed to add new entry on rename whiteout, we cannot reset the
>> old->de entry directly, because the old->de could have moved from under
>> us during make indexed dir. So find the old entry again before reset is
>> needed, otherwise it may corrupt the filesystem as below.
>>
>>   /dev/sda: Entry '00000001' in ??? (12) has deleted/unused inode 15. CLEARED.
>>   /dev/sda: Unattached inode 75
>>   /dev/sda: UNEXPECTED INCONSISTENCY; RUN fsck MANUALLY.
>>
>>   ....
>>
>> +	/*
>> +	 * old->de could have moved from under us during make indexed dir,
>> +	 * so the old->de may no longer valid and need to find it again
>> +	 * before reset old inode info.
>> +	 */
>> +	old.bh = ext4_find_entry(old.dir, &old.dentry->d_name, &old.de, NULL);
>> +	if (IS_ERR(old.bh))
>> +		retval = PTR_ERR(old.bh);
>> +	if (!old.bh)
>> +		retval = -ENOENT;
>> +	if (retval) {
>> +		ext4_std_error(old.dir->i_sb, retval);
> 
> 
> So if the directory entry may have been deleted out from under us, an
> ENOENT failure might happen under normal circumstances, shouldn't it?
> > In that case, ext4_std_error() will declare that the file system is
> inconsistent, potentially resulting in the file system to be remounted
> read-only, or causing the system to panic.  So calling
> ext4_std_error() needs to be done carefully.
> 
> Are we sure that calling ext4_std_error() is the right thing to do in
> the case where ext4_find_entry() returns ENOENT?
> 

Hi, Ted

In this error path of whiteout rename, we want to restore the old inode
number and old name back to the old entry, it's just a rollback operation.
The old entry will stay where it was in common cases, but it can be moved
from the first block to the leaf block during make indexed dir for one
special case, but it cannot be deleted in theory. So if we cannot find it
again, there must some bad thing happen and the filesystem may probably
inconsistency. So I calling ext4_std_error() here，or am I missing something？

Thanks,
Yi.

Re: [PATCH v1 1/2] ext4: find old entry again if failed to rename whiteout

[Theodore Ts'o]

On Fri, Mar 12, 2021 at 10:01:50AM +0800, zhangyi (F) wrote:
> 
> In this error path of whiteout rename, we want to restore the old inode
> number and old name back to the old entry, it's just a rollback operation.
> The old entry will stay where it was in common cases, but it can be moved
> from the first block to the leaf block during make indexed dir for one
> special case, but it cannot be deleted in theory. So if we cannot find it
> again, there must some bad thing happen and the filesystem may probably
> inconsistency. So I calling ext4_std_error() here，or am I missing something？

After looking at this more closely, I agree, this should be OK.  The
directory is going to be locked, so it shouldn't be changing out from
under us.

Thanks, applied.

					- Ted