Re: [PATCH v17 14/17] netfs: Switch folioq to bvecq

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Critical] netfs: use-after-free on spent bvecq in
  netfs_read_to_pagecache()
- [High] netfs: premature bvecq deletion race in folio unlock functions
- [Low] netfs: commit message mismatch for rolling_buffer_delete_spent()
--

commit e56d7d03f56d46856c2af8166a341d7fbaef3849
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 and pins bvecq segments on subrequests across
retries.

>      (6) rolling_buffer_delete_spent() has to clear the bvecq->next pointer
>          before putting the bvecq to avoid rolling up the entire list.  On the
>          other hand, rolling_buffer_clear() just needs to put the tail.

[Severity: Low]
Does this description match rolling_buffer_delete_spent()?  Rather than
clearing spent->next, rolling_buffer_delete_spent() clears next->prev and
takes a reference on next via bvecq_get(next) before calling
bvecq_put(spent), leaving spent->next intact for subrequests pinning spent.

> diff --git a/fs/netfs/buffered_read.c b/fs/netfs/buffered_read.c
> index 69e68ef0652a7..c6c850ffd56ea 100644
> --- a/fs/netfs/buffered_read.c
> +++ b/fs/netfs/buffered_read.c
> @@ -131,6 +131,8 @@ static ssize_t netfs_prepare_read_iterator(struct netfs_io_subrequest *subreq)
>  	}
>  
>  	subreq->io_iter	= rreq->buffer.iter;
> +	if (iov_iter_is_bvecq(&subreq->io_iter))
> +		subreq->bvecq_pin = bvecq_get((struct bvecq *)subreq->io_iter.bvecq);
>  
>  	iov_iter_truncate(&subreq->io_iter, subreq->len);
>  	rolling_buffer_advance(&rreq->buffer, subreq->len);
> @@ -215,7 +217,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 +227,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]);

[ ... ]

> @@ -408,10 +410,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 rreq->buffer.iter.bvecq and the local bq pointer in
netfs_read_to_pagecache() be used after free when a read subrequest ends at a
bvecq segment boundary?

During readahead, rolling_buffer_bulk_load_from_ra() pre-populates the chain
of bvecq segments (bq0 -> bq1 -> ...), where rreq->buffer.tail holds a
reference on bq0 and bq0->next holds a reference on bq1, while neither
rreq->buffer.iter.bvecq nor bq in netfs_read_to_pagecache() holds a
reference.

Unlike iov_iter_folioq_advance(), iov_iter_bvecq_advance() only advances to
bq->next at the top of its loop:

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

		len = bq->bv[slot].bv_len;

		if (likely(by < len))
			break;
		by -= len;
		slot++;
	} while (by);
    ...
}

When a subrequest ends at the end of the last slot in bq0, both
rolling_buffer_advance() and netfs_mark_copy_to_cache() stop as soon as the
remaining length reaches 0, leaving rreq->buffer.iter.bvecq and bq pointing
to the exhausted bq0 at slot == bq0->nr_slots:

CPU0 (issuer)                     CPU1 (collector)
netfs_read_to_pagecache()
  netfs_prepare_read_iterator(sreq0)
    sreq0->bvecq_pin = bvecq_get(bq0)
    rolling_buffer_advance()
      /* leaves iter.bvecq == bq0,
         bvecq_slot == bq0->nr_slots */
  netfs_mark_copy_to_cache(&bq, ...)
    /* leaves bq == bq0,
       slot == bq0->nr_slots */
  netfs_issue_read(rreq, sreq0)
  netfs_maybe_bulk_drop_ra_refs()
                                  netfs_collect_read_results()
                                    netfs_read_unlock_folios()
                                      rolling_buffer_delete_spent()
                                        bvecq_put(bq0)
                                    netfs_put_subrequest(sreq0)
                                      bvecq_put(bq0)  <-- frees bq0
  /* next subrequest sreq1 */
  netfs_prepare_read_iterator(sreq1)
    bvecq_get(iter.bvecq)         <-- use-after-free on bq0
    rolling_buffer_advance()      <-- use-after-free on bq0
  netfs_mark_copy_to_cache(&bq)   <-- use-after-free on bq0

