Re: [PATCH RFC v5 15/18] riscv_cbqri: resctrl: Add mbm_total_bytes bandwidth monitoring

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [Medium] Domain bringup failure and redundant allocations due to bypassed topology validation.
- [Medium] System RMID capacity artificially crippled by unused Bandwidth Controllers.

Pre-existing issues:
- [Critical] Use-after-free (UAF) execution context upon cpuhp read lock release.
- [High] Out-of-bounds RMID allocations lead to -ERANGE read failures and stale monitoring state.
--

commit 1063299534eebd0856c698cf6583a1b91c07b030
Author: Drew Fustini <fustini@kernel.org>

riscv_cbqri: resctrl: Add mbm_total_bytes bandwidth monitoring

Expose CBQRI bandwidth controller's combined read+write counter as
the L3 mbm_total_bytes event. A software accumulator keeps the
64-bit byte total monotonic across the 62-bit hardware counter wrap.

> diff --git a/drivers/resctrl/cbqri_resctrl.c b/drivers/resctrl/cbqri_resctrl.c
> --- a/drivers/resctrl/cbqri_resctrl.c
> +++ b/drivers/resctrl/cbqri_resctrl.c

[ ... ]

> @@ -305,6 +344,82 @@ int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_domain_hdr *hdr,
>  		mutex_unlock(&ctrl->lock);
>  		break;
>  
> +	case QOS_L3_MBM_TOTAL_EVENT_ID: {
> +		struct cbqri_controller *bc;
> +
> +		/*
> +		 * The L3 monitoring domain's id is the L3 cache id. The
> +		 * matching ctrl domain's hw_dom->paired_bc was cached at
> +		 * add time to avoid walking cbqri_controllers on every read.
> +		 */
> +		d = cbqri_find_ctrl_domain(&r->ctrl_domains, hdr->id);
> +		if (!d) {
> +			err = -ENOENT;
> +			break;
> +		}
> +		hw_dom = container_of(d, struct cbqri_resctrl_dom, resctrl_ctrl_dom);
> +		bc = hw_dom->paired_bc;
> +		if (!bc) {
> +			err = -ENOENT;
> +			break;
> +		}
> +		if (WARN_ON_ONCE(!bc->mbm_total_states)) {
> +			err = -EIO;
> +			break;
> +		}
> +		if (rmid >= bc->mcid_count) {
> +			err = -ERANGE;
> +			break;
> +		}

[Severity: High]
This is a pre-existing issue, but will the core allocate out-of-bounds
RMIDs that trigger this -ERANGE error? Looking at
cbqri_resctrl_control_init(), num_rmid is bound to the local capacity
rather than the global minimum (max_rmid). Could this lead to stale
monitoring state and bypass the max_rmid-bounded reset loop?

> +
> +		mutex_lock(&bc->lock);

[ ... ]

> @@ -824,15 +948,54 @@ static int cbqri_resctrl_pick_bw_alloc(void)
>  }
>  
>  /*
> - * Pick one controller per monitoring event.  L3 OCCUP comes from the
> - * picked L3 CC (if mon_capable).
> + * Pick one controller per monitoring event. L3 OCCUP comes from the
> + * picked L3 CC if mon_capable. MBM_TOTAL from the only mon-capable BC,
> + * but only when the system exposes exactly one L3 cache. Pairing a
> + * single BC with multiple L3 domains would overcount system bandwidth
> + * by a factor equal to the L3 domain count.
>   */
>  static void cbqri_resctrl_pick_counters(void)
>  {
>  	struct cbqri_resctrl_res *l3 = &cbqri_resctrl_resources[RDT_RESOURCE_L3];
> +	struct cbqri_controller *ctrl, *prev;
> +	unsigned int l3_count = 0;
>  
>  	if (l3->ctrl && l3->ctrl->mon_capable)
>  		cbqri_resctrl_counters[QOS_L3_OCCUP_EVENT_ID] = l3->ctrl;
> +
> +	/* Count distinct L3 cache_ids */
> +	list_for_each_entry(ctrl, &cbqri_controllers, list) {

[ ... ]

> +		if (!seen)
> +			l3_count++;
> +	}
> +
> +	if (l3_count > 1) {
> +		pr_warn_once("multiple L3 domains (%u) detected. mbm_total_bytes disabled\n",
> +			     l3_count);
> +		return;
> +	}

[Severity: Medium]
When mbm_total_bytes is disabled here, will the unused Bandwidth
Controller's smaller mcid_count still artificially clamp the global
max_rmid in cbqri_resctrl_accumulate_caps()? The unused BC remains
mon_capable, which might restrict valid occupancy monitoring system-wide.

> +
> +	cbqri_resctrl_counters[QOS_L3_MBM_TOTAL_EVENT_ID] =
> +		cbqri_find_only_mon_bc();
>  }

