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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] blk-iocost: lock order inversion between queue freeze and rq_qos_mutex
- [Medium] blk-iocost: stale autop timestamps across model attach/detach
--

commit 145ca796c6aa0e9492a94bdce49377cc07f92750
Author: Tao Cui <cuitao@kylinos.cn>

blk-iocost: add BPF struct_ops cost model support

This commit adds BPF struct_ops support (iocost_model_ops) to blk-iocost so
a BPF program can calculate per-bio vtime costs and provide page transfer
coefficients for a target block device. It also extends io.cost.model to
switch between the linear and attached BPF cost models while suspending
ctrl=auto profile stepping when a BPF model is active.

> diff --git a/block/blk-iocost.c b/block/blk-iocost.c
> index a81488c755cab..0f9c695b685af 100644
> --- a/block/blk-iocost.c
> +++ b/block/blk-iocost.c

[ ... ]

> @@ -806,7 +844,7 @@ static int ioc_autop_idx(struct ioc *ioc, struct gendisk *disk)
>  		return AUTOP_SSD_DFL;
>  
>  	/* if user is overriding anything, maintain what was there */
> -	if (ioc->user_qos_params || ioc->user_cost_model)
> +	if (ioc->user_qos_params || ioc->user_cost_model || ioc_model(ioc))
>  		return idx;
>  
>  	/* step up/down based on the vrate */

[ ... ]

