linux-ext4.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 1/2] ext4: find old entry again if failed to rename whiteout
@ 2021-03-03 13:17 zhangyi (F)
  2021-03-03 13:17 ` [PATCH v1 2/2] ext4: Do not iput inode under running transaction in ext4_rename() zhangyi (F)
  2021-03-11 15:43 ` [PATCH v1 1/2] ext4: find old entry again if failed to rename whiteout Theodore Ts'o
  0 siblings, 2 replies; 6+ messages in thread
From: zhangyi (F) @ 2021-03-03 13:17 UTC (permalink / raw)
  To: linux-ext4; +Cc: tytso, jack, yi.zhang, yangerkun

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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v1 2/2] ext4: Do not iput inode under running transaction in ext4_rename()
  2021-03-03 13:17 [PATCH v1 1/2] ext4: find old entry again if failed to rename whiteout zhangyi (F)
@ 2021-03-03 13:17 ` zhangyi (F)
  2021-03-21  4:09   ` Theodore Ts'o
  2021-03-11 15:43 ` [PATCH v1 1/2] ext4: find old entry again if failed to rename whiteout Theodore Ts'o
  1 sibling, 1 reply; 6+ messages in thread
From: zhangyi (F) @ 2021-03-03 13:17 UTC (permalink / raw)
  To: linux-ext4; +Cc: tytso, jack, yi.zhang, yangerkun

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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v1 1/2] ext4: find old entry again if failed to rename whiteout
  2021-03-03 13:17 [PATCH v1 1/2] ext4: find old entry again if failed to rename whiteout zhangyi (F)
  2021-03-03 13:17 ` [PATCH v1 2/2] ext4: Do not iput inode under running transaction in ext4_rename() zhangyi (F)
@ 2021-03-11 15:43 ` Theodore Ts'o
  2021-03-12  2:01   ` zhangyi (F)
  1 sibling, 1 reply; 6+ messages in thread
From: Theodore Ts'o @ 2021-03-11 15:43 UTC (permalink / raw)
  To: zhangyi (F); +Cc: linux-ext4, jack, yangerkun

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v1 1/2] ext4: find old entry again if failed to rename whiteout
  2021-03-11 15:43 ` [PATCH v1 1/2] ext4: find old entry again if failed to rename whiteout Theodore Ts'o
@ 2021-03-12  2:01   ` zhangyi (F)
  2021-03-21  3:59     ` Theodore Ts'o
  0 siblings, 1 reply; 6+ messages in thread
From: zhangyi (F) @ 2021-03-12  2:01 UTC (permalink / raw)
  To: Theodore Ts'o; +Cc: linux-ext4, jack, yangerkun

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v1 1/2] ext4: find old entry again if failed to rename whiteout
  2021-03-12  2:01   ` zhangyi (F)
@ 2021-03-21  3:59     ` Theodore Ts'o
  0 siblings, 0 replies; 6+ messages in thread
From: Theodore Ts'o @ 2021-03-21  3:59 UTC (permalink / raw)
  To: zhangyi (F); +Cc: linux-ext4, jack, yangerkun

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v1 2/2] ext4: Do not iput inode under running transaction in ext4_rename()
  2021-03-03 13:17 ` [PATCH v1 2/2] ext4: Do not iput inode under running transaction in ext4_rename() zhangyi (F)
@ 2021-03-21  4:09   ` Theodore Ts'o
  0 siblings, 0 replies; 6+ messages in thread
From: Theodore Ts'o @ 2021-03-21  4:09 UTC (permalink / raw)
  To: zhangyi (F); +Cc: linux-ext4, jack, yangerkun

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-03-21  4:11 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-03 13:17 [PATCH v1 1/2] ext4: find old entry again if failed to rename whiteout zhangyi (F)
2021-03-03 13:17 ` [PATCH v1 2/2] ext4: Do not iput inode under running transaction in ext4_rename() zhangyi (F)
2021-03-21  4:09   ` Theodore Ts'o
2021-03-11 15:43 ` [PATCH v1 1/2] ext4: find old entry again if failed to rename whiteout Theodore Ts'o
2021-03-12  2:01   ` zhangyi (F)
2021-03-21  3:59     ` Theodore Ts'o

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).