Re: [Devel] [PATCH hci-8.1 v6 5/7] block/blk-cbt: walk cbt_list under SRCU

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] block/blk-cbt: blk_cbt_update_size() iterator reads ->next of a cbt already queued for call_srcu() free
--

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

block/blk-cbt: walk cbt_list under SRCU

This patch converts the q->cbt_list walk in blk_cbt_bio_queue() from
plain RCU to SRCU, so that cbt_page_alloc() no longer drops the read
lock around its sleeping allocation. It also switches the deferred
frees of a cbt from call_rcu() to call_srcu().

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

[ ... ]

> @@ -608,7 +610,7 @@ static void blk_cbt_update_cbt_size(struct cbt_info *cbt, loff_t new_sz)
>  	in_use = cbt->count;
>  	spin_unlock_irq(&cbt->lock);
>  	if (!in_use)
> -		call_rcu(&cbt->rcu, &cbt_release_callback);
> +		call_srcu(&cbt_srcu, &cbt->rcu, &cbt_release_callback);
>  }
>  
>  void blk_cbt_update_size(struct block_device *bdev)

[Severity: Medium]
This problem wasn't introduced by this patch, but the patch touches the
call site involved. Can blk_cbt_update_size() read the ->next pointer of
a cbt that has already been freed?

blk_cbt_update_size() walks the list with a plain iterator, holding only
cbt_mutex:

    mutex_lock(&cbt_mutex);
    list_for_each_entry(cbt, &q->cbt_list, list)
        blk_cbt_update_cbt_size(cbt, bdev_nr_bytes(bdev));

For each cbt, blk_cbt_update_cbt_size() can replace it in the list and
queue the old one for freeing:

    list_replace_rcu(&cbt->list, &new->list);
    in_use = cbt->count;
    ...
    if (!in_use)
        call_srcu(&cbt_srcu, &cbt->rcu, &cbt_release_callback);

When the callee returns, the loop advances through the old cbt's
->list.next. The caller holds no SRCU read lock and no reference on that
cbt. cbt_mutex does not keep an SRCU grace period from completing.

If the task is preempted between call_srcu() and the advance for a full
grace period, cbt_release_callback() can free the cbt before the read.
The window is narrow, but it looks reachable on a preemptible kernel.

Would list_for_each_entry_safe() be appropriate here, since it loads the
next pointer before the callee runs? Alternatively, could the loop be
wrapped in an SRCU read-side section?

[ ... ]

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

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