linux-ext4 - 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: <8885691f-b977-409f-90f8-b24c41a49de0@huawei.com>
Date: Tue, 3 Dec 2024 16:29:31 +0800
From: Baokun Li <libaokun1@...wei.com>
To: Ojaswin Mujoo <ojaswin@...ux.ibm.com>
CC: <linux-ext4@...r.kernel.org>, Jan Kara <jack@...e.com>, Ritesh Harjani
	<ritesh.list@...il.com>, <linux-kernel@...r.kernel.org>,
	<linux-fsdevel@...r.kernel.org>, Jan Kara <jack@...e.cz>, Yang Erkun
	<yangerkun@...wei.com>
Subject: Re: [PATCH v2 2/2] ext4: protect ext4_release_dquot against freezing

On 2024/11/27 14:01, Ojaswin Mujoo wrote:
> On Tue, Nov 26, 2024 at 10:49:14PM +0800, Baokun Li wrote:
>> On 2024/11/21 20:38, Ojaswin Mujoo wrote:
>>> 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>
>>> ---
>>>    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)
>> The `git am` command looks for the following context code from line 6903
>> to apply the changes. But there are many functions in fs/ext4/super.c that
>> have similar code, such as ext4_write_dquot() and ext4_acquire_dquot().
> Oh that's strange, shouldn't it match the complete line like:
A rough look at the `git am` source code looks like it only focuses on
line numbers between two ‘@@’.

am_run
  run_apply
   apply_all_patches
    apply_patch
     parse_chunk
      find_header
       parse_fragment_header
     check_patch_list
      check_patch
       apply_data
        load_preimage
        apply_fragments
         apply_one_fragment
          find_pos
           match_fragment
>>> @@ -6903,6 +6916,10 @@ static int ext4_release_dquot(struct dquot *dquot)
> That should only have one occurence around line 6903? Or does it try to
> fuzzy match which ends up matching ext4_write_dquot etc?
In find_pos(), start from line 6903, compare the hash value of each line
of code line by line in forward direction, if it can't match, then match
the hash value of each line of code line by line in reverse direction from
line 6903. Fuzzy matching is used if some whitespace characters should be
ignored.


Regards,
Baokun