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

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 8358 bytes --]

> diff --git a/block/blk-iocost-bpf.c b/block/blk-iocost-bpf.c
> new file mode 100644
> index 000000000000..2306c5fd6d8d
> --- /dev/null
> +++ b/block/blk-iocost-bpf.c

[ ... ]

> +static int bpf_iocost_reg(void *kdata, struct bpf_link *link)
> +{
> +	struct iocost_model_ops *ops = kdata;
> +
> +	if (!ops->dev)
> +		return -EINVAL;
> +
> +	return ioc_bpf_attach(ops);
> +}

Does this guard against multiple registrations of the same struct_ops?

bpf_struct_ops_link_create() allows several links from one BPF_F_LINK map.
bpf_struct_ops_valid_to_reg() only checks the map type, BPF_F_LINK flag,
and the READY state.  The state stays READY after a link is created, so
every bpf_link_create() on the map calls ->reg(st_map->kvalue.data, link)
on the same kdata.

hid_bpf_reg(), which the commit message names as the model, guards this
case:

    /* prevent multiple attach of the same struct_ops */
    if (ops->hdev)
        return -EINVAL;

While the first link is attached, a second link on the same device is
refused by the ioc->model check in ioc_bpf_attach() with -EBUSY.  That
check stops working once the device goes away:

1. link1 = bpf_link_create(map). ioc_bpf_attach() opens f1 =
   bdev_file_open_by_dev(dev) and stores ops->q = q1 and ops->bdev_file =
   f1.
2. The disk is removed, for example a null_blk device deleted through
   configfs. null_del_dev() calls ida_free(&nullb_indexes, nullb->index)
   and then del_gendisk(). __del_gendisk() -> rq_qos_exit() ->
   ioc_rqos_exit() clears ioc->model and calls ioc_bpf_detach(), which does
   WRITE_ONCE(ops->q, NULL) but deliberately leaves ops->bdev_file = f1.
   The old disk and queue stay alive because f1 pins them. __del_gendisk()
   also calls bdev_unhash(), so the old bdev can no longer be looked up by
   dev_t.
3. A new null_blk device is created. It gets the same index, and so the
   same dev_t, because null_add_dev() sets disk->first_minor = nullb->index
   and the index was already freed.
4. link2 = bpf_link_create(map). bpf_iocost_reg() sees ops->dev != 0.
   ioc_bpf_attach() opens f2 on the new disk and finds no model on the new
   ioc. It then runs:
       rcu_assign_pointer(ioc->model, ops);
       ops->q = q;
       ops->bdev_file = bdevf;
   This overwrites f1, and nothing ever releases f1. The dead gendisk,
   request_queue, bdev and the driver module reference held through the
   open file are leaked, so null_blk can no longer be unloaded.
5. Closing link1 then runs bpf_iocost_unreg() -> ioc_bpf_unreg(). It reads
   q = new queue and bdevf = f2, detaches the model from the new device and
   fputs f2. link2 is still alive but its model has been silently removed.
   When link2 is released, ops->q and ops->bdev_file are both NULL and
   nothing happens.

The map's dev cannot be changed after the first update, because
map_update_elem requires state INIT, so dev_t reuse is the trigger.  Every
step can be reached by a user with CAP_BPF, CAP_PERFMON and the ability to
add and remove block devices.

> diff --git a/block/blk-iocost.c b/block/blk-iocost.c
> index 2745bffcd5ee..21e4f8cbd9f2 100644
> --- a/block/blk-iocost.c
> +++ b/block/blk-iocost.c

[ ... ]

