Re: [PATCH v17 08/17] netfs: Explicitly pass tail padding to netfs_writeback_single()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] afs: i_size exceeding directory_size in writepages triggers
  WARN_ON_ONCE()
--

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

netfs: Explicitly pass tail padding to netfs_writeback_single()

This commit adds an explicit content length parameter to
netfs_writeback_single() so the caller provides a padded buffer via the
iterator while specifying the actual content size separately, and updates
AFS directory and symlink writeback to zero tail padding and pass both
sizes.

> 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. */
>  	}
> 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
[ ... ]
> @@ -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);
>  	}
>  
>  	if (ret == 0) {
> 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,
>  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;

[Severity: Medium]
Can i_size exceed directory_size in afs_dir_writepages() or
afs_symlink_writepages() and trigger the new
WARN_ON_ONCE(iov_iter_count(iter) < len) in netfs_writeback_single()?

In afs_dir_writepages(), dvnode->validate_lock is held shared while
checking AFS_VNODE_DIR_VALID and reading dvnode->directory_size and
inode->i_size.  A concurrent RPC completion such as
afs_do_lookup_success() calls afs_vnode_commit_status() ->
afs_apply_status() under cb_lock and i_lock without holding validate_lock.
If a third party grew the directory on the server beyond
dvnode->directory_size, afs_apply_status() clears AFS_VNODE_DIR_VALID and
updates inode->i_size without updating dvnode->directory_size:

CPU0                              CPU1
afs_dir_writepages()
  down_read(&dvnode->validate_lock)
  test_bit(AFS_VNODE_DIR_VALID, ...)
                                  afs_vnode_commit_status()
                                    afs_apply_status()
                                      afs_invalidate_dir()
                                        clear_bit(AFS_VNODE_DIR_VALID, ...)
                                      i_size_write(inode, status->size)
  iov_iter_folio_queue(...,
    dvnode->directory_size)
  netfs_writeback_single(...,
    i_size_read(...))
    WARN_ON_ONCE(iov_iter_count(iter) < len)  <-- triggers warning

Even after commit "afs: Use a bvecq to hold dir content rather than
folioq" later in this series reads i_size and directory_size under
dvnode->cb_lock, test_bit(AFS_VNODE_DIR_VALID, &dvnode->flags) is still
checked before acquiring cb_lock, so the same race window remains.

Similarly, afs_do_read_symlink() allocates vnode->directory to PAGE_SIZE.
If the server returns a symlink status with size > PAGE_SIZE in
netfs_read_single() (or a concurrent status update grows inode->i_size
past PAGE_SIZE and sets AFS_VNODE_ZAP_DATA), netfs_rreq_assess_single()
still marks the inode dirty before afs_do_read_symlink() returns -EFBIG.
When afs_symlink_writepages() runs, it does not check AFS_VNODE_ZAP_DATA
or whether i_size <= vnode->directory_size, hitting the same
WARN_ON_ONCE() and returning -EIO (which also skips freeing
vnode->directory until eviction).

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