linux-kernel - Re: [PATCH v2 2/2] ext4: protect ext4_release

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]

Message-ID: <87serc2bu5.fsf@gmail.com>
Date: Thu, 28 Nov 2024 10:28:58 +0530
From: Ritesh Harjani (IBM) <ritesh.list@...il.com>
To: Ojaswin Mujoo <ojaswin@...ux.ibm.com>, linux-ext4@...r.kernel.org, Jan Kara <jack@...e.com>
Cc: linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org, Baokun Li <libaokun1@...wei.com>, Jan Kara <jack@...e.cz>
Subject: Re: [PATCH v2 2/2] ext4: protect ext4_release_dquot against freezing

Ojaswin Mujoo <ojaswin@...ux.ibm.com> writes:

> Protect ext4_release_dquot against freezing so that we
> don't try to start a transaction when FS is frozen, leading
> to warnings.
>
> Further, avoid taking the freeze protection if a transaction
> is already running so that we don't need end up in a deadlock
> as described in
>
>   46e294efc355 ext4: fix deadlock with fs freezing and EA inodes
>
> Suggested-by: Jan Kara <jack@...e.cz>
> Signed-off-by: Ojaswin Mujoo <ojaswin@...ux.ibm.com>

Sorry for being late on this. Ideally, shouldn't it be the
responsibility of higher level FS (ext4) to make sure that
FS never freezes while there is pending work for releasing dquot
structures and that it should also prevent any context where such dquot 
structures gets added for release/delayed release. 

e.g. this is what FS takes care during freeze path i.e.
  freeze_super() -> sync_fs -> ext4_sync_fs()-> dquot_writeback_dquots() -> flush_delayed_work() (now fixed)

Now coming to iput() case which Jan mentioned [1] which could still
be called after FS have frozen. As I see we have a protection from FS
freeze in the ext4_evict_path() right? So ideally we should never see
dquot_drop() w/o fs freeze protection. And say, if the FS freezing immediately
happened after we scheduled this delayed work (but before the work gets
scheduled), then that will be taken care in the freeze_super() chain,
where we will flush all the delayed work no? - which is what Patch-1 is
fixing.

(There still might be an error handling path in ext4_evict_inode() ->
ext4_clear_inode() which we don't freeze protect. I still need to take a
closer look at that though).

So.. isn't this patch trying to hide the problem where FS failed to
freeze protect some code path?

-ritesh

> ---
>  fs/ext4/super.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 16a4ce704460..f7437a592359 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -6887,12 +6887,25 @@ static int ext4_release_dquot(struct dquot *dquot)
>  {
>  	int ret, err;
>  	handle_t *handle;
> +	bool freeze_protected = false;
> +
> +	/*
> +	 * Trying to sb_start_intwrite() in a running transaction
> +	 * can result in a deadlock. Further, running transactions
> +	 * are already protected from freezing.
> +	 */
> +	if (!ext4_journal_current_handle()) {
> +		sb_start_intwrite(dquot->dq_sb);
> +		freeze_protected = true;
> +	}
>  
>  	handle = ext4_journal_start(dquot_to_inode(dquot), EXT4_HT_QUOTA,
>  				    EXT4_QUOTA_DEL_BLOCKS(dquot->dq_sb));
>  	if (IS_ERR(handle)) {
>  		/* Release dquot anyway to avoid endless cycle in dqput() */
>  		dquot_release(dquot);
> +		if (freeze_protected)
> +			sb_end_intwrite(dquot->dq_sb);
>  		return PTR_ERR(handle);
>  	}
>  	ret = dquot_release(dquot);
> @@ -6903,6 +6916,10 @@ static int ext4_release_dquot(struct dquot *dquot)
>  	err = ext4_journal_stop(handle);
>  	if (!ret)
>  		ret = err;
> +
> +	if (freeze_protected)
> +		sb_end_intwrite(dquot->dq_sb);
> +
>  	return ret;
>  }
>  
> -- 
> 2.43.5