Re: [RFC PATCH v9 1/4] 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:
- [Critical] bpf: UAF on ioc in ioc_bpf_detach due to lockless fetch before freeze
- [Critical] bpf: UAF on struct_ops map due to concurrent modification of ops->q
- [High] bpf: TOCTOU race in ioc_bpf_detach reading ops->q leads to UAF on queue
- [High] bpf: ioc_bpf_detach fails to verify ops->link causes cross-link teardown
--

commit d8ef9c16531866711abee0afcce05f21400897b9
Author: Tao Cui <cui.tao@linux.dev>
Subject: blk-iocost: add BPF struct_ops cost model support

This patch adds BPF struct_ops support to blk-iocost, allowing BPF
programs to provide custom IO pricing models. The struct_ops is attached
per-device and replaces the built-in linear model for IO cost
calculations.

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

[ ... ]

> @@ -2888,6 +2959,45 @@ static void ioc_rqos_queue_depth_changed(struct rq_qos *rqos)
>  	spin_unlock_irq(&ioc->lock);
>  }
>  
> +#ifdef CONFIG_BLK_CGROUP_IOCOST_BPF
> +/*
> + * Eject the model on device removal.  The caller holds rq_qos_mutex
> + * and blkcg_deactivate_policy() has already run, so every existing
> + * blkg got iocg_free() from ioc_pd_free() and no walk is needed.
> + *
> + * bdev is read into a local before ops->q is cleared: once q is
> + * NULL, .unreg returns without taking any lock and the map holding
> + * ops can be freed, so ops must not be touched afterwards.
> + */
> +static void ioc_bpf_eject(struct ioc *ioc)
> +{
> +	struct request_queue *q = ioc->rqos.disk->queue;
> +	struct iocost_model_ops *ops;
> +	struct block_device *bdev;
> +
> +	spin_lock_irq(&q->queue_lock);
> +	spin_lock(&ioc->lock);
> +	ops = (struct iocost_model_ops *)rcu_dereference_protected(
> +			ioc->attached, lockdep_is_held(&ioc->lock));
> +	if (!ops) {
> +		spin_unlock(&ioc->lock);
> +		spin_unlock_irq(&q->queue_lock);
> +		return;
> +	}
> +	bdev = ops->bdev;
> +	rcu_assign_pointer(ioc->attached, NULL);
> +	rcu_assign_pointer(ioc->model, NULL);
> +	spin_unlock(&ioc->lock);
> +	WRITE_ONCE(ops->q, NULL);

[Severity: Critical]
Is it possible for this ejection to clear ops->q when the map has been
re-attached to a different queue?

If a user attaches the same struct_ops map to multiple devices, ops->q gets
overwritten. If the first device is then removed, ioc_bpf_eject() clears
ops->q. This can cause subsequent unreg operations for other active links
to fail to detach the map, resulting in a use-after-free when the map is
freed but the surviving queues still use it.

> +	WRITE_ONCE(ops->link, NULL);
> +	spin_unlock_irq(&q->queue_lock);
> +
> +	blkdev_put_no_open(bdev);
> +}

[ ... ]

> +int ioc_bpf_attach(struct iocost_model_ops *ops, struct bpf_link *link)
> +{
> +	struct block_device *bdev;
> +	struct request_queue *q;

[ ... ]

> +	rcu_assign_pointer(ioc->attached, ops);
> +	rcu_assign_pointer(ioc->model, ops);
> +	spin_unlock(&ioc->lock);
> +
> +	ops->bdev = bdev;
> +	WRITE_ONCE(ops->link, link);
> +	WRITE_ONCE(ops->q, q);

[Severity: Critical]
Does this allow concurrent modification of the shared ops state?

ops->q is only protected by the target queue's queue_lock. If the same map
is attached to multiple queues, this will clobber ops->q, setting up the
ejection bug mentioned above.

> +
> +	/* pair iocg_init() with the cgroups which already exist */
> +	ioc_bpf_walk_iocgs(ioc, ops, true);
> +
> +	spin_unlock_irq(&q->queue_lock);
> +	mutex_unlock(&q->blkcg_mutex);

[ ... ]

> +/*
> + * Detach a model: switch back to the builtin model when the attached
> + * model is in use, clear the attachment, and deliver iocg_free() to
> + * the cgroups which still exist, under the same freeze and quiesce as
> + * the attach.  The caller has already checked ops->q.
> + */
> +void ioc_bpf_detach(struct iocost_model_ops *ops, struct bpf_link *link)
> +{
> +	struct request_queue *q = READ_ONCE(ops->q);

[Severity: High]
Does re-reading ops->q here introduce a TOCTOU race?

In bpf_iocost_unreg(), a queue reference is correctly secured using
blk_get_queue_rcu(q). However, the queue is re-read here. If a concurrent
attach overwrites ops->q before this read, the code will operate on a
different, unreferenced queue. This can lead to a use-after-free when
calling blk_mq_freeze_queue() if the new queue is concurrently destroyed.

> +	struct block_device *bdev;
> +	struct ioc *ioc;
> +	unsigned int memflags;
> +
> +	if (!q)
> +		return;
> +	ioc = q_to_ioc(q);

[Severity: Critical]
Can this lockless fetch of ioc lead to a use-after-free?

The ioc pointer is fetched here without securing an independent reference.
When the function blocks on blk_mq_freeze_queue(q) directly below, a
concurrent device hot-unplug (e.g. del_gendisk) can run rq_qos_exit() and
free the ioc struct. 

When this thread resumes, the later spin_lock(&ioc->lock) would dereference
the freed ioc pointer.

> +	if (!ioc)
> +		return;
> +
> +	bdev = ops->bdev;
> +
> +	/*
> +	 * the freeze is taken before rq_qos_mutex like in the attach;
> +	 * the link is re-checked under the mutex below, as the ejection
> +	 * or a newer attachment may have won the race meanwhile
> +	 */
> +	memflags = blk_mq_freeze_queue(q);
> +	blk_mq_quiesce_queue(q);
> +
> +	mutex_lock(&q->rq_qos_mutex);
> +
> +	mutex_lock(&q->blkcg_mutex);
> +	spin_lock_irq(&q->queue_lock);
> +
> +	spin_lock(&ioc->lock);
> +	if (rcu_dereference_protected(ioc->attached,
> +				      lockdep_is_held(&ioc->lock)) != ops) {

[Severity: High]
Is there a missing check to verify the link argument here?

ioc_bpf_detach() is passed a link pointer but never validates it against
READ_ONCE(ops->link). If the same map was attached to another queue via
a new link, closing the old link could cause this function to mistakenly
tear down the new link's attachment, leaving the intended link orphaned.

> +		/* the ejection or a newer link won; nothing to detach */
> +		spin_unlock(&ioc->lock);
> +		spin_unlock_irq(&q->queue_lock);
> +		mutex_unlock(&q->blkcg_mutex);
> +		mutex_unlock(&q->rq_qos_mutex);
> +		blk_mq_unquiesce_queue(q);
> +		blk_mq_unfreeze_queue(q, memflags);
> +		return;
> +	}

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