Re: [PATCH v16 07/15] netfs: Explicitly pass tail padding to netfs_writeback_single()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] afs: race on i_size in afs_dir_writepages() triggers
  WARN_ON_ONCE()
- [Medium] netfs: netfs_writeback_single() does not set wreq->i_size to
  len
- [Medium] afs: i_size exceeding directory_size in writepages triggers
  WARN_ON_ONCE()
--

commit 85f14f8d404627b37bc4c8b65ec7be7797cc5f30
Author: David Howells <dhowells@redhat.com>

netfs: Explicitly pass tail padding to netfs_writeback_single()

This commit changes netfs_writeback_single() to take an explicit content
length alongside the full DIO-rounded buffer iterator, and updates AFS
directory and symlink writeback to zero the buffer tail and pass both
lengths.

> diff --git a/fs/afs/dir.c b/fs/afs/dir.c
> index 75f8f70cfeff0..51c158d8f723f 100644
> --- a/fs/afs/dir.c
> +++ b/fs/afs/dir.c

[ ... ]

> @@ -2228,9 +2236,10 @@ static int afs_dir_writepages(struct address_space *mapping,
>  	}
>  
>  	if (test_bit(AFS_VNODE_DIR_VALID, &dvnode->flags)) {
> -		iov_iter_folio_queue(&iter, ITER_SOURCE, dvnode->directory, 0, 0,
> -				     i_size_read(&dvnode->netfs.inode));
> -		ret = netfs_writeback_single(mapping, wbc, &iter);
> +		iov_iter_folio_queue(&iter, ITER_SOURCE, dvnode->directory,
> +				     0, 0, dvnode->directory_size);
> +		ret = netfs_writeback_single(mapping, wbc, &iter,
> +					     i_size_read(&dvnode->netfs.inode));
>  		if (ret == 1)
>  			ret = 0; /* Skipped write due to lock conflict. */
>  	}

[Severity: Medium]
Can a concurrent status update grow inode->i_size past
dvnode->directory_size here and trigger the WARN_ON_ONCE() in
netfs_writeback_single()?

RPC completion callbacks such as afs_do_lookup_success() call
afs_vnode_commit_status() -> afs_apply_status() without holding
dvnode->validate_lock:

CPU0                                  CPU1
afs_dir_writepages()
  down_read(&dvnode->validate_lock)
  test_bit(AFS_VNODE_DIR_VALID, ...)
  iov_iter_folio_queue(...,
    dvnode->directory_size)
                                      afs_do_lookup_success()
                                        ...
                                        afs_apply_status()
                                          afs_invalidate_dir()
                                          netfs_write_sizes()
  i_size_read(&dvnode->netfs.inode)
  netfs_writeback_single()
    WARN_ON_ONCE(iov_iter_count(iter) < len)  <-- triggers warning, -EIO

If a remote modification grows the directory beyond dvnode->directory_size
in that window, netfs_writeback_single() warns and fails writeback with
-EIO.

> diff --git a/fs/afs/symlink.c b/fs/afs/symlink.c
> index a1dbd552ddff0..c18e4f8064234 100644
> --- a/fs/afs/symlink.c
> +++ b/fs/afs/symlink.c
> @@ -228,6 +228,10 @@ int afs_readlink(struct dentry *dentry, char __user *buffer, int buflen)
>  /*
>   * Write the symlink contents to the cache as a single blob.  We then throw
>   * away the page we used to receive it.
> + *
> + * Note that the buffer tail is cleared under a shared lock on ->validate_lock
> + * - though hopefully there won't be racing writers since the only
> + * 'modification' is to be either read from the server or locally created.
>   */
>  int afs_symlink_writepages(struct address_space *mapping,
>  			   struct writeback_control *wbc)
> @@ -247,9 +251,17 @@ int afs_symlink_writepages(struct address_space *mapping,
>  
>  	if (vnode->directory &&
>  	    atomic64_read(&vnode->cb_expires_at) != AFS_NO_CB_PROMISE) {
> +		size_t i_size = i_size_read(&vnode->netfs.inode);
> +
> +		if (i_size < vnode->directory_size) {
> +			iov_iter_folio_queue(&iter, ITER_DEST, vnode->directory, 0, 0,
> +					     vnode->directory_size);
> +			iov_iter_advance(&iter, i_size);
> +			iov_iter_zero(vnode->directory_size - i_size, &iter);
> +		}
>  		iov_iter_folio_queue(&iter, ITER_SOURCE, vnode->directory, 0, 0,
> -				     i_size_read(&vnode->netfs.inode));
> -		ret = netfs_writeback_single(mapping, wbc, &iter);
> +				     vnode->directory_size);
> +		ret = netfs_writeback_single(mapping, wbc, &iter, i_size);
>  	}

