Re: [RFC PATCH v3 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] Kernel panic due to NULL pointer dereference in the bio charging path when a BPF struct_ops map is loaded without implementing calc_cost.
- [Medium] The cgroup interface breaks save/restore symmetry because `ctrl=bpf` is printed on read but rejected on write.
- [Medium] Any write to io.cost.model that updates linear coefficients without explicitly specifying model=... silently unbinds the active BPF model.
--

commit 7407199b3c34d6a89c70eca2c0e70034c01c2bd7
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 the IO
controller, allowing the replacement of the built-in linear model. It
provides lifecycle callbacks and integration with the IO charging path
to price IO operations based on the bound BPF model.

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

[ ... ]

> +/*
> + * kdata is seeded from the CFI stubs, so calc_cost is never NULL; a
> + * model which did not implement it inherits the stub, which prices
> + * every IO at 0.  Compare against the stub to reject it.
> + */
> +static int bpf_iocost_validate(void *kdata)
> +{
> +	struct iocost_model_ops *ops = kdata;
> +
> +	if (ops->calc_cost == bpf_iocost_calc_cost_stub)
> +		return -EINVAL;

[Severity: High]
Does this validation catch the case where a user provides an incomplete
BPF map without a calc_cost implementation? The BPF subsystem does not
copy cfi_stubs values into kdata for unimplemented members, so they
remain NULL. Comparing ops->calc_cost to the stub address might fail to
detect a NULL value, allowing an invalid model to be registered.

> +	return 0;
> +}

[ ... ]

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

[ ... ]

> @@ -2571,10 +2577,28 @@ static void calc_vtime_cost_builtin(struct bio *bio, struct ioc_gq *iocg,
>  
>  static u64 calc_vtime_cost(struct bio *bio, struct ioc_gq *iocg, bool is_merge)
>  {
> +#ifdef CONFIG_BLK_CGROUP_IOCOST_BPF
> +	const struct iocost_model_ops *model;
>  	u64 cost;
>  
> -	calc_vtime_cost_builtin(bio, iocg, is_merge, &cost);
> -	return cost;
> +	rcu_read_lock();
> +	model = rcu_dereference(iocg->ioc->model);
> +	if (model) {
> +		cost = model->calc_cost(bio->bi_opf, bio->bi_iter.bi_size,

[Severity: High]
If bpf_iocost_validate() allowed an empty calc_cost through, could this
lead to a NULL pointer dereference here in the bio charging path? A
missing bounds check here combined with the validation hole might cause a
deterministic kernel panic reachable by a privileged user loading an
incomplete BPF map.

> +					bio->bi_iter.bi_sector,
> +					iocg_to_blkg(iocg)->blkcg,
> +					is_merge ? IOCOST_COST_F_MERGE : 0);
> +		rcu_read_unlock();
> +		return min(cost, VTIME_PER_SEC);
> +	}
> +	rcu_read_unlock();
> +#endif

[ ... ]

> @@ -3438,12 +3493,30 @@ static u64 ioc_cost_model_prfill(struct seq_file *sf,
>  		return 0;
>  
>  	spin_lock_irq(&ioc->lock);
> +#ifdef CONFIG_BLK_CGROUP_IOCOST_BPF
> +	{
> +		const struct iocost_model_ops *model =
> +			rcu_dereference_protected(ioc->model,
> +					lockdep_is_held(&ioc->lock));
> +
> +		seq_printf(sf, "%s ctrl=%s model=%s "
> +			   "rbps=%llu rseqiops=%llu rrandiops=%llu "
> +			   "wbps=%llu wseqiops=%llu wrandiops=%llu\n",
> +			   dname, model ? "bpf" :
> +				   ioc->user_cost_model ? "user" : "auto",

[Severity: Medium]
When a BPF model is active, this prints "ctrl=bpf". Does this break the
cgroup interface save/restore symmetry? ioc_cost_model_write() below
seems to reject the "ctrl=bpf" string.

> +			   model ? model->name : "linear",
> +			   u[I_LCOEF_RBPS], u[I_LCOEF_RSEQIOPS],
> +			   u[I_LCOEF_RRANDIOPS], u[I_LCOEF_WBPS],
> +			   u[I_LCOEF_WSEQIOPS], u[I_LCOEF_WRANDIOPS]);
> +	}
> +#else

[ ... ]

> @@ -3512,6 +3632,7 @@ 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;
> +	bpf_model[0] = '\0';

[Severity: Medium]
If a user writes an update to linear coefficients (e.g., rbps=1000)
without explicitly specifying "model=...", bpf_model remains an empty
string because it is not initialized with the currently active model's
name. Could this inadvertently cause the active BPF model to be silently
unbound when the configuration is applied later in this function?

>  
>  	ret = -EINVAL;
>  
> @@ -3533,11 +3654,16 @@ static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input,
>  				user = true;
>  			else
>  				goto unlock;

[Severity: Medium]
It appears that if the input contains "ctrl=bpf" (as output by
ioc_cost_model_prfill), it hits the "else" branch and goes to unlock,
failing the write. Cgroup attributes usually need to parse their own
output to support configuration backup and restore (like systemd or
cgroup-tools). Could this prevent restoring valid saved configurations?

> +			bpf_model[0] = '\0';
>  			continue;
>  		case COST_MODEL:

[ ... ]

> @@ -3550,19 +3676,41 @@ static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input,
>  		user = true;
>  	}
>  
> -	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);
> -
>  	ret = 0;
>  
>  unlock:
>  	spin_unlock_irq(&ioc->lock);
>  
> +	/*
> +	 * Resolve the model name outside ioc->lock (the registry lookup
> +	 * takes a mutex), so a bad name rejects the whole write before
> +	 * anything is applied.  The resolved model is this write's own
> +	 * state, not per-device state, so concurrent writers cannot
> +	 * clobber each other's staged model.  On success, re-take the
> +	 * lock to apply the coefficients and swap the model in one go.
> +	 */
> +	if (!ret) {
> +		const struct iocost_model_ops *new, *old;
> +
> +		new = ioc_bpf_model_prepare(bpf_model);
> +		if (IS_ERR(new)) {
> +			ret = PTR_ERR(new);
> +		} else {
> +			spin_lock_irq(&ioc->lock);
> +			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);
> +			old = ioc_bpf_model_commit(ioc, new);

[Severity: Medium]
If the BPF model name wasn't explicitly provided during this write,
bpf_model is "", making new NULL. ioc_bpf_model_commit() then unbinds the
existing BPF model, falling back to the linear model. Since users have
legitimate reasons to tune linear coefficients while a BPF model is active
(for request sizing), this silent unbinding could drop their QoS policy.

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