Re: [PATCH v16 03/15] netfs: Add some tools for managing bvecq chains

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] netfs: mempool deadlock in bvecq_alloc_chain() when
  for_writeback is true
- [Medium] netfs: uninitialized mem_type and bv[] in
  bvecq_alloc_buffer2() when size is 0
--

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

netfs: Add some tools for managing bvecq chains

This commit adds helper functions for allocating, populating, expanding,
and refcounting bvecq nodes and chains, along with a mempool for writeback
allocations.

> diff --git a/fs/netfs/bvecq.c b/fs/netfs/bvecq.c
> new file mode 100644
> index 0000000000000..06adb44ea93fc
> --- /dev/null
> +++ b/fs/netfs/bvecq.c

[ ... ]

> +struct bvecq *bvecq_alloc_chain(size_t nr_slots, gfp_t gfp, bool for_writeback)
> +{
> +	struct bvecq *head = NULL, *tail = NULL;
> +
> +	_enter("%zu", nr_slots);
> +
> +	for (;;) {
> +		struct bvecq *bq;
> +
> +		bq = bvecq_alloc_one(min(nr_slots, BVECQ_4KB_SLOTS), gfp, for_writeback);
> +		if (!bq)
> +			goto oom;
> +
> +		if (tail)
> +			bvecq_append(tail, bq);
> +		else
> +			head = bq;
> +		tail = bq;
> +		if (tail->max_slots >= nr_slots)
> +			break;
> +		nr_slots -= tail->max_slots;
> +	}
> +
> +	return head;
> +oom:
> +	bvecq_put(head);
> +	return NULL;
> +}
> +EXPORT_SYMBOL(bvecq_alloc_chain);

[Severity: High]
Can bvecq_alloc_chain() deadlock on netfs_bvecq_pool when for_writeback is
true?

Under memory pressure, bvecq_alloc_one() falls back to
mempool_alloc(&netfs_bvecq_pool, gfp).  Because this loop allocates
multiple nodes from the same 100-element pool while holding previously
allocated nodes in head, a single request needing more than 100 pool nodes
(~11.3 MiB) or concurrent writeback threads exhausting the pool midway
through their chains will sleep in mempool_alloc() waiting for elements
that they themselves are pinning.

> +
> +/**
> + * bvecq_alloc_buffer2 - Allocate a bvecq chain and populate with buffers
> + * @size: Target size of the buffer (can be 0 for an empty buffer)
> + * @pre_slots: Number of preamble slots to set aside
> + * @gfp: The allocation constraints.
> + * @for_writeback: True if allocating for writeback
> + *
> + * Allocate a chain of bvecq nodes and populate the slots with sufficient pages
> + * to provide at least the requested amount of space, leaving the first
> + * @pre_slots slots unset.  The pre-slots must all fit into the first bvecq.
> + * Note that any memory segments set in the pre_slots will be freed as
> + * specified by BVECQ_MEM_ALLOCED.
> + *
> + * The caller should assume that the pages allocated may be compound pages
> + * larger than PAGE_SIZE and thus occupy fewer slots.  The pages have their
> + * refcounts set to 1 and can be passed to MSG_SPLICE_PAGES.
> + *
> + * Return: The first node pointer or NULL on allocation failure.
> + */
> +struct bvecq *bvecq_alloc_buffer2(size_t size, unsigned int pre_slots, gfp_t gfp,
> +				  bool for_writeback)
> +{
> +	struct bvecq *head = NULL, *p = NULL;
> +	size_t nr_per_bq = BVECQ_POOL_SLOTS;
> +	size_t count = pre_slots + DIV_ROUND_UP(size, PAGE_SIZE);
> +
> +	_enter("%zx,%zx,%u", size, count, pre_slots);
> +
> +	if (WARN_ON_ONCE(pre_slots > nr_per_bq))
> +		return NULL;
> +
> +	head = bvecq_alloc_chain(count, gfp, for_writeback);
> +	if (!head)
> +		return NULL;
> +
> +	p = head;
> +	do {
> +		struct page **pages;
> +		size_t unused, want, got, slot;
> +
> +		if (!count)
> +			break;
> +		if (WARN_ON_ONCE(!p))
> +			goto oom;
> +
> +		if (p->nr_slots == 0) {
> +			/* Need to clear pre slots and pages[], so just clear all. */
> +			memset(p->bv, 0, p->max_slots * sizeof(p->bv[0]));
> +			p->mem_type = BVECQ_MEM_ALLOCED;
> +			p->nr_slots = pre_slots;
> +			count -= pre_slots;
> +			pre_slots = 0;
> +			if (!count)
> +				break;
> +		}

[Severity: Medium]
Does calling bvecq_alloc_buffer2() with size == 0 and pre_slots == 0 (for
instance via bvecq_alloc_buffer(0, ...)) leave p->bv uninitialised and
p->mem_type set to BVECQ_MEM_EXTERNAL instead of BVECQ_MEM_ALLOCED?

When both size and pre_slots are 0, count is 0, so the first if (!count)
check breaks out of the loop before the p->nr_slots == 0 block zeroes
p->bv and sets p->mem_type = BVECQ_MEM_ALLOCED.  In addition, passing
count == 0 to bvecq_alloc_chain() rounds up sizeof(*bq) alone, which yields
max_slots == 0 on 32-bit architectures (or 1 on 64-bit) when not allocated
from the mempool.  If the caller later populates slots in this node,
bvecq_put() will leak the pages, and bvecq_expand_buffer() will allocate a
new bvecq node leaving the empty head node unused.

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