Re: [Devel] [PATCH hci-8.1 v3 3/3] block/blk-cbt: account a write request also when dm starts it

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] block: blk_cbt_add() sleeps in cbt_page_alloc() mid RCU list walk, successor can be freed
--

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

block/blk-cbt: account a write request also when dm starts it

This patch accounts a write in CBT a second time when request based dm
starts the request, so that a request submitted before a dm suspend but
dispatched after resume is still tracked. To make this possible without
allocating under the dispatch RCU read lock, bitmap pages are now
preallocated at bio submission time in blk_cbt_add(). If a page is still
missing at dm start time, dm returns BLK_STS_RESOURCE and the page is
allocated from kblockd.

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

[ ... ]

> +/* Allocate the missing map pages for blocks [block, block + count) */
> +static void cbt_alloc_pages(struct cbt_info *cbt, blkcnt_t block,
> +			    blkcnt_t count)
> +{
> +	unsigned long idx, last;
> +
> +	if (unlikely(!count || block + count > cbt->block_max))
> +		return;
> +
> +	last = (block + count - 1) / BITS_PER_PAGE;
> +	for (idx = block / BITS_PER_PAGE; idx <= last; idx++) {
> +		struct cbt_info *c = cbt;
> +
[Severity: Medium]
> +		if (!CBT_PAGE(cbt, idx) && cbt_page_alloc(&c, idx, 1))
> +			return;

Can this sleep in the middle of the RCU list walk in blk_cbt_bio_queue()?

Passing in_rcu=1 makes cbt_page_alloc() do:

    if (in_rcu)
        rcu_read_unlock();
    page = alloc_page(GFP_NOIO|__GFP_ZERO);
    if (in_rcu)
        rcu_read_lock();

The call chain is:

blk_cbt_bio_queue()
  list_for_each_entry_rcu(cbt, &q->cbt_list, list)
    blk_cbt_add()
      cbt_alloc_pages()
        cbt_page_alloc()

The only thing pinned across the sleep is cbt->count of the current cbt,
which does not protect the next list entry. If the current cbt is
unlinked in that window, for example by blk_cbt_del() with
list_del_rcu(), its list.next is left unchanged. If the successor is also
removed and a grace period elapses (possible because this task holds no
RCU read lock while sleeping), the iterator then follows the stale next
pointer into freed memory.

This needs at least two CBTs on the queue and a concurrent stop or resize
operation, so the window is narrow. A similar shape already existed for
the eviction flush in blk_cbt_add() with in_rcu=1. However, the new call
extends it to the first write touching any bitmap page that is not yet
allocated, on every write submission.

The new allocation also ignores REQ_NOWAIT semantics on the submission
path. Would it be safer to avoid dropping the RCU read lock inside the
list walk, for example by deferring the allocation the way
cbt_queue_alloc() does?

> +	}
> +}
> +
>  static void blk_cbt_add(struct cbt_info *cbt, blkcnt_t start, blkcnt_t len)

[ ... ]

> +	/*
> +	 * Preallocate the pages for blk_cbt_rq_start(): it runs under the
> +	 * dispatch rcu_read_lock(), and a missing page there means delaying
> +	 * the request and allocating the page from the kblockd workqueue.
> +	 */
> +	cbt_alloc_pages(cbt, start, len);

This is the call site in blk_cbt_add() that the comment above refers to.
It is reached from blk_cbt_bio_queue() under rcu_read_lock() while
iterating q->cbt_list.

Thanks,

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

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