Re: [Devel] [PATCH hci-8.1 v5 5/6] block/blk-cbt: don't sleep in CBT for a REQ_NOWAIT write

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] block: blk-cbt: CBT_PAGE() double load lets nowait path see MISSED sentinel as a page
--

commit f17e6c3b3f1e49f78dddaddbbcaa6d9eb0d26f75
Author: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>

block/blk-cbt: don't sleep in CBT for a REQ_NOWAIT write

For a REQ_NOWAIT write, this patch makes blk_cbt_add() skip the per-cpu
extent cache and set the bits directly. It first checks with the new
cbt_pages_present() that every bitmap page exists. If one is missing,
submit_bio_noacct() fails the bio with BLK_STS_AGAIN. The throttle
dispatch work keeps the old, possibly sleeping, behaviour.

Link: https://virtuozzo.atlassian.net/browse/VSTOR-149268

> diff --git a/block/blk-cbt.c b/block/blk-cbt.c
> --- a/block/blk-cbt.c
> +++ b/block/blk-cbt.c

[ ... ]

> +/* Return true if all the map pages for [block, block + count) are there */
> +static bool cbt_pages_present(struct cbt_info *cbt, blkcnt_t block,
> +			      blkcnt_t count)
> +{
> +	unsigned long idx, last;
> +
> +	if (unlikely(!count || block + count > cbt->block_max))
> +		return true;
> +
> +	last = (block + count - 1) / BITS_PER_PAGE;
> +	for (idx = block / BITS_PER_PAGE; idx <= last; idx++)
> +		if (!CBT_PAGE(cbt, idx))
> +			return false;
> +
> +	return true;
> +}

[Severity: Medium]
Can this lockless check be fooled by a concurrent flush?

CBT_PAGE() expands to two plain loads of cbt->map[idx]:

    (cbt->map[idx] == CBT_PAGE_MISSED ? NULL : cbt->map[idx])

There is no READ_ONCE().  The writer is __blk_cbt_set(), reached from
cbt_flush_cache() with pages_missed set.  It stores the sentinel under
cbt->lock:

    if (!installed)
            cbt->map[idx] = CBT_PAGE_MISSED;

The reader here takes no lock, only cbt_srcu.  Suppose the first load in
CBT_PAGE() sees NULL and the flusher then stores CBT_PAGE_MISSED.  The
second load returns CBT_PAGE_MISSED, which is (struct page *)1.  The
compiler may also legally reload the slot.

In that case cbt_pages_present() treats a missing page as present.

[ ... ]

> +	if (unlikely(nowait)) {
> +		/* Don't sleep: set the bits directly, only with all pages there */
> +		if (!cbt_pages_present(cbt, start, len))
> +			return true;
> +		__blk_cbt_set(cbt, start, len, 1, NULL, NULL);
> +		return false;
> +	}

[Severity: Medium]
If cbt_pages_present() was fooled as described above, what happens in
this __blk_cbt_set() call with pages_missed == NULL?

__blk_cbt_set() reads the slot again through CBT_PAGE().  If it now sees
CBT_PAGE_MISSED, page is NULL and pages_missed is NULL, so it falls
through to cbt_page_alloc().  That allocates with GFP_NOIO and can sleep
in a REQ_NOWAIT submission, which is the case this patch is meant to
avoid.

If the double load tears again, page_address() and the page lock are
applied to (struct page *)1, which looks like it would oops.

The window is narrow and only exists while a flush is installing the
sentinel.  The same CBT_PAGE() race already existed in __blk_cbt_set().
This patch adds a new lockless reader and makes the no-sleep guarantee
depend on it being exact.

Would it make sense to load the slot once with READ_ONCE() in CBT_PAGE()
(or in cbt_pages_present()), and to make the nowait path in
__blk_cbt_set() fail instead of allocating when a page is missing?

The follow-up patch "block/blk-cbt: account a write request also when dm
starts it" does not change CBT_PAGE(), add READ_ONCE(), or add locking,
so the issue remains in the final series state.

[ ... ]

-- 
Sashiko AI review · https://sashiko.vzint.dev/#/patchset/20261027-ptikhomirov-vstor-149268-cbt-account-at-rq-start-v5-0-88dccc74b48b@virtuozzo.com?part=5

_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel