Re: [PATCH 1/2] iomap: don't skip reading in !uptodate folios when unsharing a range

[Ritesh Harjani (IBM) <ritesh.list@gmail.com>]

Ritesh Harjani (IBM) <ritesh.list@gmail.com> writes:

> "Darrick J. Wong" <djwong@kernel.org> writes:
>
>> From: Darrick J. Wong <djwong@kernel.org>
>>
>> Prior to commit a01b8f225248e, we would always read in the contents of a
>> !uptodate folio prior to writing userspace data into the folio,
>> allocated a folio state object, etc.  Ritesh introduced an optimization
>> that skips all of that if the write would cover the entire folio.
>>
>> Unfortunately, the optimization misses the unshare case, where we always
>> have to read in the folio contents since there isn't a data buffer
>> supplied by userspace.  This can result in stale kernel memory exposure
>> if userspace issues a FALLOC_FL_UNSHARE_RANGE call on part of a shared
>> file that isn't already cached.
>>
>> This was caught by observing fstests regressions in the "unshare around"
>> mechanism that is used for unaligned writes to a reflinked realtime
>> volume when the realtime extent size is larger than 1FSB, though I think
>> it applies to any shared file.
>>
>> Cc: ritesh.list@gmail.com, willy@infradead.org
>> Fixes: a01b8f225248e ("iomap: Allocate ifs in ->write_begin() early")
>> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
>> ---
>>  fs/iomap/buffered-io.c |    6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> Thanks for catching this case. Fix for this looks good to me. 
> I have verified on my setup. w/o this patch it indeed can cause
> corruption in the unshare case, since we don't read the disk contents
> and we might end up writing garbage from the page cache.

To add more info to my above review. iomap_write_begin() is used by 
1. iomap_write_iter()
2. iomap_zero_iter()
3. iomap_unshare_iter()

And looks like out of the 3, iomap_unshare_iter() is the only one which
will not write anything to the folio in the foliocache, & we
definitely need to read the extent in folio cache in iomap_write_begin()
for unsharing.

Hence I believe iomap_unshare_iter() should be the only path to be
fixed, which this patch does by checking IOMAP_UNSHARE flag in
__iomap_write_begin().

>
> Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>
>

-ritesh