Re: [PATCH] shmem: do not extend i_size when no data was copied

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] shmem: shmem_write_end() marks and retains folios when copied is zero
--

commit 8699add3f60dd22c2b86a526ad2c46fc91ebbdec
Author: Hungyu Lin <dennylin0707@gmail.com>

shmem: do not extend i_size when no data was copied

This commit updates shmem_write_end() to only advance inode->i_size when
copied is non-zero, preventing buffered writes that fail to copy data (such
as when faulting in the source buffer fails with -EFAULT) from extending the
file size.

> diff --git a/mm/shmem.c b/mm/shmem.c
> index fd0953d55fd64..3dc14bc589a48 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -3488,7 +3488,7 @@ shmem_write_end(const struct kiocb *iocb, struct address_space *mapping,
>  {
>  	struct inode *inode = mapping->host;
>  
> -	if (pos + copied > inode->i_size)
> +	if (copied && pos + copied > inode->i_size)
>  		i_size_write(inode, pos + copied);
>  
>  	if (!folio_test_uptodate(folio)) {

[Severity: High]
When copied is zero (for example, when generic_perform_write() calls
copy_folio_from_iter_atomic() on an unreadable user buffer and then
fault_in_iov_iter_readable() fails with -EFAULT), skipping i_size_write()
while still running the rest of shmem_write_end() zeroes the folio, marks it
uptodate and dirty, and leaves it in mapping->i_pages:

mm/shmem.c:shmem_write_end() {
	if (copied && pos + copied > inode->i_size)
		i_size_write(inode, pos + copied);

	if (!folio_test_uptodate(folio)) {
		if (copied < folio_size(folio)) {
			size_t from = offset_in_folio(folio, pos);
			folio_zero_segments(folio, 0, from,
					from + copied, folio_size(folio));
		}
		folio_mark_uptodate(folio);
	}
	folio_mark_dirty(folio);
}

Can leaving the folio marked uptodate and dirty in mapping->i_pages when
copied is zero cause several regressions?

1. If a write fails on a hole or an unwritten preallocated folio created by
   shmem_fallocate(), shmem_get_folio_gfp() intentionally leaves the folio
   !folio_test_uptodate(folio) so that mapping_seek_hole_data() ->
   folio_seek_hole_data() treats it as a hole:

   mm/filemap.c:folio_seek_hole_data() {
	if (xa_is_value(folio) || folio_test_uptodate(folio))
		return seek_data ? start : end;
	if (!ops->is_partially_uptodate)
		return seek_data ? end : start;
   }

   Does marking the folio uptodate when copied is zero turn unwritten
   fallocated ranges and holes into SEEK_DATA?

2. When a write past i_size fails with copied == 0, shmem_write_begin() has
   already allocated and charged a folio at index = pos >> PAGE_SHIFT,
   incrementing info->alloced and inode->i_blocks. Because inode->i_size is
   not updated, the file retains allocated dirty blocks beyond EOF (visible
   in st_blocks via shmem_getattr()). If the file is later extended via
   ftruncate(), shmem_setattr() only calls shmem_truncate_range() when
   newsize <= oldsize:

   mm/shmem.c:shmem_setattr() {
		if (newsize <= oldsize) {
			loff_t holebegin = round_up(newsize, PAGE_SIZE);
			if (oldsize > holebegin)
				unmap_mapping_range(inode->i_mapping,
							holebegin, 0, 1);
			if (info->alloced)
				shmem_truncate_range(inode,
							newsize, (loff_t)-1);
   }

   Won't extending i_size with ftruncate() expose those uptodate folios left
   beyond oldsize as SEEK_DATA?

3. When large folios are enabled, shmem_write_begin() can allocate a large
   folio at folio->index >= DIV_ROUND_UP(i_size, PAGE_SIZE) (for instance,
   at folio->index == 0 when i_size == 0) and queue the inode on
   sbinfo->shrinklist via shmem_unused_huge_add(). When i_size remains 0 and
   info->fallocend is 0, shmem_unused_huge_shrink() computes end == 0 and
   bails out without splitting or freeing the folio:

   mm/shmem.c:shmem_unused_huge_shrink() {
		i_size = i_size_read(inode);
		folio = filemap_get_entry(inode->i_mapping, i_size / PAGE_SIZE);
		if (!folio || xa_is_value(folio))
			goto drop;
		...
		folio_end = folio_next_index(folio);
		end = shmem_fallocend(inode, DIV_ROUND_UP(i_size, PAGE_SIZE));
		if (end <= folio->index || end >= folio_end) {
			folio_put(folio);
			goto drop;
		}
   }

   Similarly, shmem_writeout() only splits a large folio if index >
   folio->index:

   mm/shmem.c:shmem_writeout() {
	if (folio_test_large(folio)) {
		index = shmem_fallocend(inode,
			DIV_ROUND_UP(i_size_read(inode), PAGE_SIZE));
		if ((index > folio->index && index < folio_next_index(folio)) ||
		    !IS_ENABLED(CONFIG_THP_SWAP))
			split = true;
	}
   }

   Doesn't this prevent both shmem_unused_huge_shrink() and shmem_writeout()
   from splitting large folios allocated past i_size when copied is zero?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261025164619.89992-1-dennylin0707@gmail.com?part=1