> +#ifdef CONFIG_BLK_CGROUP_IOCOST_BPF
> +/*
> + * Attach a BPF cost model to the device named by ops->dev: resolve the
> + * queue, make sure iocost is on it, and publish the model.  Attaching
> + * switches the device away from the builtin linear model; detaching
> + * restores it.  The struct_ops core holds the program alive for the
> + * whole registered period, so no extra reference is taken on the ops.
> + */
> +int ioc_bpf_attach(struct iocost_model_ops *ops)
> +{
> +	struct block_device *bdev;
> +	struct request_queue *q;
> +	struct ioc *ioc;
> +	const struct iocost_model_ops *old;
> +	struct file *bdevf;
> +	int ret;
> +
> +	bdevf = bdev_file_open_by_dev(new_decode_dev(ops->dev),
> +				      BLK_OPEN_READ, NULL, NULL);
> +	if (IS_ERR(bdevf))
> +		return PTR_ERR(bdevf);
> +	bdev = file_bdev(bdevf);
> +
> +	if (bdev_is_partition(bdev)) {
> +		fput(bdevf);
> +		return -EINVAL;
> +	}
> +
> +	q = bdev->bd_queue;
> +	if (!queue_is_mq(q)) {
> +		fput(bdevf);
> +		return -EOPNOTSUPP;
> +	}
> +
> +	mutex_lock(&q->rq_qos_mutex);
> +	ioc = q_to_ioc(q);
> +	if (!ioc) {
> +		ret = blk_iocost_init(bdev->bd_disk);

Does this check that the disk is still live?

ioc_bpf_attach() takes q->rq_qos_mutex and, if the queue has no ioc, calls
blk_iocost_init() to add one. It never checks that the disk is still live.
The only liveness check is the one bdev_file_open_by_dev() does under
open_mutex at open time, and __del_gendisk() does not wait for existing
openers.

The blkcg config path handles the same race explicitly in
blkg_conf_open_bdev():

    mutex_lock(&bdev->bd_queue->rq_qos_mutex);
    if (!disk_live(bdev->bd_disk)) {
        blkdev_put_no_open(bdev);
        mutex_unlock(&bdev->bd_queue->rq_qos_mutex);
        return -ENODEV;
    }

Without that check, this interleaving is possible:

1. bpf_iocost_reg() -> ioc_bpf_attach(): bdev_file_open_by_dev() succeeds
   while the disk is live.
2. __del_gendisk() on another CPU: bdev_unhash, GD_DEAD, blk_unregister_queue,
   device_del, blk_mq_freeze_queue_wait, then rq_qos_exit(q). rq_qos_exit()
   unlinks every rqos under rq_qos_mutex and runs ioc_rqos_exit(), so
   q->rq_qos no longer has an ioc.
3. ioc_bpf_attach() then gets rq_qos_mutex. q_to_ioc(q) is NULL, so
   blk_iocost_init() runs on the dead disk. rq_qos_add() only checks for a
   duplicate id, so it links a new rqos, and blkcg_activate_policy() succeeds
   because the blkgs are only destroyed later in disk_release(). The model is
   then published and blk_stat accounting is turned on.

Nothing tears down an rqos added after rq_qos_exit(). disk_release() only
calls blkcg_exit_disk(), and blk_free_queue() does not look at q->rq_qos.
The consequences depend on who owns the queue:

- GD_OWNS_QUEUE: the ioc and its pcpu_stat leak.
- Queue not owned by the disk, e.g. SCSI sd, where __del_gendisk()
  unfreezes the queue and it is reused for the next disk on sd rebind: the
  stale ioc stays on q->rq_qos with ioc->rqos.disk pointing at the freed
  old gendisk. The following paths then dereference it:
  - the next rq_qos_exit(), via ioc_rqos_exit() ->
    blkcg_deactivate_policy(rqos->disk, ...)
  - ioc_rqos_queue_depth_changed() -> ioc_refresh_params() ->
    ioc_refresh_params_disk(ioc, force, ioc->rqos.disk)
  - an io.cost.qos write on the new disk, where q_to_ioc() returns the
    stale ioc

[ ... ]

> @@ -3531,11 +3778,31 @@ static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input,
>  				user = false;
>  			else if (!strcmp(buf, "user"))
>  				user = true;
> +#ifdef CONFIG_BLK_CGROUP_IOCOST_BPF
> +			else if (!strcmp(buf, "bpf")) {
> +				/*
> +				 * readback value while a BPF model is
> +				 * attached; attaching is done by loading
> +				 * the struct_ops, not through this file
> +				 */
> +				continue;
> +			}
> +#endif
>  			else
>  				goto unlock;

Is ctrl=bpf meant to be accepted here?

The COST_CTRL case now accepts "ctrl=bpf" as a silent no-op. Before this
patch the write failed with -EINVAL. The comment justifies this as a
"readback value while a BPF model is attached", and the documentation
(da89d4c2165e) says ctrl=bpf is accepted "so a saved configuration still
parses". But the read side never produces ctrl=bpf. ioc_cost_model_prfill()
still prints only

    dname, ioc->user_cost_model ? "user" : "auto",
    model ? "bpf" : "linear",

so only model= can read back as "bpf". The same docs commit also says
"ctrl keeps describing the builtin coefficients" and lists ctrl as "auto"
or "user". So no saved configuration can contain ctrl=bpf, and the stated
reason for accepting it does not hold.

The branch adds a write-only io.cost.model token with no read-side
counterpart and no effect. As user ABI it would have to be kept forever.
It is also accepted when no BPF model is attached, so a mistyped config is
taken silently where it used to be rejected.

The model=bpf branch in COST_MODEL is consistent with the readback and is
not affected.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/35962141400