linux-ext4 - Re: [PATCH v3 03/10] ext4: fix stale data if it bail out of the extents mapping loop

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: <fa74ce4b-6dd0-4f65-8daf-36faa94709ef@huaweicloud.com>
Date: Thu, 3 Jul 2025 10:05:14 +0800
From: Zhang Yi <yi.zhang@...weicloud.com>
To: Jan Kara <jack@...e.cz>
Cc: linux-ext4@...r.kernel.org, linux-fsdevel@...r.kernel.org,
 linux-kernel@...r.kernel.org, tytso@....edu, adilger.kernel@...ger.ca,
 ojaswin@...ux.ibm.com, sashal@...nel.org, yi.zhang@...wei.com,
 libaokun1@...wei.com, yukuai3@...wei.com, yangerkun@...wei.com
Subject: Re: [PATCH v3 03/10] ext4: fix stale data if it bail out of the
 extents mapping loop

On 2025/7/2 22:07, Jan Kara wrote:
> On Tue 01-07-25 21:06:28, Zhang Yi wrote:
>> From: Zhang Yi <yi.zhang@...wei.com>
>>
>> During the process of writing back folios, if
>> mpage_map_and_submit_extent() exits the extent mapping loop due to an
>> ENOSPC or ENOMEM error, it may result in stale data or filesystem
>> inconsistency in environments where the block size is smaller than the
>> folio size.
>>
>> When mapping a discontinuous folio in mpage_map_and_submit_extent(),
>> some buffers may have already be mapped. If we exit the mapping loop
>> prematurely, the folio data within the mapped range will not be written
>> back, and the file's disk size will not be updated. Once the transaction
>> that includes this range of extents is committed, this can lead to stale
>> data or filesystem inconsistency.
>>
>> Fix this by submitting the current processing partially mapped folio.
>>
>> Suggested-by: Jan Kara <jack@...e.cz>
>> Signed-off-by: Zhang Yi <yi.zhang@...wei.com>
> 
> Looks good. Feel free to add:
> 
> Reviewed-by: Jan Kara <jack@...e.cz>
> 
> Just one naming suggestion below:
> 
>> +/*
>> + * This is used to submit mapped buffers in a single folio that is not fully
>> + * mapped for various reasons, such as insufficient space or journal credits.
>> + */
>> +static int mpage_submit_buffers(struct mpage_da_data *mpd)
> 
> mpage_submit_buffers() sounds somewhat generic. How about
> mpage_submit_partial_folio()?
> 
> 								Honza
>

Yeah, mpage_submit_partial_folio looks better to me.

Thanks,
Yi.


>> +{
>> +	struct inode *inode = mpd->inode;
>> +	struct folio *folio;
>> +	loff_t pos;
>> +	int ret;
>> +
>> +	folio = filemap_get_folio(inode->i_mapping,
>> +				  mpd->start_pos >> PAGE_SHIFT);
>> +	if (IS_ERR(folio))
>> +		return PTR_ERR(folio);
>> +	/*
>> +	 * The mapped position should be within the current processing folio
>> +	 * but must not be the folio start position.
>> +	 */
>> +	pos = mpd->map.m_lblk << inode->i_blkbits;
>> +	if (WARN_ON_ONCE((folio_pos(folio) == pos) ||
>> +			 !folio_contains(folio, pos >> PAGE_SHIFT)))
>> +		return -EINVAL;
>> +
>> +	ret = mpage_submit_folio(mpd, folio);
>> +	if (ret)
>> +		goto out;
>> +	/*
>> +	 * Update start_pos to prevent this folio from being released in
>> +	 * mpage_release_unused_pages(), it will be reset to the aligned folio
>> +	 * pos when this folio is written again in the next round. Additionally,
>> +	 * do not update wbc->nr_to_write here, as it will be updated once the
>> +	 * entire folio has finished processing.
>> +	 */
>> +	mpd->start_pos = pos;
>> +out:
>> +	folio_unlock(folio);
>> +	folio_put(folio);
>> +	return ret;
>> +}
>> +
>>  /*
>>   * mpage_map_and_submit_extent - map extent starting at mpd->lblk of length
>>   *				 mpd->len and submit pages underlying it for IO
>> @@ -2411,8 +2452,16 @@ static int mpage_map_and_submit_extent(handle_t *handle,
>>  			 */
>>  			if ((err == -ENOMEM) ||
>>  			    (err == -ENOSPC && ext4_count_free_clusters(sb))) {
>> -				if (progress)
>> +				/*
>> +				 * We may have already allocated extents for
>> +				 * some bhs inside the folio, issue the
>> +				 * corresponding data to prevent stale data.
>> +				 */
>> +				if (progress) {
>> +					if (mpage_submit_buffers(mpd))
>> +						goto invalidate_dirty_pages;
>>  					goto update_disksize;
>> +				}
>>  				return err;
>>  			}
>>  			ext4_msg(sb, KERN_CRIT,
>> -- 
>> 2.46.1
>>