linux-kernel - Re: [PATCH 5/6] iomap: Lift blocksize restriction on atomic writes

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: <87zfmsmsvc.fsf@gmail.com>
Date: Fri, 25 Oct 2024 15:01:03 +0530
From: Ritesh Harjani (IBM) <ritesh.list@...il.com>
To: John Garry <john.g.garry@...cle.com>, linux-ext4@...r.kernel.org
Cc: Theodore Ts'o <tytso@....edu>, Jan Kara <jack@...e.cz>, "Darrick J . Wong" <djwong@...nel.org>, Christoph Hellwig <hch@...radead.org>, Ojaswin Mujoo <ojaswin@...ux.ibm.com>, Dave Chinner <david@...morbit.com>, linux-kernel@...r.kernel.org, linux-xfs@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH 5/6] iomap: Lift blocksize restriction on atomic writes

Hi John, 

John Garry <john.g.garry@...cle.com> writes:

> On 25/10/2024 04:45, Ritesh Harjani (IBM) wrote:
>> Filesystems like ext4 can submit writes in multiples of blocksizes.
>> But we still can't allow the writes to be split. Hence let's check if
>> the iomap_length() is same as iter->len or not.
>> 
>> This shouldn't affect XFS since it anyways checks for this in
>> xfs_file_write_iter() to not support atomic write size request of more
>> than FS blocksize.
>> 
>> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@...il.com>
>> ---
>>   fs/iomap/direct-io.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
>> index ed4764e3b8f0..1d33b4239b3e 100644
>> --- a/fs/iomap/direct-io.c
>> +++ b/fs/iomap/direct-io.c
>> @@ -306,7 +306,7 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter,
>>   	size_t copied = 0;
>>   	size_t orig_count;
>>   
>> -	if (atomic && length != fs_block_size)
>> +	if (atomic && length != iter->len)
>>   		return -EINVAL;
>
> Here you expect just one iter for an atomic write always.

Here we are lifting the limitation of iomap to support entire iter->len
rather than just 1 fsblock. 

>
> In 6/6, you are saying that iomap does not allow an atomic write which 
> covers unwritten and written extents, right?

No, it's not that. If FS does not provide a full mapping to iomap in
->iomap_begin then the writes will get split. For atomic writes this
should not happen, hence the check in iomap above to return -EINVAL if
mapped length does not match iter->len.

>
> But for writing a single fs block atomically, we don't mandate it to be 
> in unwritten state. So there is a difference in behavior in writing a 
> single FS block vs multiple FS blocks atomically.

Same as mentioned above. We can't have atomic writes to get split.
This patch is just lifting the restriction of iomap to allow more than
blocksize but the mapped length should still meet iter->len, as
otherwise the writes can get split.

>
> So we have 3x choices, as I see:
> a. add a check now in iomap that the extent is in written state (for an 
> atomic write)
> b. add extent zeroing code, as I was trying for originally
> c. document this peculiarity
>
> Thanks,
> John