[PATCH] ext4: Fix deadlock with fs freezing and EA inodes

[PATCH] ext4: Fix deadlock with fs freezing and EA inodes

[Jan Kara]

Xattr code using inodes with large xattr data can end up dropping last
inode reference (and thus deleting the inode) from places like
ext4_xattr_set_entry(). That function is called with transaction started
and so ext4_evict_inode() can deadlock against fs freezing like:

CPU1					CPU2

removexattr()				freeze_super()
  vfs_removexattr()
    ext4_xattr_set()
      handle = ext4_journal_start()
      ...
      ext4_xattr_set_entry()
        iput(old_ea_inode)
          ext4_evict_inode(old_ea_inode)
					  sb->s_writers.frozen = SB_FREEZE_FS;
					  sb_wait_write(sb, SB_FREEZE_FS);
					  ext4_freeze()
					    jbd2_journal_lock_updates()
					      -> blocks waiting for all
					         handles to stop
            sb_start_intwrite()
	      -> blocks as sb is already in SB_FREEZE_FS state

Generally it is advisable to delete inodes from a separate transaction
as it can consume quite some credits however in this case it would be
quite clumsy and furthermore the credits for inode deletion are quite
limited and already accounted for. So just tweak ext4_evict_inode() to
avoid freeze protection if we have transaction already started and thus
it is not really needed anyway.

CC: stable@vger.kernel.org
Fixes: dec214d00e0d ("ext4: xattr inode deduplication")
CC: Tahsin Erdogan <tahsin@google.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/inode.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 72534319fae5..777eb08b29cd 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -175,6 +175,7 @@ void ext4_evict_inode(struct inode *inode)
 	 */
 	int extra_credits = 6;
 	struct ext4_xattr_inode_array *ea_inode_array = NULL;
+	bool freeze_protected = false;
 
 	trace_ext4_evict_inode(inode);
 
@@ -232,9 +233,14 @@ void ext4_evict_inode(struct inode *inode)
 
 	/*
 	 * Protect us against freezing - iput() caller didn't have to have any
-	 * protection against it
+	 * protection against it. When we are in a running transaction though,
+	 * we are already protected against freezing and we cannot grab further
+	 * protection due to lock ordering constraints.
 	 */
-	sb_start_intwrite(inode->i_sb);
+	if (!ext4_journal_current_handle()) {
+		sb_start_intwrite(inode->i_sb);
+		freeze_protected = true;
+	}
 
 	if (!IS_NOQUOTA(inode))
 		extra_credits += EXT4_MAXQUOTAS_DEL_BLOCKS(inode->i_sb);
@@ -253,7 +259,8 @@ void ext4_evict_inode(struct inode *inode)
 		 * cleaned up.
 		 */
 		ext4_orphan_del(NULL, inode);
-		sb_end_intwrite(inode->i_sb);
+		if (freeze_protected)
+			sb_end_intwrite(inode->i_sb);
 		goto no_delete;
 	}
 
@@ -294,7 +301,8 @@ void ext4_evict_inode(struct inode *inode)
 stop_handle:
 		ext4_journal_stop(handle);
 		ext4_orphan_del(NULL, inode);
-		sb_end_intwrite(inode->i_sb);
+		if (freeze_protected)
+			sb_end_intwrite(inode->i_sb);
 		ext4_xattr_inode_array_free(ea_inode_array);
 		goto no_delete;
 	}
@@ -323,7 +331,8 @@ void ext4_evict_inode(struct inode *inode)
 	else
 		ext4_free_inode(handle, inode);
 	ext4_journal_stop(handle);
-	sb_end_intwrite(inode->i_sb);
+	if (freeze_protected)
+		sb_end_intwrite(inode->i_sb);
 	ext4_xattr_inode_array_free(ea_inode_array);
 	return;
 no_delete:
-- 
2.16.4

Re: [PATCH] ext4: Fix deadlock with fs freezing and EA inodes

[Andreas Dilger]

