Re: [RFC PATCH v2 1/5] blk-iocost: add BPF struct_ops cost model support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Unregistered but active BPF models miss blkcg_offline notifications, leading to per-cgroup state leaks.
- [High] BPF cost model is entirely bypassed during request-level sizing (for completed requests), silently falling back to the builtin linear model.
- [Medium] Partial configuration application on the error path when BPF model binding fails breaks transactional semantics.
- [Medium] Non-atomic configuration update in ioc_cost_model_write allows interleaved mixed state (TOCTOU-like).
--

commit 74ed4cc46ea69cb83c7676b643c08876cce22ab0
Author: Tao Cui <cuitao@kylinos.cn>
Date:   Thu Sep 10 12:58:13 2026 +0000

    blk-iocost: add BPF struct_ops cost model support

    Add the iocost_model_ops struct_ops: a bound BPF model fully replaces
    the builtin linear model on a device.  calc_cost() receives the full
    bio->bi_opf (including REQ_PREFLUSH and REQ_FUA), the IO size, the
    start sector (sector_t), the issuing blkcg and the iocost-specific call
    metadata (the merge-path indicator), and is called from both the
    bio charging path and the request-level sizing path, so a model
    owns pricing for every IO on the device.  The builtin cursor is
    not exposed: a model is expected to track its own stream state.

[ ... ]

> diff --git a/block/blk-iocost-bpf.c b/block/blk-iocost-bpf.c
> new file mode 100644
> index 0000000000000..aec6df2795990
> --- /dev/null
> +++ b/block/blk-iocost-bpf.c
[ ... ]
> +void iocost_notify_blkcg_offline(struct blkcg *blkcg)
> +{
> +	struct iocost_bpf_model *m;
> +
> +	guard(mutex)(&iocost_bpf_reg_lock);
> +	list_for_each_entry(m, &iocost_bpf_models, list) {
> +		if (m->ops->blkcg_offline)
> +			m->ops->blkcg_offline(blkcg);
> +	}
> +}
[Severity: High]
Does this skip unregistered but active models?

By only iterating over the global iocost_bpf_models registry, any model that
has been unregistered but is still bound to a device will miss this
blkcg_offline notification.

Could this result in a resource leak of per-cgroup state in the BPF program
if cgroups are subsequently offlined?

[ ... ]
> +static void bpf_iocost_unreg(void *kdata, struct bpf_link *link)
> +{
> +	struct iocost_model_ops *ops = kdata;
> +	struct iocost_bpf_model *m;
> +
> +	mutex_lock(&iocost_bpf_reg_lock);
> +	m = iocost_bpf_model_lookup(ops);
> +	if (m) {
> +		list_del(&m->list);
> +		bpf_struct_ops_put(ops);
> +		kfree(m);
> +	}
> +	mutex_unlock(&iocost_bpf_reg_lock);
> +}
[Severity: High]
Can removing the model from the broadcast list here cut it off from future
lifecycle events?

The model is eagerly removed from the list when unregistered, which means it
will stop receiving blkcg_offline notifications even if devices are still
actively using it.

[ ... ]
> diff --git a/block/blk-iocost.c b/block/blk-iocost.c
> index 2745bffcd5eef..182601ad783f0 100644
> --- a/block/blk-iocost.c
> +++ b/block/blk-iocost.c
[ ... ]
> @@ -2596,10 +2620,28 @@ static void calc_size_vtime_cost_builtin(struct request *rq, struct ioc *ioc,
>  
>  static u64 calc_size_vtime_cost(struct request *rq, struct ioc *ioc)
>  {
> -	u64 cost;
> +#ifdef CONFIG_BLK_CGROUP_IOCOST_BPF
> +	const struct iocost_model_ops *model;
>  
> -	calc_size_vtime_cost_builtin(rq, ioc, &cost);
> -	return cost;
> +	rcu_read_lock();
> +	model = rcu_dereference(ioc->model);
> +	if (model && rq->bio && rq->bio->bi_blkg) {
> +		u64 cost;
> +
> +		cost = model->calc_cost(rq->cmd_flags, blk_rq_bytes(rq),
> +					blk_rq_pos(rq),
> +					rq->bio->bi_blkg->blkcg, 0);
[Severity: High]
Is the BPF cost model bypassed during request-level sizing?

During request completion (such as in ioc_rqos_done()), the request has been
fully completed by blk_update_request(), meaning rq->bio is NULL and
blk_rq_bytes(rq) is 0.

Because of the check for rq->bio here, it seems this will always fall back
to calc_size_vtime_cost_builtin(), completely bypassing the BPF model for
completed requests and breaking the API contract.

[ ... ]
> @@ -3563,6 +3674,14 @@ static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input,
>  
>  	if (user) {
>  		memcpy(ioc->params.i_lcoefs, u, sizeof(u));
>  		ioc->user_cost_model = true;
>  	} else {
>  		ioc->user_cost_model = false;
>  	}
>  	ioc_refresh_params(ioc, true);
[Severity: Medium]
Can this result in partial configuration being applied if binding the BPF
model fails later?

The linear coefficients are irreversibly applied to the device here before
the model validation completes.

>  
>  	ret = 0;
>  
>  unlock:
>  	spin_unlock_irq(&ioc->lock);
>  
> +	/*
> +	 * Bind the BPF model outside ioc->lock: the registry lookup
> +	 * takes the registration mutex and the old model's reference
> +	 * is dropped after the swap.
> +	 */
> +	if (!ret)
> +		ret = ioc_bpf_model_bind(ioc, bpf_model);
[Severity: Medium]
If ioc_bpf_model_bind() fails (e.g., -ENOENT), does it return an error
without reverting the linear coefficient changes? This could leave a torn
configuration where the old model is paired with new linear coefficients.

Also, since the lock is dropped before binding the new BPF model, is there a
race window for concurrent sysfs writes?

If two threads write to io.cost.model concurrently, Thread B's model could be
bound before Thread A's model, pairing Thread A's model with Thread B's
parameters.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910125817.223354-1-cui.tao@linux.dev?part=1