[ ... ]

> diff --git a/fs/netfs/write_collect.c b/fs/netfs/write_collect.c
> index 6e8ea534230df..91b42820c8925 100644
> --- a/fs/netfs/write_collect.c
> +++ b/fs/netfs/write_collect.c
> @@ -130,9 +130,9 @@ static void netfs_writeback_unlock_folios(struct netfs_io_request *wreq,
>  		return;
>  	}
>  
> -	if (slot >= folioq_nr_slots(folioq)) {
> -		folioq = rolling_buffer_delete_spent(&wreq->buffer);
> -		if (!folioq)
> +	while (!bvecq_acquire_slot(bq, slot)) {
> +		bq = rolling_buffer_delete_spent(&wreq->buffer);
> +		if (!bq)
>  			return;
>  		slot = 0;
>  	}

[ ... ]

> @@ -166,15 +166,15 @@ static void netfs_writeback_unlock_folios(struct netfs_io_request *wreq,
>  		wreq->cleaned_to = fpos + fsize;
>  		*notes |= MADE_PROGRESS;
>  
> -		/* Clean up the head folioq.  If we clear an entire folioq, then
> -		 * we can get rid of it provided it's not also the tail folioq
> +		/* Clean up the head bq.  If we clear an entire bq, then
> +		 * we can get rid of it provided it's not also the tail bq
>  		 * being filled by the issuer.
>  		 */
> -		folioq_clear(folioq, slot);
> +		bq->bv[slot].bv_page = NULL;
>  		slot++;
> -		if (slot >= folioq_nr_slots(folioq)) {
> -			folioq = rolling_buffer_delete_spent(&wreq->buffer);
> -			if (!folioq)
> +		while (!bvecq_acquire_slot(bq, slot)) {
> +			bq = rolling_buffer_delete_spent(&wreq->buffer);
> +			if (!bq)
>  				goto done;
>  			slot = 0;
>  		}
>  
>  		if (fpos + fsize >= collected_to)
>  			break;
>  	}

[Severity: High]
Can this race with the producer and delete a partially-consumed bvecq segment,
leaving the remaining folios in that segment permanently locked or under
writeback?

Previously, rolling_buffer_delete_spent() was only called when
slot >= folioq_nr_slots(folioq), which tested the fixed capacity of the
segment.  Here, bvecq_acquire_slot(bq, slot) tests slot < bq->nr_slots, and
it runs immediately after slot++ before checking fpos + fsize >= collected_to.
Because the producer updates bq->nr_slots before setting bq->next, while the
collector reads bq->nr_slots before reading bq->next in
rolling_buffer_delete_spent(), the collector can delete bq0 while uncollected
folios remain in slots k..max_slots - 1:

CPU0 (producer)                   CPU1 (collector)
netfs_write_folio()               netfs_writeback_unlock_folios()
  /* bq0 has k slots filled */      /* unlocks slot k - 1 */
                                    slot++ /* slot == k */
                                    bvecq_acquire_slot(bq0, k)
                                      /* sees nr_slots == k, false */
  /* fills slots k..max_slots-1
     of bq0 */
  rolling_buffer_make_space()
    bvecq_append(bq0, bq1)
      bq0->next = bq1
                                    rolling_buffer_delete_spent()
                                      /* sees bq0->next == bq1,
                                         deletes bq0! */
                                    slot = 0 /* on bq1 */
                                    /* slots k..max_slots-1 of bq0
                                       are skipped forever */

Those skipped folios in bq0 never have folio_end_writeback() called, hanging
writeback on the inode.  The same pattern is present in
netfs_pgpriv2_unlock_copied_folios() and netfs_read_unlock_folios().

Checking slot >= bq->max_slots (or checking slot >= bq->nr_slots only after
verifying bvecq_next(bq) is non-NULL with an acquire load) before calling
rolling_buffer_delete_spent() would avoid dropping a bvecq that still has
unprocessed slots.

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