linux-kernel - Re: [PATCH 3/4] iomap: don't increase i

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: <929f44c6-68e0-1a32-26fd-63b0e87f02fc@huaweicloud.com>
Date: Wed, 13 Mar 2024 15:09:17 +0800
From: Zhang Yi <yi.zhang@...weicloud.com>
To: "Darrick J. Wong" <djwong@...nel.org>
Cc: linux-xfs@...r.kernel.org, linux-fsdevel@...r.kernel.org,
 linux-kernel@...r.kernel.org, hch@...radead.org, brauner@...nel.org,
 david@...morbit.com, tytso@....edu, jack@...e.cz, yi.zhang@...wei.com,
 chengzhihao1@...wei.com, yukuai3@...wei.com
Subject: Re: [PATCH 3/4] iomap: don't increase i_size if it's not a write
 operation

On 2024/3/13 0:24, Darrick J. Wong wrote:
> On Tue, Mar 12, 2024 at 08:59:15PM +0800, Zhang Yi wrote:
>> On 2024/3/11 23:48, Darrick J. Wong wrote:
>>> On Mon, Mar 11, 2024 at 08:22:54PM +0800, Zhang Yi wrote:
>>>> From: Zhang Yi <yi.zhang@...wei.com>
>>>>
[...]
>>>> @@ -927,6 +908,24 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i)
>>>>  		copied = copy_folio_from_iter_atomic(folio, offset, bytes, i);
>>>>  		status = iomap_write_end(iter, pos, bytes, copied, folio);
>>>>  
>>>> +		/*
>>>> +		 * Update the in-memory inode size after copying the data into
>>>> +		 * the page cache.  It's up to the file system to write the
>>>> +		 * updated size to disk, preferably after I/O completion so that
>>>> +		 * no stale data is exposed.
>>>> +		 */
>>>> +		old_size = iter->inode->i_size;
>>>> +		if (pos + status > old_size) {
>>>> +			i_size_write(iter->inode, pos + status);
>>>> +			iter->iomap.flags |= IOMAP_F_SIZE_CHANGED;
>>>> +		}
>>>> +		__iomap_put_folio(iter, pos, status, folio);
>>>
>>> Why is it necessary to hoist the __iomap_put_folio calls from
>>> iomap_write_end into iomap_write_iter, iomap_unshare_iter, and
>>> iomap_zero_iter?  None of those functions seem to use it, and it makes
>>> more sense to me that iomap_write_end releases the folio that
>>> iomap_write_begin returned.
>>>
>>
>> Because we have to update i_size before __iomap_put_folio() in
>> iomap_write_iter(). If not, once we unlock folio, it could be raced
>> by the backgroud write back which could start writing back and call
>> folio_zero_segment() (please see iomap_writepage_handle_eof()) to
>> zero out the valid data beyond the not updated i_size. So we
>> have to move out __iomap_put_folio() out together with the i_size
>> updating.
> 
> Ahah.  Please make a note of that in the comment for dunces like me.
> 
> 	/*
> 	 * Update the in-memory inode size after copying the data into
> 	 * the page cache.  It's up to the file system to write the
> 	 * updated size to disk, preferably after I/O completion so that
> 	 * no stale data is exposed.  Only once that's done can we
> 	 * unlock and release the folio.
> 	 */
> 

Sure.