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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 29 Nov 2020 14:32:23 -0700
From:   Andreas Dilger <adilger@...ger.ca>
To:     Jan Kara <jack@...e.cz>
Cc:     Ted Tso <tytso@....edu>, linux-ext4@...r.kernel.org,
        stable@...r.kernel.org, Tahsin Erdogan <tahsin@...gle.com>
Subject: Re: [PATCH] ext4: Fix deadlock with fs freezing and EA inodes

On Nov 27, 2020, at 4:06 AM, Jan Kara <jack@...e.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@...r.kernel.org
> Fixes: dec214d00e0d ("ext4: xattr inode deduplication")
> CC: Tahsin Erdogan <tahsin@...gle.com>
> Signed-off-by: Jan Kara <jack@...e.cz>

Reviewed-by: Andreas Dilger <adilger@...ger.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






Download attachment "signature.asc" of type "application/pgp-signature" (874 bytes)