I mostly looked from a struct_ops perspective.
> +
> +static const struct bpf_func_proto *
> +bpf_iocost_get_func_proto(enum bpf_func_id func_id,
> + const struct bpf_prog *prog)
> +{
> + switch (func_id) {
> +#ifdef CONFIG_CGROUPS
> + case BPF_FUNC_cgrp_storage_get:
> + return &bpf_cgrp_storage_get_proto;
This seems redundant. BPF_FUNC_cgrp_storage_get should already be part
of bpf_base_func_proto.
> +#endif
> + default:
> + return bpf_base_func_proto(func_id, prog);
> + }
> +}
> +
> +static int bpf_iocost_check_member(const struct btf_type *t,
> + const struct btf_member *member,
> + const struct bpf_prog *prog)
> +{
> + /* calc_cost() is called with RCU read lock held */
> + if (prog->sleepable)
> + return -EINVAL;
> + return 0;
> +}
> +
> +static int bpf_iocost_init_member(const struct btf_type *t,
> + const struct btf_member *member,
> + void *kdata, const void *udata)
> +{
> + struct iocost_model_ops *ops = kdata;
> + const struct iocost_model_ops *uops = udata;
> + u32 moff = __btf_member_bit_offset(t, member) / 8;
> +
> + switch (moff) {
> + case offsetof(struct iocost_model_ops, name):
> + if (bpf_obj_name_cpy(ops->name, uops->name,
> + sizeof(ops->name)) <= 0)
> + return -EINVAL;
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> +static u64 bpf_iocost_calc_cost_stub(u64 opf, u64 nbytes, u64 sector,
> + struct blkcg *blkcg, u64 model_flags);
> +
> +/*
> + * kdata is seeded from the CFI stubs, so calc_cost is never NULL; a
> + * model which did not implement it inherits the stub, which prices
> + * every IO at 0. Compare against the stub to reject it.
> + */
> +static int bpf_iocost_validate(void *kdata)
> +{
> + struct iocost_model_ops *ops = kdata;
> +
> + if (ops->calc_cost == bpf_iocost_calc_cost_stub)
> + return -EINVAL;
This should check !ops->calc_cost. CFI stubs are used to construct
trampolines; they are not copied into kdata for callbacks omitted by
userspace. Also, should the reserved name "linear" be rejected here?
> + return 0;
> +}
> +
> +static int bpf_iocost_reg(void *kdata, struct bpf_link *link)
> +{
> + struct iocost_model_ops *ops = kdata;
> + struct iocost_bpf_model *m;
> + int ret = 0;
> +
> + if (!bpf_struct_ops_get(ops))
> + return -ENOENT;
struct_ops core should hold a map reference when calling .reg().
Calling bpf_struct_ops_get() in reg() and bpf_struct_ops_put() in
unreg() doesn't seem necessary.
> +
> + m = kzalloc(sizeof(*m), GFP_KERNEL);
> + if (!m) {
> + bpf_struct_ops_put(ops);
> + return -ENOMEM;
> + }
> + refcount_set(&m->refs, 1);
> +
> + mutex_lock(&iocost_bpf_reg_lock);
> + {
> + struct iocost_bpf_model *other;
> +
> + list_for_each_entry(other, &iocost_bpf_models, list) {
> + if (!strcmp(other->ops->name, ops->name)) {
> + ret = -EEXIST;
> + break;
> + }
> + }
> + }
> + if (!ret) {
> + m->ops = ops;
> + list_add(&m->list, &iocost_bpf_models);
> + list_add(&m->lifecycle, &iocost_bpf_lifecycle);
This seems to contradict what the changelog says: "blkcg
online/offline notifications follow the model binding, not the name
registry"
> + }
> + mutex_unlock(&iocost_bpf_reg_lock);
> +
> + if (ret) {
> + bpf_struct_ops_put(ops);
> + kfree(m);
> + }
> + return ret;
> +}
> +
> +/*
> + * Unregistering drops the registration reference. When the last
> + * reference is gone (no device bound), the node leaves the lifecycle
> + * list and is freed; otherwise bound devices keep it alive and it
> + * keeps receiving blkcg online/offline notifications.
> + */
> +static void bpf_iocost_unreg(void *kdata, struct bpf_link *link)
> +{
> + struct iocost_model_ops *ops = kdata;
> + struct iocost_bpf_model *m;
> +
> + mutex_lock(&iocost_bpf_reg_lock);
> + m = iocost_bpf_model_lookup(ops);
> + if (m) {
> + list_del(&m->list);
> + if (refcount_dec_and_test(&m->refs)) {
> + list_del(&m->lifecycle);
> + kfree(m);
> + }
> + }
> + mutex_unlock(&iocost_bpf_reg_lock);
> +
> + bpf_struct_ops_put(ops);
Mentioned above. Doesn't seem to be necessary.
[...]
> +static const struct iocost_model_ops *
> +ioc_bpf_model_prepare(const char *name)
> +{
> +#ifdef CONFIG_BLK_CGROUP_IOCOST_BPF
> + const struct iocost_model_ops *new = NULL;
> + int ret;
> +
> + if (!name[0])
> + return NULL;
> + ret = iocost_bpf_model_get(name, &new);
It looks more straightforward if iocost_bpf_model_get() just return
the ops ptr and ERR_PTR on error.
> + return ret ? ERR_PTR(ret) : new;
> +#else
> + return name[0] ? ERR_PTR(-ENOENT) : NULL;
> +#endif
> +}
