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

[Tao Cui <cui.tao@linux.dev>]

于 2026年10月3日 GMT+08:00 09:52:07，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

Thank you for catching these issues.
I've noted all the reported UAF and TOCTOU races and am working on fixes.

>--
>
>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;
>> +	}
>