> @@ -2908,10 +2968,33 @@ static void ioc_rqos_queue_depth_changed(struct rq_qos *rqos)
>  	spin_unlock_irq(&ioc->lock);
>  }
>  
> +/* Called by .unreg and by device removal, whichever is first. */
> +static void ioc_detach_model(struct ioc *ioc)
> +{
> +#ifdef CONFIG_BLK_CGROUP_IOCOST_BPF
> +	const struct iocost_model_ops *ops = ioc->attached;
> +
> +	lockdep_assert_held(&ioc->rqos.disk->queue->rq_qos_mutex);
> +
> +	if (!ops)
> +		return;
> +
> +	spin_lock_irq(&ioc->lock);
> +	rcu_assign_pointer(ioc->model, NULL);
> +	ioc->attached = NULL;
> +	ioc->attached_link = NULL;
> +	spin_unlock_irq(&ioc->lock);

[Severity: Medium]
When ctrl=auto is active under the builtin linear model and vrate_pct crosses
too_fast_vrate_pct or too_slow_vrate_pct in ioc_autop_idx(), the current
timestamp is recorded in ioc->autop_too_fast_at or ioc->autop_too_slow_at,
requiring AUTOP_CYCLE_NSEC (10 seconds) of sustained condition before stepping
ioc->autop_idx.

When a BPF model is attached in ioc_bpf_reg() and later detached in
ioc_detach_model(), ioc->model is updated without calling
ioc_refresh_params(ioc, true) or clearing ioc->autop_too_fast_at and
ioc->autop_too_slow_at, unlike ioc_cost_model_write() and ioc_qos_write():

block/blk-iocost.c:ioc_refresh_params_disk() {
    ...
	idx = ioc_autop_idx(ioc, disk);
	p = &autop[idx];

	if (idx == ioc->autop_idx && !force)
		return false;
    ...
	ioc->autop_idx = idx;
	ioc->autop_too_fast_at = 0;
	ioc->autop_too_slow_at = 0;
    ...
}

While the BPF model is active, ioc_autop_idx() returns idx early at the
ioc_model(ioc) check before reaching the timestamp update and reset logic, and
ioc_refresh_params_disk(ioc, false, disk) returns false early because
idx == ioc->autop_idx, leaving any pre-attachment timestamp in
ioc->autop_too_fast_at or ioc->autop_too_slow_at intact.

Once the BPF model is detached via ioc_bpf_unreg() -> ioc_detach_model(), on
the first timer tick (ioc_timer_fn() -> ioc_refresh_params(ioc, false) ->
ioc_autop_idx()), if vrate_pct is past the threshold,
now_ns - ioc->autop_too_fast_at (or autop_too_slow_at) includes the entire
time the BPF model was attached.

Could this cause ioc_autop_idx() to immediately exceed AUTOP_CYCLE_NSEC and
step ioc->autop_idx (resetting vtime_rate) on the first tick after detach
instead of waiting for a full AUTOP_CYCLE_NSEC cycle? Should ioc_bpf_reg() and
ioc_detach_model() reset the autop timestamps or call
ioc_refresh_params(ioc, true) under ioc->lock?

[ ... ]

> @@ -3559,7 +3644,11 @@ static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input,
>  			continue;
>  		case COST_MODEL:
>  			match_strlcpy(buf, &args[0], sizeof(buf));
> -			if (strcmp(buf, "linear"))
> +			if (!strcmp(buf, "linear"))
> +				model = NULL;
> +			else if (!strcmp(buf, "bpf") && ioc->attached)
> +				model = ioc->attached;
> +			else
>  				goto unlock;
>  			continue;
>  		}
> @@ -3579,6 +3668,7 @@ static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input,
>  	} else {
>  		ioc->user_cost_model = false;
>  	}
> +	rcu_assign_pointer(ioc->model, model);
>  	ioc_refresh_params(ioc, true);
>  
>  	ret = 0;
> @@ -3626,6 +3716,166 @@ static struct blkcg_policy blkcg_policy_iocost = {
>  	.pd_stat_fn	= ioc_pd_stat,
>  };
>  
> +#ifdef CONFIG_BLK_CGROUP_IOCOST_BPF
> +static int ioc_bpf_reg(void *kdata, struct bpf_link *link)
> +{
> +	struct iocost_model_ops *ops = kdata;
> +	struct block_device *bdev;
> +	struct request_queue *q;
> +	unsigned int memflags;
> +	struct ioc *ioc;
> +	int ret;
> +
> +	bdev = blkdev_get_no_open(new_decode_dev(ops->dev), false);
> +	if (!bdev)
> +		return -ENODEV;
> +
> +	q = bdev_get_queue(bdev);
> +	if (bdev_is_partition(bdev) || !queue_is_mq(q)) {
> +		blkdev_put_no_open(bdev);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	/* the order of ioc_qos_write() */
> +	memflags = blk_mq_freeze_queue(q);
> +	mutex_lock(&q->rq_qos_mutex);

[Severity: High]
In ioc_bpf_reg(), blk_mq_freeze_queue(q) is called before acquiring
q->rq_qos_mutex, which establishes the lock order:

  q->io_lockdep_map -> q->q_lockdep_map -> q->rq_qos_mutex

However, in ioc_cost_model_write() (which is used to toggle between
model=linear and model=bpf), blkg_conf_open_bdev(&ctx) acquires
q->rq_qos_mutex first, and unlike ioc_qos_write(), ioc_cost_model_write() does
not drop q->rq_qos_mutex before calling blk_iocost_init() and
blk_mq_freeze_queue(q):

block/blk-iocost.c:ioc_cost_model_write() {
	blkg_conf_init(&ctx, input);

	ret = blkg_conf_open_bdev(&ctx); // locks q->rq_qos_mutex
	if (ret)
		return ret;
    ...
	ioc = q_to_ioc(q);
	if (!ioc) {
		ret = blk_iocost_init(ctx.bdev->bd_disk);
		if (ret)
			goto close_bdev;
		ioc = q_to_ioc(q);
	}

	memflags = blk_mq_freeze_queue(q); // locks q->io_lockdep_map -> q->q_lockdep_map
    ...
}

This establishes the opposite lock order in ioc_cost_model_write():

  q->rq_qos_mutex -> q->io_lockdep_map -> q->q_lockdep_map

In addition, paths that hold q->rq_qos_mutex without freezing the queue first
(such as ioc_cost_model_write() -> blk_iocost_init() and ioc_weight_write() ->
blkg_conf_prep()) perform GFP_KERNEL allocations under q->rq_qos_mutex without
memalloc_noio_save(), which can enter fs_reclaim and wait on a frozen queue.

Can attaching a BPF model via ioc_bpf_reg() and writing to io.cost.model via
ioc_cost_model_write() trigger a lockdep circular locking dependency splat or
an ABBA deadlock between queue freeze and q->rq_qos_mutex? Should
ioc_cost_model_write() release and re-acquire q->rq_qos_mutex around
blk_mq_freeze_queue(q) the same way ioc_qos_write() does?

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