Re: [PATCH v17 11/17] afs: Use a bvecq to hold dir content rather than folioq

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] afs: TOCTOU race checking AFS_VNODE_DIR_VALID in
  afs_dir_writepages()
--

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

afs: Use a bvecq to hold dir content rather than folioq

This commit replaces folio_queue buffers with bvecq buffers for AFS
directories and symlinks, and snapshots the directory size, inode size, and
bvecq buffer under cb_lock during directory writeback.

Link: https://sashiko.dev/#/patchset/20261107160823.1465553-1-dhowells%40redhat.com

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

[ ... ]

> @@ -2220,6 +2220,7 @@ static int afs_dir_writepages(struct address_space *mapping,
>  			      struct writeback_control *wbc)
>  {
>  	struct afs_vnode *dvnode = AFS_FS_I(mapping->host);
> +	struct bvecq *directory = NULL;
>  	struct iov_iter iter;
>  	int ret = 0;
>  
> @@ -2236,14 +2237,21 @@ 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, dvnode->directory_size);
> -		ret = netfs_writeback_single(mapping, wbc, &iter,
> -					     i_size_read(&dvnode->netfs.inode));
> +		size_t i_size, d_size;
> +
> +		read_seqlock_excl(&dvnode->cb_lock);
> +		i_size = i_size_read(&dvnode->netfs.inode);
> +		d_size = dvnode->directory_size;
> +		directory = bvecq_get(dvnode->directory);
> +		read_sequnlock_excl(&dvnode->cb_lock);
> +
> +		iov_iter_bvec_queue(&iter, ITER_SOURCE, directory, 0, 0, d_size);
> +		ret = netfs_writeback_single(mapping, wbc, &iter, i_size);
>  		if (ret == 1)
>  			ret = 0; /* Skipped write due to lock conflict. */
>  	}

[Severity: Medium]
Can afs_dir_writepages() still read a new i_size paired with an old
directory_size if a third-party status update races between the
AFS_VNODE_DIR_VALID check and read_seqlock_excl(&dvnode->cb_lock)?

CPU0                              CPU1
afs_dir_writepages()
  down_read(&dvnode->validate_lock)
  test_bit(AFS_VNODE_DIR_VALID, ...)
                                  afs_vnode_commit_status()
                                    write_seqlock(&dvnode->cb_lock)
                                    afs_apply_status()
                                      afs_invalidate_dir()
                                        clear_bit(AFS_VNODE_DIR_VALID, ...)
                                      netfs_write_sizes()
                                        i_size_write(..., status->size)
                                    write_sequnlock(&dvnode->cb_lock)
  read_seqlock_excl(&dvnode->cb_lock)
  i_size = i_size_read(...)
  d_size = dvnode->directory_size
  read_sequnlock_excl(&dvnode->cb_lock)
  netfs_writeback_single(..., i_size)
    WARN_ON_ONCE(iov_iter_count(iter) < len)  <-- i_size > d_size

When afs_apply_status() processes a third-party directory change under
cb_lock without holding validate_lock, it clears AFS_VNODE_DIR_VALID and
updates inode->i_size to the new server size, while leaving
dvnode->directory_size and dvnode->directory unchanged until the directory
is later re-read.  If the directory grew on the server beyond d_size,
netfs_writeback_single() hits WARN_ON_ONCE(iov_iter_count(iter) < len) and
fails with -EIO; otherwise it writes the invalidated buffer with the new
i_size to the cache.

Re-checking AFS_VNODE_DIR_VALID inside the cb_lock critical section would
ensure i_size and directory_size come from a valid, consistent state.

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