Re: [Devel] [PATCH hci-8.1 v5 3/6] block/blk-cbt: don't overwrite a map page with CBT_PAGE_MISSED

[no-reply@sashiko.virtuozzo.com]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] blk-cbt: spinlock_t cbt->lock taken in on_each_cpu() IPI callback, invalid wait context
--

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

block/blk-cbt: don't overwrite a map page with CBT_PAGE_MISSED

This patch changes __blk_cbt_set() so that the CBT_PAGE_MISSED marker is stored under cbt->lock. If a map page was installed in the meantime, the bits are set in that page instead. The aim is to stop the cache flush from overwriting, and leaking, a page installed by cbt_page_alloc().

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

> diff --git a/block/blk-cbt.c b/block/blk-cbt.c
> index 1066b6a33b6b..01114362bb2b 100644
> --- a/block/blk-cbt.c
> +++ b/block/blk-cbt.c

[ ... ]

> @@ -194,10 +194,19 @@ static int __blk_cbt_set(struct cbt_info  *cbt, blkcnt_t block,
>  			block += len;
>  			continue;
>  		} else if (pages_missed) {
> +			bool installed;
> +
> +			/* Don't overwrite a page installed meanwhile */
> +			spin_lock_irqsave(&cbt->lock, flags);

[Severity: Medium]
Is it safe to take cbt->lock here? The pages_missed branch of __blk_cbt_set() is reached from the IPI callback:

cbt_flush_cache()
  on_each_cpu(__cbt_flush_cpu_cache, &ctx, 1)
    __cbt_flush_cpu_cache()
      __blk_cbt_set(..., &ctx->pages_missed, ...)

On remote CPUs this runs in hardirq context.

cbt->lock is declared in struct cbt_info as a plain spinlock_t, not a raw_spinlock_t. A spinlock_t is a sleeping lock on PREEMPT_RT. It is also an invalid wait context in hardirq under CONFIG_PROVE_RAW_LOCK_NESTING.

The previous code stored the marker without taking any lock. With this change, kernels built with CONFIG_PROVE_LOCKING and CONFIG_PROVE_RAW_LOCK_NESTING (as in the shipped debug config) could report an "Invalid wait context" splat whenever a flush finds a missing map page. On a PREEMPT_RT build this would be a sleeping lock taken in hardirq context.

Would it be better to make cbt->lock a raw_spinlock_t? Alternatively, the IPI path could install the marker lock-free, for example with cmpxchg(&cbt->map[idx], NULL, CBT_PAGE_MISSED).

> +			installed = CBT_PAGE(cbt, idx);
> +			if (!installed)
> +				cbt->map[idx] = CBT_PAGE_MISSED;
> +			spin_unlock_irqrestore(&cbt->lock, flags);
> +			if (installed)
> +				continue;
>  			(*pages_missed)++;
>  			if (!*idx_first)
>  				*idx_first = idx;
> -			cbt->map[idx] = CBT_PAGE_MISSED;
>  			count -= len;
>  			block += len;
>  			continue;

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

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