[Severity: Medium]
Can i_size exceed vnode->directory_size in afs_symlink_writepages() and
trigger WARN_ON_ONCE(iov_iter_count(iter) < len) in
netfs_writeback_single()?

In afs_do_read_symlink(), vnode->directory is allocated to PAGE_SIZE.  If
netfs_read_single() fetches the symlink from the server and the returned
AFSFetchStatus reports status.size > PAGE_SIZE, afs_apply_status() updates
inode->i_size to status.size and sets the callback promise while
netfs_rreq_assess_single() marks the inode dirty.  afs_do_read_symlink()
then returns -EFBIG without freeing vnode->directory or clearing the dirty
state.  When afs_symlink_writepages() later runs, i_size is larger than
vnode->directory_size, causing netfs_writeback_single() to warn and return
-EIO (which also skips freeing vnode->directory).

> diff --git a/fs/netfs/write_issue.c b/fs/netfs/write_issue.c
> index b8237e1314d8a..e041023811ed0 100644
> --- a/fs/netfs/write_issue.c
> +++ b/fs/netfs/write_issue.c
> @@ -747,26 +747,33 @@ static int netfs_write_folio_single(struct netfs_io_request *wreq,
>   * netfs_writeback_single - Write back a monolithic payload
>   * @mapping: The mapping to write from
>   * @wbc: Hints from the VM
> - * @iter: Data to write, must be ITER_FOLIOQ.
> + * @iter: Data buffer to write from, must be ITER_FOLIOQ.
> + * @len: Amount of data to write.
>   *
>   * Write a monolithic, non-pagecache object back to the server and/or
>   * the cache.
>   *
> + * The data buffer may be bigger than @len (as set in @iter's count), and if
> + * so, this function will use the extra as needed to provide padding into the
> + * cache to handle DIO rounding, so the caller should clear it.
> + *
>   * Return: 0 if successful; 1 if skipped due to lock conflict and WB_SYNC_NONE;
>   * or a negative error code.
>   */
>  int netfs_writeback_single(struct address_space *mapping,
>  			   struct writeback_control *wbc,
> -			   struct iov_iter *iter)
> +			   struct iov_iter *iter, size_t len)
>  {
>  	struct netfs_io_request *wreq;
>  	struct netfs_inode *ictx = netfs_inode(mapping->host);
>  	struct folio_queue *fq;
> -	size_t size = iov_iter_count(iter);
> +	size_t size = len;
>  	int ret;
>  
>  	if (WARN_ON_ONCE(!iov_iter_is_folioq(iter)))
>  		return -EIO;
> +	if (WARN_ON_ONCE(iov_iter_count(iter) < len))
> +		return -EIO;
>  
>  	if (!netfs_wb_begin(ictx, wbc->sync_mode == WB_SYNC_NONE)) {
>  		/* The VFS will have undirtied the inode. */

[Severity: Medium]
Should netfs_writeback_single() initialize wreq->i_size from len instead of
letting netfs_create_write_req() -> netfs_alloc_request() re-read
i_size_read(inode)?

If inode->i_size changes concurrently (via afs_apply_status() ->
netfs_write_sizes()) between the caller passing len and
netfs_alloc_request() reading wreq->i_size, wreq->i_size and len disagree.
If wreq->i_size < len and the loop advances to a folio with fpos >
wreq->i_size, netfs_write_folio_single() underflows the unsigned
wreq->i_size - fpos subtraction:

fs/netfs/write_issue.c:netfs_write_folio_single() {
    ...
	flen = folio_size(folio);
	if (flen > wreq->i_size - fpos) {
		flen = wreq->i_size - fpos;
		folio_zero_segment(folio, flen, fsize);
		to_eof = true;
	} else if (flen == wreq->i_size - fpos) {
		to_eof = true;
	}
    ...
}

Although the follow-up commit "netfs: Change netfs_writeback_single() to
not look inside the iterator" removes netfs_write_folio_single() and sets
wreq->len = len, it still leaves wreq->i_size initialized from
i_size_read(inode), which causes cachefiles_issue_write() to skip DIO tail
padding when start + len != wreq->i_size and triggers WARN_ON(len &
(cache->bsize - 1)) in cachefiles_collect_write().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107160823.1465553-1-dhowells@redhat.com?part=7