Re: [RFC v3 1/3] block: Introduce the UFQ I/O scheduler

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [Critical] block: ufq_bio_merge() destroys active target request leading to UAF
- [Critical] block: missing state validation in UFQ merges allows corruption of in-flight requests
- [Critical] bpf: unbounded stack buffer overflow in UFQ struct_ops via PTR_TO_BUF
- [High] bpf: bpf_request_try_merge() permanently leaks absorbed requests
- [High] block: ufq_insert_requests() re-inserts requests that absorbed a back-merge
- [High] bpf: incompatible reference contract in dispatch_req leads to UAF
- [High] block: TOCTOU Use-After-Free in UFQ merge and dispatch paths
- [High] bpf: bpf_request_bio_try_merge() allows double-merging of bios causing list corruption
- [High] bpf: bpf_request_try_merge() crashes on already-merged requests
- [Medium] bpf: spin_trylock() inside KF_SPINLOCK_SAFE causes Invalid wait context on PREEMPT_RT
- [Low] block: modpost failure due to missing EXPORT_SYMBOL_GPL for core functions
--

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

block: Introduce the UFQ I/O scheduler

Introduce IOSCHED_UFQ, a blk-mq elevator ("ufq: User-programmable
Flexible Queueing") whose policy is supplied by an eBPF program via
struct_ops (insert, dispatch, merge, finish, etc.).

> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index e929f94b25c7f..aa8ae5bd4b6fd 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -809,7 +809,7 @@ static void blk_mq_finish_request(struct request *rq)
>  	}
>  }
>  
> -static void __blk_mq_free_request(struct request *rq)
> +void __blk_mq_free_request(struct request *rq)
>  {

[Severity: Low]
Will this cause a modpost failure?

CONFIG_IOSCHED_UFQ can be compiled as a module, and it calls
__blk_mq_free_request(), bpf_attempt_merge(), and blk_attempt_bio_merge().
Since these core block layer functions are not exported via EXPORT_SYMBOL_GPL,
linking the module will fail during modpost.

> diff --git a/block/ufq-bpfops.c b/block/ufq-bpfops.c
> new file mode 100644
> index 0000000000000..5efcfcaa9cb8c
> --- /dev/null
> +++ b/block/ufq-bpfops.c
> [ ... ]
> +static bool bpf_ufq_is_valid_access(int off, int size,
> +				    enum bpf_access_type type,
> +				    const struct bpf_prog *prog,
> +				    struct bpf_insn_access_aux *info)
> +{
> [ ... ]
> +	if (size == sizeof(__u64) && prog->aux->attach_func_name &&
> +	    ((!strcmp(prog->aux->attach_func_name, "merge_req") && off == 16) ||
> +	     (!strcmp(prog->aux->attach_func_name, "merge_bio") && off == 24))) {
> +		if (!btf_ctx_access(off, size, type, prog, info))
> +			return false;
> +		info->reg_type = PTR_TO_BUF;
> +		return true;
> +	}

[Severity: Critical]
Does this allow an unbounded stack buffer overflow?

The verifier is told to treat the pointer as a variable-sized buffer
(PTR_TO_BUF) to allow pointer writes, but the subsystem fails to validate the
size against the actual variable size in the check_member callback. A
malicious BPF program can write out of bounds into the stack variables passed
to merge_req and merge_bio.

> diff --git a/block/ufq-iosched.c b/block/ufq-iosched.c
> new file mode 100644
> index 0000000000000..f7964ccfc1ee3
> --- /dev/null
> +++ b/block/ufq-iosched.c
> [ ... ]
> +static struct request *ufq_dispatch_request(struct blk_mq_hw_ctx *hctx)
> +{
> [ ... ]
> +		/*
> +		 * The BPF insert_req callback bumps the request's reference
> +		 * count; dispatch_req returns that same request with an extra
> +		 * reference held. The kernel must put that reference here,
> +		 * and the request's refcount is always greater than zero at
> +		 * this point.
> +		 */
> +		if (WARN_ON_ONCE(req_ref_put_and_test(rq))) {
> +			__blk_mq_free_request(rq);

[Severity: High]
Is the reference contract here incompatible with the BPF verifier?

ufq_dispatch_request() expects BPF to transfer an acquired reference.
However, BPF struct_ops do not support returning acquired references. To
bypass the verifier, a BPF program must return an unacquired pointer,
which causes the kernel to decrement the core reference count instead of
an extra one, freeing an active request prematurely.

> +			ufq_bpfops_put();
> +			return NULL;
> +		}
> +
> +		ctx = rq->mq_ctx;
> +		spin_lock(&ctx->lock);

[Severity: High]
Does this expose a Time-of-Check to Time-of-Use race?

The BPF-owned reference is dropped via req_ref_put_and_test() before
dereferencing rq->mq_ctx and acquiring the lock. If the request completes
concurrently during this window, the reference count drops to zero and it is
freed, resulting in a Use-After-Free.

> [ ... ]
> +static bool ufq_bio_merge(struct request_queue *q, struct bio *bio,
> +			  unsigned int nr_segs)
> +{
> [ ... ]
> +			if (rq) {
> +				spin_lock(&rq->mq_ctx->lock);
> +				if (!list_empty(&rq->queuelist)) {
> +					list_del_init(&rq->queuelist);
> +					atomic_dec(&ufq->rqs_count);
> +				}
> +				spin_unlock(&rq->mq_ctx->lock);
> +				/* merge_bio transfers an extra BPF-owned reference. */
> +				WARN_ON_ONCE(req_ref_put_and_test(rq));
> +				blk_mq_free_request(rq);

[Severity: Critical]
Does this cause a Use-After-Free by destroying the active target request?

If the BPF policy's merge_bio hook successfully merges a bio into a request,
it returns the target rq. The kernel then erroneously calls
blk_mq_free_request(rq), actively destroying the target request that is
still waiting for dispatch instead of just dropping the BPF reference.

> [ ... ]
> +static enum elv_merge ufq_try_insert_merge(struct request_queue *q,
> +					   struct request **new)
> +{
> [ ... ]
> +	ops = ufq_bpfops_tryget();
> +	if (ops && ops->merge_req) {
> +		target = ops->merge_req(q, rq, &merge_type);

[Severity: Critical]
Can this overflow the merge_type stack variable?

This points to the stack variable merge_type which is passed to the BPF
program via PTR_TO_BUF without proper bounds checking, leading to local
privilege escalation.

> +		type = (enum elv_merge)merge_type;
> +	}
> +
> +	if (target && WARN_ON_ONCE(req_ref_put_and_test(target))) {
> +		__blk_mq_free_request(target);

[Severity: High]
Does this introduce a Time-of-Check to Time-of-Use race?

The BPF-owned reference on the target request is dropped via
req_ref_put_and_test() before taking target->mq_ctx->lock and validating
the request. If the request completes concurrently during this window, the
reference count drops to zero and it is freed, resulting in a Use-After-Free
when dereferencing target->mq_ctx later in this function.

> +		ufq_bpfops_put();
> +		return ELEVATOR_NO_MERGE;
> +	}
> +
> +	if (type == ELEVATOR_NO_MERGE || !target) {
> +		if (ops)
> +			ufq_bpfops_put();
> +		return ELEVATOR_NO_MERGE;
> +	} else if (type == ELEVATOR_FRONT_MERGE) {
> +		if (rq->mq_ctx != target->mq_ctx || rq->mq_hctx != target->mq_hctx)
> +			goto rollback;
> +		spin_lock(&target->mq_ctx->lock);

[Severity: Critical]
Is there a missing state validation here?

The BPF merge_req callback blindly returns an arbitrary target request. There
is no verification that the target request belongs to the correct queue, nor
that its state is MQ_RQ_IDLE. Modifying an in-flight request concurrently
with active DMA transfers corrupts data.

> [ ... ]
> +static void ufq_insert_requests(struct blk_mq_hw_ctx *hctx,
> +			       struct list_head *list,
> +			       blk_insert_t flags)
> +{
> [ ... ]
> +		if (ops && rq && ops->insert_req) {
> +			rq->elv.priv[0] = (void *)((uintptr_t)rq->elv.priv[0]
> +				  | UFQ_PRIV_IN_BPF);
> +			ret = ops->insert_req(q, rq, flags);

[Severity: High]
Does this cause double-insertion for requests that absorbed a back-merge?

If ufq_try_insert_merge() results in an ELEVATOR_BACK_MERGE, rq is updated
to point to the target request. This function then unconditionally calls
ops->insert_req() on this pre-existing target request, corrupting the BPF
scheduler's internal tracking.

> diff --git a/block/ufq-kfunc.c b/block/ufq-kfunc.c
> new file mode 100644
> index 0000000000000..843aeb0e67cff
> --- /dev/null
> +++ b/block/ufq-kfunc.c
> @@ -0,0 +1,169 @@
> [ ... ]
> +__bpf_kfunc bool bpf_request_bio_try_merge(struct request *rq, struct bio *bio,
> +					   unsigned int nr_segs)
> +{
> [ ... ]
> +	/*
> +	 * KF_SPINLOCK_SAFE callers (BPF merge_bio) often already hold a scheduler
> +	 * lock taken with irqsave (e.g. PFQ dd->lock).  Blocking on ctx->lock
> +	 * nesting that order deadlocks / hard-locks under load.  Trylock: skip
> +	 * the merge if ctx is busy; bio will be issued as a new request.
> +	 */
> +	if (!spin_trylock(&ctx->lock))
> +		goto out;

[Severity: Medium]
Will this cause an Invalid wait context warning on PREEMPT_RT?

This kfunc is annotated with KF_SPINLOCK_SAFE, meaning it can execute with
interrupts disabled or a raw_spinlock_t held. Calling spin_trylock() inside
this context violates strict lock nesting rules, as it maps to an rt_mutex
operation on PREEMPT_RT.

> +
> +	merged = blk_attempt_bio_merge(q, rq, bio, nr_segs, true) == BIO_MERGE_OK;

[Severity: Critical]
Is there missing state validation for the target request here?

The kfunc modifies the target request's internal structures without verifying
that its state is MQ_RQ_IDLE, which can corrupt data if the request is already
in-flight.

[Severity: High]
Can a BPF program trigger double-merging of bios here?

Because sector proximity checks will still pass and there is no safeguard
against already-merged bios, calling this kfunc multiple times with the
same bio will link it into the biotail of multiple requests, corrupting
the bi_next pointers.

> [ ... ]
> +__bpf_kfunc struct request *bpf_request_try_merge(struct request *rq, struct request *next)
> +{
> [ ... ]
> +	free = bpf_attempt_merge(q, rq, next);

[Severity: High]
Can this code crash by dereferencing a NULL pointer?

If bpf_request_try_merge() successfully absorbs a request, the block layer
sets next->bio = NULL. If the BPF program erroneously calls this kfunc
again on the already-absorbed request, attempt_merge() will
unconditionally dereference next->bio->bi_write_hint.

> [ ... ]
> +BTF_KFUNCS_START(ufq_kfunc_set_ops)
> +BTF_ID_FLAGS(func, bpf_request_acquire, KF_ACQUIRE | KF_RET_NULL)
> +BTF_ID_FLAGS(func, bpf_request_release, KF_RELEASE)
> +BTF_ID_FLAGS(func, bpf_request_bio_try_merge, KF_SPINLOCK_SAFE)
> +BTF_ID_FLAGS(func, bpf_request_try_merge, KF_SPINLOCK_SAFE)

[Severity: High]
Does this code permanently leak the absorbed request?

bpf_request_try_merge() returns the redundant absorbed request, but without
the KF_ACQUIRE flag, the BPF verifier treats it as an unreferenced pointer.
The BPF program cannot legally free it via bpf_request_release(), and the
kernel omits freeing it, resulting in a permanent leak and eventual
exhaustion of block tags.

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