[ ... ]

> @@ -948,6 +1111,26 @@ static int cbqri_attach_cpu_to_l3_mon(struct cbqri_controller *ctrl,
>  	else
>  		list_add_tail(&mon_dom->hdr.list, &res->mon_domains);
>  
> +	/*
> +	 * Pair this L3 domain with the system's mon-capable BC and
> +	 * initialise the BC's per-MCID software accumulators before
> +	 * resctrl_online_mon_domain() exposes the domain to userspace.
> +	 * A concurrent sysfs read of mbm_total_bytes between online and
> +	 * BC init would otherwise pass the !bc->mbm_total_states check
> +	 * with a half-initialised pointer.
> +	 */
> +	hw_dom = container_of(ctrl_dom, struct cbqri_resctrl_dom, resctrl_ctrl_dom);
> +
> +	hw_dom->paired_bc = cbqri_find_only_mon_bc();
> +	if (hw_dom->paired_bc) {
> +		err = cbqri_init_bc_mon_counters(hw_dom->paired_bc);

[Severity: Medium]
If mbm_total_bytes was disabled in cbqri_resctrl_pick_counters() due to
multiple L3 domains, does this unconditionally allocate memory and issue
MMIO resets for an unused BC? Could an initialization failure in this unused
BC cause the entire L3 bringup to abort here?

> +		if (err) {
> +			pr_err("BC @%pa: mon init failed (%d)\n", &hw_dom->paired_bc->addr, err);
> +			hw_dom->paired_bc = NULL;
> +			goto err_listdel;
> +		}
> +	}
> +
>  	err = resctrl_online_mon_domain(res, &mon_dom->hdr);
>  	if (err)
>  		goto err_listdel;
>  
>  	err = cbqri_init_mon_counters(ctrl);
>  	if (err)
>  		goto err_offline;
>  
>  	return 0;
>  
>  err_offline:
>  	/*
>  	 * cancel_delayed_work (not _sync) avoids deadlocking against the
>  	 * cqm_limbo worker which takes cpus_read_lock while this hotplug
>  	 * callback already holds cpus_write_lock. mbm_over is only
>  	 * INIT_DELAYED_WORK'd when MBM_TOTAL was enabled, so gate the
>  	 * cancel on the same condition to avoid touching a zeroed work
>  	 * struct.
>  	 */
>  	cancel_delayed_work(&mon_dom->cqm_limbo);
> +	if (resctrl_is_mon_event_enabled(QOS_L3_MBM_TOTAL_EVENT_ID))
> +		cancel_delayed_work(&mon_dom->mbm_over);
>  	resctrl_offline_mon_domain(res, &mon_dom->hdr);
>  err_listdel:
>  	list_del(&mon_dom->hdr.list);
>  err_free:
>  	kfree(mon_dom);
>  	return err;
>  }

[Severity: Critical]
This is a pre-existing issue, but since this patch introduces a new trigger
instance for mbm_over, does this create a use-after-free window?

Since cancel_delayed_work() is used without _sync, if a worker is currently
stalled waiting on the cpus_read_lock, won't kfree(mon_dom) execute
synchronously here? Once the write lock is dropped, couldn't the pending
worker resume and dereference the freed memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260524-ssqosid-cbqri-rqsc-v7-0-v5-0-78d3a7ba9dbe@kernel.org?part=15