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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] bpf: resource leak and broken blkcg notifications on BPF model unbind
- [High] bpf: coefficient-only writes fail if bound model is unregistered
- [Low] bpf: multi-line comments do not follow modern kernel style
--

commit 4e914860298423131f3423ab63f79ed4a655051f
Author: Tao Cui <cuitao@kylinos.cn>

blk-iocost: add BPF struct_ops cost model support

This commit introduces a BPF struct_ops based cost model for blk-iocost,
replacing the builtin linear model on a device. It implements a
registration framework where sysfs bindings map a registered model to a
device.

> diff --git a/block/blk-iocost-bpf.c b/block/blk-iocost-bpf.c
> new file mode 100644
> index 0000000000000..7f1f9a2e51cb2
> --- /dev/null
> +++ b/block/blk-iocost-bpf.c

[ ... ]

> +const struct iocost_model_ops *iocost_bpf_model_get(const char *name)
> +{
> +	struct iocost_bpf_model *m;
> +	const struct iocost_model_ops *ops = ERR_PTR(-ENOENT);
> +
> +	mutex_lock(&iocost_bpf_reg_lock);
> +	list_for_each_entry(m, &iocost_bpf_models, list) {

[Severity: High]
Will this lookup fail to find models that are unregistered but still
actively bound to devices? If a user issues a coefficient-only write on
a device using an unregistered model, ioc_cost_model_write() passes the
name here, but since this only searches the registry (iocost_bpf_models),
it returns -ENOENT and rejects the write.

> +		if (!strcmp(m->ops->name, name)) {

[ ... ]

> +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) {
> +		/* keep the linkage queryable so model_put() can tell a
> +		 * still-registered node (one ref held by the registry)
> +		 * from one kept alive only by device bindings
> +		 */

[Severity: Low]
Does this comment style follow the BPF subsystem requirements? The
standard specifies that multi-line comments should have the opening /*
on its own line.

> +		list_del_init(&m->list);

[ ... ]

> +void iocost_bpf_model_put(const struct iocost_model_ops *ops)
> +{
> +	struct iocost_bpf_model *m;
> +
> +	mutex_lock(&iocost_bpf_reg_lock);
> +	list_for_each_entry(m, &iocost_bpf_lifecycle, lifecycle) {
> +		if (m->ops == ops)
> +			break;
> +	}
> +	if (&m->lifecycle != &iocost_bpf_lifecycle) {
> +		bool freed = refcount_dec_and_test(&m->refs);
> +
> +		/* leave the notify list with the last binding: the
> +		 * remaining ref is either the registry's (node still on
> +		 * iocost_bpf_models) or zero and the node is freed
> +		 */

[Severity: Low]
Similarly here, this multi-line comment is missing the opening /* on its
own line.

> +		if (freed || !list_empty(&m->list))
> +			list_del(&m->lifecycle);
> +		mutex_unlock(&iocost_bpf_reg_lock);
> +		bpf_struct_ops_put(ops);
> +		if (freed)
> +			kfree(m);
> +		return;
> +	}

[Severity: High]
Can this sequence prematurely detach registered models from the lifecycle
list and leak BPF map references?

When the first device is unbound from a registered model,
!list_empty(&m->list) will evaluate to true. This removes the model from
the lifecycle list early, breaking blkcg notifications.

When subsequent devices are unbound, list_for_each_entry() at the start of
iocost_bpf_model_put() will fail to find the model, causing execution to
jump over this block completely. The bpf_struct_ops_put() call will be
skipped, leading to a map reference leak.

> +	mutex_unlock(&iocost_bpf_reg_lock);
> +}

> diff --git a/block/blk-iocost.c b/block/blk-iocost.c
> index 2745bffcd5eef..22b2ece52fa58 100644
> --- a/block/blk-iocost.c
> +++ b/block/blk-iocost.c

[ ... ]

> @@ -3591,6 +3628,24 @@ static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input,
>  	spin_lock_irq(&ioc->lock);
>  	memcpy(u, ioc->params.i_lcoefs, sizeof(u));
>  	user = ioc->user_cost_model;
> +#ifdef CONFIG_BLK_CGROUP_IOCOST_BPF
> +	{
> +		const struct iocost_model_ops *model =
> +			rcu_dereference_protected(ioc->model,
> +					lockdep_is_held(&ioc->lock));
> +
> +		/* seed from the bound model so a coefficient-only write
> +		 * keeps it bound; ctrl=auto/user and model=linear are
> +		 * the explicit ways back to the builtin model
> +		 */

[Severity: Low]
This comment also places text on the same line as the opening /*, which
diverges from the preferred BPF multi-line comment style.

> +		if (model)
> +			strscpy(bpf_model, model->name, sizeof(bpf_model));

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