Re: [RFC v3 3/3] tools/ufq_iosched: add PFQ eBPF I/O scheduler

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] bpf: load failure due to unsupported special BTF fields in PERCPU_ARRAY
- [Critical] bpf: verifier rejection due to direct BTF field writes and PTR_UNTRUSTED usage
- [High] bpf: RB-tree corruption via in-place sorting key modification
- [High] block: permanent I/O stall when waiting for SPECIAL+READ queues
- [High] bpf: lock inversion and PREEMPT_RT violation in insert error path
--

commit b7bd634a597878034765fedd606831b034490328
Author: Kaitao Cheng <chengkaitao@kylinos.cn>

tools/ufq_iosched: add PFQ eBPF I/O scheduler

Add a Priority Fair Queue (PFQ) eBPF I/O scheduler to the UFQ framework.
It supports weighted fair scheduling, request merging, and preferential
service for configured interactive threads.

> diff --git a/tools/ufq_iosched/pfq.bpf.c b/tools/ufq_iosched/pfq.bpf.c
> new file mode 100644
> index 0000000000000..c1aa8650138e9
> --- /dev/null
> +++ b/tools/ufq_iosched/pfq.bpf.c

[ ... ]

> +struct {
> +	__uint(type, BPF_MAP_TYPE_PERCPU_ARRAY);
> +	__uint(max_entries, 1);
> +	__type(key, u32);
> +	__type(value, struct pfq_disk_data);
> +} pfq_disk_scratch SEC(".maps");

[Severity: Critical]
Can the verifier load this program?

The pfq_disk_scratch map is a BPF_MAP_TYPE_PERCPU_ARRAY, but its value type
struct pfq_disk_data contains a bpf_spin_lock, bpf_list_head, and bpf_rb_root.

The verifier natively rejects special BTF fields in per-CPU arrays via
map_check_btf(), returning -EOPNOTSUPP.

[ ... ]

> +static void pfq_disk_scratch_reset(struct pfq_disk_data *d)
> +{
> +	u32 i;
> +
> +	d->lock = (struct bpf_spin_lock){};

[Severity: Critical]
Is it valid to write directly to opaque special BTF fields?

The verifier strictly forbids direct assignments to bpf_spin_lock and
graph nodes.

[ ... ]

> +static void pfq_finish_front_bio_merge(struct pfq_disk_data *dd,
> +				       struct pfq_queue_data *queue,
> +				       struct pfq_rq_core *rn,
> +				       u64 new_sector)
> +{
> +	rn->sector = new_sector;

[Severity: High]
Does this corrupt the fifo_trees RB-tree?

This modifies rn->sector while rn is still linked in the FIFO tree.
The sector field is used as a tie-breaker sorting key in fifo_less().

[ ... ]

> +static __noinline int
> +pfq_insert_core_locked(struct pfq_disk_data *dd,
> +		       struct pfq_queue_data *queue,
> +		       struct pfq_rq_core *core,
> +		       struct request *rq,
> +		       struct pfq_insert_cleanup *cleanup)
> +{
> +	struct pfq_rq_core *list_ref, *list_n, *tree_n, *held = NULL;
> +	struct bpf_rb_root *rq_root = pfq_rq_tree(dd, queue->qid);
> +	struct bpf_rb_root *fifo_root = pfq_fifo_tree(dd, queue->qid);
> +	struct request *old;
> +
> +	list_ref = bpf_refcount_acquire(core);
> +	if (!list_ref) {

[ ... ]

> +	if (!rq_root || bpf_rbtree_add(rq_root, &core->rb_node, rq_less)) {

[Severity: High]
Does this error path introduce a lock inversion and panic on PREEMPT_RT
kernels?

If bpf_rbtree_add() fails, the BPF core synchronously drops the consumed node,
triggering the destructor bpf_request_release() for its inner kptr.

Because dd->lock is held (a bpf_spin_lock mapping to a raw spinlock), and
bpf_request_release() calls __blk_mq_free_request() which acquires a sleepable
spinlock on PREEMPT_RT systems, this violates atomic context rules.

This also applies to pfq_dispatch_push_dn() in pfq_insert_req().

[ ... ]

> +	if (!fifo_root ||
> +	    bpf_rbtree_add(fifo_root, &list_ref->fifo_rb_node, fifo_less)) {

[ ... ]

> +	/*
> +	 * Keep an owning reference for activation rollback or final cleanup.
> +	 * Successful insertion retains both tree references independently.
> +	 */
> +	held = bpf_refcount_acquire(core);

[Severity: Critical]
Can core be acquired here after it was passed to bpf_rbtree_add() earlier
in this function?

The tree insertion consumes the owning reference, making this a use-after-consume
that the verifier will reject.

[ ... ]

> +static void pfq_update_queue_vtime(struct pfq_disk_data *dd,
> +				   struct pfq_queue_data *queue,
> +				   u32 sectors)
> +{
> +	u64 delta;
> +
> +	if (queue->vtime < dd->vtime)
> +		queue->vtime = dd->vtime;
> +	delta = ((u64)sectors << PFQ_VTIME_SHIFT) / queue->weight;
> +	/* Integer truncation must not give small requests free service. */
> +	if (sectors && !delta)
> +		delta = 1;
> +	queue->vtime += delta;

[Severity: High]
Does this corrupt the service_tree RB-tree?

This modifies queue->vtime while queue remains actively linked in the tree,
altering its sorting key in place.

[ ... ]

> +static void pfq_start_idle_hold(struct pfq_disk_data *dd,
> +			       struct pfq_queue_data *queue, u64 now, u32 random,
> +			       struct pfq_stats *stats)
> +{

[ ... ]

> +	WRITE_ONCE(dd->idle_queue_qid, queue->qid);
> +	dd->waiting_idle = true;

[Severity: High]
When an empty SPECIAL read queue is selected, pfq_start_idle_hold() sets
dd->waiting_idle = true, causing the dispatcher to return NULL below.

Does this permanently stall the block layer queue?

Returning NULL instructs the block layer that no requests are available.
Since the scheduler does not explicitly arm a block layer timer or delayed
workqueue to resume dispatching when the idle deadline passes, the system
will never wake up to check the deadline unless another I/O request is
coincidentally inserted.

[ ... ]

> +static int pfq_init_queue_fields(struct pfq_disk_data *dd, u32 idx)
> +{
> +	struct pfq_queue_data *queue = pfq_queue_scalar(dd, idx);
> +
> +	if (!queue)
> +		return -ENOENT;
> +
> +	queue->qid = idx;

[Severity: Critical]
Can we write to queue here?

pfq_queue_scalar() returns an unacquired pointer (PTR_UNTRUSTED) from a map
lookup. The verifier forbids writing to untrusted kptrs.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261003042748.33795-1-kaitao.cheng@linux.dev?part=3