Re: [PATCH v16 12/15] netfs: Switch folioq to bvecq

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Critical] netfs: use-after-free on bvecq boundary in
  netfs_read_to_pagecache()
- [High] netfs: use-after-free in netfs_reset_iter() on subrequest retry
--

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

netfs: Switch folioq to bvecq

This commit switches netfslib's rolling_buffer and buffered read/writeback
paths from folio_queue to bvecq.

> diff --git a/fs/netfs/buffered_read.c b/fs/netfs/buffered_read.c
> index 69e68ef0652a7..61b44af6774b7 100644
> --- a/fs/netfs/buffered_read.c
> +++ b/fs/netfs/buffered_read.c
> @@ -215,7 +215,7 @@ static void netfs_issue_read(struct netfs_io_request *rreq,
>   * otherwise we set the deprecated PG_private_2.
>   */
>  static void netfs_mark_copy_to_cache(struct netfs_io_request *rreq,
> -				     struct folio_queue **fq,
> +				     struct bvecq **bq,
>  				     unsigned int *offset,
>  				     int *slot,
>  				     size_t len,
> @@ -225,21 +225,21 @@ static void netfs_mark_copy_to_cache(struct netfs_io_request *rreq,
>  		struct folio *folio;
>  		size_t fsize, overlap;
>  
> -		if (!*fq)
> +		if (!*bq)
>  			break;
> -		if (*slot >= folioq_count(*fq)) {
> -			*fq = (*fq)->next;
> +		if (!bvecq_acquire_slot(*bq, *slot)) {
> +			*bq = bvecq_next(*bq);
>  			*slot = 0;
>  			*offset = 0;
>  			continue;
>  		}
>  
>  		/* Determine how much the subreq overlaps the folio, if at all. */
> -		fsize = folioq_folio_size(*fq, *slot);
> +		fsize = (*bq)->bv[*slot].bv_len;
>  		overlap = min(len, fsize - *offset);
>  
>  		if (overlap > 0 && copy) {
> -			folio = folioq_folio(*fq, *slot);
> +			folio = bvec_folio(&(*bq)->bv[*slot]);
>  			if (netfs_using_pgpriv2(rreq)) {
>  				if (!folio_test_private_2(folio))
>  					folio_start_private_2(folio);
> @@ -275,7 +275,7 @@ static void netfs_read_to_pagecache(struct netfs_io_request *rreq)
>  		.cached_to[1]	= ULLONG_MAX,
>  	};
>  	struct fscache_occupancy *occ = &_occ;
> -	struct folio_queue *fq = rreq->buffer.tail;
> +	struct bvecq *bq = rreq->buffer.tail;
>  	unsigned int offset = 0;
>  	ssize_t size = rreq->len;
>  	uoff_t start = rreq->start;
> @@ -408,10 +408,10 @@ static void netfs_read_to_pagecache(struct netfs_io_request *rreq)
>  		if (size <= 0)
>  			netfs_all_subreqs_queued(rreq);
>  
> -		if (fq) {
> +		if (bq) {
>  			/* See if the cache indicated this should be cached. */
>  			copy = test_bit(NETFS_SREQ_COPY_TO_CACHE, &subreq->flags);
> -			netfs_mark_copy_to_cache(rreq, &fq, &slot, &offset, slice, copy);
> +			netfs_mark_copy_to_cache(rreq, &bq, &slot, &offset, slice, copy);
>  		}
>  
>  		trace_netfs_sreq(subreq, netfs_sreq_trace_submit);

[Severity: Critical]
Can a subrequest ending at a bvecq boundary leave rreq->buffer.iter and bq
pointing at a spent bvecq that gets freed before the next subrequest steps
to bq->next?

In netfs_readahead(), rolling_buffer_bulk_load_from_ra() builds a multi-bvecq
chain (bq0 -> bq1 -> ...) upfront with refcount 1 on each segment.  Unlike
iov_iter_folioq_advance(), which stepped to folioq->next as soon as slot hit
folioq_nr_slots(), iov_iter_bvecq_advance() only steps to bq->next at the top
of its do-while (by) loop:

lib/iov_iter.c:iov_iter_bvecq_advance() {
	...
	do {
		if (slot >= bq->nr_slots) {
			if (!bq->next)
				break;
			bq = bq->next;
			slot = 0;
			continue;
		}
		...
		by -= len;
		slot++;
	} while (by);
}

netfs_mark_copy_to_cache() does the same in its while (len > 0) loop.  When a
subrequest ends at the end of bq0, both rreq->buffer.iter.bvecq and bq are
left pointing at bq0 with slot == bq0->nr_slots, and the next subrequest's
subreq->io_iter copies rreq->buffer.iter before advancing:

CPU0 (issuer)                     CPU1 (collector)
netfs_read_to_pagecache()
  subreq1 covers all of bq0
  netfs_prepare_read_iterator(subreq1)
    rolling_buffer_advance()
      /* iter.bvecq == bq0,
         bvecq_slot == bq0->nr_slots */
  netfs_mark_copy_to_cache()
    /* bq == bq0,
       slot == bq0->nr_slots */
  netfs_issue_read(subreq1)
  netfs_maybe_bulk_drop_ra_refs()
                                  netfs_read_unlock_folios()
                                    rolling_buffer_delete_spent()
                                      bq0->next = NULL;
                                      bvecq_put(bq0);  <-- frees bq0
  subreq2 prepared for bq1:
  netfs_prepare_read_iterator(subreq2)
    netfs_limit_bvecq(&rreq->buffer.iter)
      bvecq_next(bq0)             <-- use-after-free, sees NULL
  netfs_mark_copy_to_cache(&bq)
    bvecq_next(bq0)               <-- use-after-free, sees NULL

The same happens on a single CPU when subreq1 is NETFS_FILL_WITH_ZEROES and
completes synchronously inside netfs_issue_read().  Once bq0 is freed and its
next pointer is cleared, subreq2's iterator is truncated to 0 bytes and the
remaining folios in bq1 are never populated or unlocked.

> diff --git a/fs/netfs/rolling_buffer.c b/fs/netfs/rolling_buffer.c
> index 4347c99452d83..76cede046dcb7 100644
> --- a/fs/netfs/rolling_buffer.c
> +++ b/fs/netfs/rolling_buffer.c

[ ... ]

> @@ -205,44 +205,23 @@ ssize_t rolling_buffer_append(struct rolling_buffer *roll, struct folio *folio,
>   * don't return the last buffer to keep the pointers independent, but return
>   * NULL instead.
>   */
> -struct folio_queue *rolling_buffer_delete_spent(struct rolling_buffer *roll)
> +struct bvecq *rolling_buffer_delete_spent(struct rolling_buffer *roll)
>  {
> -	struct folio_queue *spent = roll->tail, *next = READ_ONCE(spent->next);
> +	struct bvecq *spent = roll->tail, *next = bvecq_next(spent);
>  
>  	if (!next)
>  		return NULL;
>  	next->prev = NULL;
> -	netfs_folioq_free(spent, netfs_trace_folioq_delete);
>  	roll->tail = next;
> +	spent->next = NULL;
> +	bvecq_put(spent);
>  	return next;
>  }

[Severity: High]
Does freeing spent bvecq segments during collection cause a use-after-free in
netfs_reset_iter() when a subrequest spanning multiple bvecq segments is
retried?

subreq->io_iter is initialized from rreq->buffer.iter (or wreq->buffer.iter)
without taking a reference on its starting bvecq segment (bq0).  If a
subrequest spans bq0 and bq1, transfers enough data to finish bq0, and then
fails with NETFS_SREQ_NEED_RETRY, both netfs_collect_read_results() and
netfs_collect_write_results() unlock the completed folios in bq0 before
entering the retry path:

fs/netfs/write_collect.c:netfs_collect_write_results() {
	...
	if (notes & NEED_UNLOCK) {
		if (wreq->cleaned_to < wreq->collected_to)
			netfs_writeback_unlock_folios(wreq, &notes);
	} else {
		wreq->cleaned_to = wreq->collected_to;
	}

	// TODO: Discard encryption buffers

	if (notes & NEED_RETRY)
		goto need_retry;
	...
}

netfs_writeback_unlock_folios() (and netfs_read_unlock_folios() on the read
side) calls rolling_buffer_delete_spent(), which clears bq0->next = NULL and
frees bq0.  When netfs_retry_writes() or netfs_retry_reads() then calls
netfs_reset_iter(subreq) -> iov_iter_advance(&subreq->io_iter, ...),
iov_iter_bvecq_advance() dereferences the freed bq0 from subreq->io_iter.bvecq
and cannot advance to bq1 because bq0->next was cleared.

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