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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] blk: lock inversion between rq_qos_mutex and queue freeze
- [Low] bpf: multi-line comment style violation in BPF-related files
--

commit 54129eafa6fdb7e951520269403d0945a53f2d20
Author: Tao Cui <cui.tao@linux.dev>

blk-iocost: add BPF struct_ops cost model support

This commit introduces BPF struct_ops support for the block iocost
controller, allowing a BPF program to replace the builtin linear cost
model. It adds attach and detach paths for the BPF model and modifies
the cost model sysfs write path to allow switching between the linear
and BPF models.

> diff --git a/block/blk-iocost-bpf.c b/block/blk-iocost-bpf.c
> new file mode 100644
> --- /dev/null
> +++ b/block/blk-iocost-bpf.c
[ ... ]
> @@ -108,13 +108,13 @@ static void bpf_iocost_unreg(void *kdata, struct bpf_link *link)
>  	struct iocost_model_ops *ops = kdata;
>  	struct request_queue *q;
>  
>  	/*
>  	 * ops->q may already have been cleared by the removal ejection;
>  	 * take a queue reference under RCU before entering the queue,
>  	 * as the queue may be dying and its memory is only guaranteed
>  	 * under rcu_read_lock()
>  	 */
>  	rcu_read_lock();
>  	/* pairs with the smp_store_release() in the attach: a non-NULL
>  	 * q guarantees the owning link store is visible */

[Severity: Low]
This isn't a bug, but does this multi-line comment violate the BPF subsystem
comment style? The opening /* is expected to be on its own line for
multi-line comments in BPF-related files.

>  	q = smp_load_acquire(&ops->q);
[ ... ]
> diff --git a/block/blk-iocost.c b/block/blk-iocost.c
> --- a/block/blk-iocost.c
> +++ b/block/blk-iocost.c
[ ... ]
> @@ -3621,7 +3621,265 @@ static int ioc_cost_model_show(struct seq_file *sf, void *v)
[ ... ]
> +int ioc_bpf_attach(struct iocost_model_ops *ops, struct bpf_link *link)
> +{
[ ... ]
> +	if (!queue_is_mq(q)) {
> +		ret = -EOPNOTSUPP;
> +		goto put;
> +	}
> +
> +	/*
> +	 * the freeze is taken before rq_qos_mutex; taking the mutex
> +	 * around the freeze instead cycles against the io.cost.qos
> +	 * write path, which freezes with the mutex held.  Holding
> +	 * rq_qos_mutex from the disk_live() check through the model
> +	 * switch keeps the ioc from being freed under us.
> +	 */
> +	memflags = blk_mq_freeze_queue(q);
> +
> +	mutex_lock(&q->rq_qos_mutex);

[Severity: High]
Could this locking sequence cause an ABBA deadlock? Here we freeze the
queue and then acquire rq_qos_mutex. However, in ioc_cost_model_write()
introduced further below, the order appears to be reversed.

> +
> +	/*
> +	 * one map binds one device at a time: the ops carries a single
[ ... ]
> +void ioc_bpf_detach(struct iocost_model_ops *ops, struct bpf_link *link,
> +		   struct request_queue *q)
> +{
[ ... ]
> +	/*
> +	 * the freeze is taken before rq_qos_mutex like in the attach;
> +	 * the attachment and the link are re-checked under the mutex
> +	 * below, as the ejection or a newer attachment through another
> +	 * link may have won the race meanwhile
> +	 */
> +	memflags = blk_mq_freeze_queue(q);
> +	blk_mq_quiesce_queue(q);
> +
> +	mutex_lock(&q->rq_qos_mutex);

[Severity: High]
Similar to ioc_bpf_attach(), this establishes the queue freeze ->
rq_qos_mutex lock order.

> +
> +	ioc = q_to_ioc(q);
[ ... ]
> @@ -3484,6 +3907,8 @@ static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input,
>  	bool user;
>  	char *body, *p;
>  	int ret;
> +	const struct iocost_model_ops *new_model = NULL;
> +	bool model_write = false;
>  
>  	blkg_conf_init(&ctx, input);
>  

[Severity: High]
Does this code establish a reverse locking order compared to the attach and
detach paths?

Further down in ioc_cost_model_write(), the call to blkg_conf_open_bdev()
acquires rq_qos_mutex, and then blk_mq_freeze_queue() is called. This
rq_qos_mutex -> queue freeze order looks like it will trigger a circular
locking dependency warning in lockdep if someone loads a BPF struct_ops
and writes to io.cost.model.

> @@ -3536,9 +3961,25 @@ static ssize_t ioc_cost_model_write(struct kernfs_open_file *of, char *input,
>  			continue;
>  		case COST_MODEL:
>  			match_strlcpy(buf, &args[0], sizeof(buf));
> -			if (strcmp(buf, "linear"))
> -				goto unlock;
> -			continue;
> +			if (!strcmp(buf, "linear")) {
> +				/* staged and committed below, so a parse
> +				 * error later in the same write leaves
> +				 * the model selection untouched
> +				 */

[Severity: Low]
This isn't a bug, but this multi-line comment also appears to miss having
the opening /* on its own line, which goes against the BPF subsystem
formatting guidelines.

> +				new_model = NULL;
> +				model_write = true;
> +				continue;
> +			}

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