[-- Attachment #1: Type: text/plain, Size: 3542 bytes --]

On Nov 27, 2020, at 4:06 AM, Jan Kara <jack@suse.cz> wrote:
> 
> Xattr code using inodes with large xattr data can end up dropping last
> inode reference (and thus deleting the inode) from places like
> ext4_xattr_set_entry(). That function is called with transaction started
> and so ext4_evict_inode() can deadlock against fs freezing like:
> 
> CPU1					CPU2
> 
> removexattr()				freeze_super()
>  vfs_removexattr()
>    ext4_xattr_set()
>      handle = ext4_journal_start()
>      ...
>      ext4_xattr_set_entry()
>        iput(old_ea_inode)
>          ext4_evict_inode(old_ea_inode)
> 					  sb->s_writers.frozen = SB_FREEZE_FS;
> 					  sb_wait_write(sb, SB_FREEZE_FS);
> 					  ext4_freeze()
> 					    jbd2_journal_lock_updates()
> 					      -> blocks waiting for all
> 					         handles to stop
>            sb_start_intwrite()
> 	      -> blocks as sb is already in SB_FREEZE_FS state
> 
> Generally it is advisable to delete inodes from a separate transaction
> as it can consume quite some credits however in this case it would be
> quite clumsy and furthermore the credits for inode deletion are quite
> limited and already accounted for. So just tweak ext4_evict_inode() to
> avoid freeze protection if we have transaction already started and thus
> it is not really needed anyway.
> 
> CC: stable@vger.kernel.org
> Fixes: dec214d00e0d ("ext4: xattr inode deduplication")
> CC: Tahsin Erdogan <tahsin@google.com>
> Signed-off-by: Jan Kara <jack@suse.cz>

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

> ---
> fs/ext4/inode.c | 19 ++++++++++++++-----
> 1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 72534319fae5..777eb08b29cd 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -175,6 +175,7 @@ void ext4_evict_inode(struct inode *inode)
> 	 */
> 	int extra_credits = 6;
> 	struct ext4_xattr_inode_array *ea_inode_array = NULL;
> +	bool freeze_protected = false;
> 
> 	trace_ext4_evict_inode(inode);
> 
> @@ -232,9 +233,14 @@ void ext4_evict_inode(struct inode *inode)
> 
> 	/*
> 	 * Protect us against freezing - iput() caller didn't have to have any
> -	 * protection against it
> +	 * protection against it. When we are in a running transaction though,
> +	 * we are already protected against freezing and we cannot grab further
> +	 * protection due to lock ordering constraints.
> 	 */
> -	sb_start_intwrite(inode->i_sb);
> +	if (!ext4_journal_current_handle()) {
> +		sb_start_intwrite(inode->i_sb);
> +		freeze_protected = true;
> +	}
> 
> 	if (!IS_NOQUOTA(inode))
> 		extra_credits += EXT4_MAXQUOTAS_DEL_BLOCKS(inode->i_sb);
> @@ -253,7 +259,8 @@ void ext4_evict_inode(struct inode *inode)
> 		 * cleaned up.
> 		 */
> 		ext4_orphan_del(NULL, inode);
> -		sb_end_intwrite(inode->i_sb);
> +		if (freeze_protected)
> +			sb_end_intwrite(inode->i_sb);
> 		goto no_delete;
> 	}
> 
> @@ -294,7 +301,8 @@ void ext4_evict_inode(struct inode *inode)
> stop_handle:
> 		ext4_journal_stop(handle);
> 		ext4_orphan_del(NULL, inode);
> -		sb_end_intwrite(inode->i_sb);
> +		if (freeze_protected)
> +			sb_end_intwrite(inode->i_sb);
> 		ext4_xattr_inode_array_free(ea_inode_array);
> 		goto no_delete;
> 	}
> @@ -323,7 +331,8 @@ void ext4_evict_inode(struct inode *inode)
> 	else
> 		ext4_free_inode(handle, inode);
> 	ext4_journal_stop(handle);
> -	sb_end_intwrite(inode->i_sb);
> +	if (freeze_protected)
> +		sb_end_intwrite(inode->i_sb);
> 	ext4_xattr_inode_array_free(ea_inode_array);
> 	return;
> no_delete:
> --
> 2.16.4
> 


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

Re: [PATCH] ext4: Fix deadlock with fs freezing and EA inodes

[Theodore Y. Ts'o]

On Fri, Nov 27, 2020 at 12:06:49PM +0100, Jan Kara wrote:
> Xattr code using inodes with large xattr data can end up dropping last
> inode reference (and thus deleting the inode) from places like
> ext4_xattr_set_entry(). That function is called with transaction started
> and so ext4_evict_inode() can deadlock against fs freezing like:
> 
> CPU1					CPU2
> 
> removexattr()				freeze_super()
>   vfs_removexattr()
>     ext4_xattr_set()
>       handle = ext4_journal_start()
>       ...
>       ext4_xattr_set_entry()
>         iput(old_ea_inode)
>           ext4_evict_inode(old_ea_inode)
> 					  sb->s_writers.frozen = SB_FREEZE_FS;
> 					  sb_wait_write(sb, SB_FREEZE_FS);
> 					  ext4_freeze()
> 					    jbd2_journal_lock_updates()
> 					      -> blocks waiting for all
> 					         handles to stop
>             sb_start_intwrite()
> 	      -> blocks as sb is already in SB_FREEZE_FS state
> 
> Generally it is advisable to delete inodes from a separate transaction
> as it can consume quite some credits however in this case it would be
> quite clumsy and furthermore the credits for inode deletion are quite
> limited and already accounted for. So just tweak ext4_evict_inode() to
> avoid freeze protection if we have transaction already started and thus
> it is not really needed anyway.
> 
> CC: stable@vger.kernel.org
> Fixes: dec214d00e0d ("ext4: xattr inode deduplication")
> CC: Tahsin Erdogan <tahsin@google.com>
> Signed-off-by: Jan Kara <jack@suse.cz>

thanks, applied.

					- Ted