Re: [PATCH bpf-next v6 07/10] bpf, net: switch to dynamic registration

[Eduard Zingerman <eddyz87@gmail.com>]

On Fri, 2023-10-27 at 14:32 -0700, Kui-Feng Lee wrote:
> 
> On 10/26/23 21:39, Kui-Feng Lee wrote:
> > 
> > 
> > On 10/26/23 14:02, Eduard Zingerman wrote:
> > > On Sat, 2023-10-21 at 22:03 -0700, thinker.li@gmail.com wrote:
> > > > From: Kui-Feng Lee <thinker.li@gmail.com>
> [...]
> > > > +
> > > > +    btf = btf_get_module_btf(st_ops->owner);
> > > > +    if (!btf)
> > > > +        return -EINVAL;
> > > > +
> > > > +    log = kzalloc(sizeof(*log), GFP_KERNEL | __GFP_NOWARN);
> > > > +    if (!log) {
> > > > +        err = -ENOMEM;
> > > > +        goto errout;
> > > > +    }
> > > > +
> > > > +    log->level = BPF_LOG_KERNEL;
> > > 
> > > Nit: maybe use bpf_vlog_init() here to avoid breaking encapsulation?
> > 
> > Agree!
> > 
> 
> I don't use bpf_vlog_init() eventually.
> 
> I found bpf_vlog_init() is not for BPF_LOG_KERNEL.
> According to the comment next to BPF_LOG_KERNEL, it
> is an internal log level.
> According to the code of bpf_vlog_init(), the level passing to
> bpf_vlog_init() should be covered by BPF_LOG_MASK. BPF_LOG_KERNEL is
> defined as BPF_LOG_MASK + 1. So, it is intended not being used with
> bpf_vlog_init().

I see, looks like btf_parse_vmlinux does the same, sorry should have checked there.
Thank you for looking into it.

> 
> > > 
> > > > +
> > > > +    desc = btf_add_struct_ops(btf, st_ops);
> > > > +    if (IS_ERR(desc)) {
> > > > +        err = PTR_ERR(desc);
> > > > +        goto errout;
> > > > +    }
> > > > +
> > > > +    bpf_struct_ops_init(desc, btf, log);
> > > 
> > > Nit: I think bpf_struct_ops_init() could be changed to return 'int',
> > >       then register_bpf_struct_ops() could report to calling module if
> > >       something went wrong on the last phase, wdyt?
> > 
> > 
> > Agree!
> > 
> > > 
> > > > +
> > > > +errout:
> > > > +    kfree(log);
> > > > +    btf_put(btf);
> > > > +
> > > > +    return err;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(register_bpf_struct_ops);
> > > > diff --git a/net/bpf/bpf_dummy_struct_ops.c 
> > > > b/net/bpf/bpf_dummy_struct_ops.c
> > > > index ffa224053a6c..148a5851c4fa 100644
> > > > --- a/net/bpf/bpf_dummy_struct_ops.c
> > > > +++ b/net/bpf/bpf_dummy_struct_ops.c
> > > > @@ -7,7 +7,7 @@
> > > >   #include <linux/bpf.h>
> > > >   #include <linux/btf.h>
> > > > -extern struct bpf_struct_ops bpf_bpf_dummy_ops;
> > > > +static struct bpf_struct_ops bpf_bpf_dummy_ops;
> > > >   /* A common type for test_N with return value in bpf_dummy_ops */
> > > >   typedef int (*dummy_ops_test_ret_fn)(struct bpf_dummy_ops_state 
> > > > *state, ...);
> > > > @@ -223,11 +223,13 @@ static int bpf_dummy_reg(void *kdata)
> > > >       return -EOPNOTSUPP;
> > > >   }
> > > > +DEFINE_STRUCT_OPS_VALUE_TYPE(bpf_dummy_ops);
> > > > +
> > > >   static void bpf_dummy_unreg(void *kdata)
> > > >   {
> > > >   }
> > > > -struct bpf_struct_ops bpf_bpf_dummy_ops = {
> > > > +static struct bpf_struct_ops bpf_bpf_dummy_ops = {
> > > >       .verifier_ops = &bpf_dummy_verifier_ops,
> > > >       .init = bpf_dummy_init,
> > > >       .check_member = bpf_dummy_ops_check_member,
> > > > @@ -235,4 +237,12 @@ struct bpf_struct_ops bpf_bpf_dummy_ops = {
> > > >       .reg = bpf_dummy_reg,
> > > >       .unreg = bpf_dummy_unreg,
> > > >       .name = "bpf_dummy_ops",
> > > > +    .owner = THIS_MODULE,
> > > >   };
> > > > +
> > > > +static int __init bpf_dummy_struct_ops_init(void)
> > > > +{
> > > > +    BTF_STRUCT_OPS_TYPE_EMIT(bpf_dummy_ops);
> > > > +    return register_bpf_struct_ops(&bpf_bpf_dummy_ops);
> > > > +}
> > > > +late_initcall(bpf_dummy_struct_ops_init);
> > > > diff --git a/net/ipv4/bpf_tcp_ca.c b/net/ipv4/bpf_tcp_ca.c
> > > > index 3c8b76578a2a..b36a19274e5b 100644
> > > > --- a/net/ipv4/bpf_tcp_ca.c
> > > > +++ b/net/ipv4/bpf_tcp_ca.c
> > > > @@ -12,7 +12,7 @@
> > > >   #include <net/bpf_sk_storage.h>
> > > >   /* "extern" is to avoid sparse warning.  It is only used in 
> > > > bpf_struct_ops.c. */
> > > > -extern struct bpf_struct_ops bpf_tcp_congestion_ops;
> > > > +static struct bpf_struct_ops bpf_tcp_congestion_ops;
> > > >   static u32 unsupported_ops[] = {
> > > >       offsetof(struct tcp_congestion_ops, get_info),
> > > > @@ -277,7 +277,9 @@ static int bpf_tcp_ca_validate(void *kdata)
> > > >       return tcp_validate_congestion_control(kdata);
> > > >   }
> > > > -struct bpf_struct_ops bpf_tcp_congestion_ops = {
> > > > +DEFINE_STRUCT_OPS_VALUE_TYPE(tcp_congestion_ops);
> > > > +
> > > > +static struct bpf_struct_ops bpf_tcp_congestion_ops = {
> > > >       .verifier_ops = &bpf_tcp_ca_verifier_ops,
> > > >       .reg = bpf_tcp_ca_reg,
> > > >       .unreg = bpf_tcp_ca_unreg,
> > > > @@ -287,10 +289,18 @@ struct bpf_struct_ops bpf_tcp_congestion_ops = {
> > > >       .init = bpf_tcp_ca_init,
> > > >       .validate = bpf_tcp_ca_validate,
> > > >       .name = "tcp_congestion_ops",
> > > > +    .owner = THIS_MODULE,
> > > >   };
> > > >   static int __init bpf_tcp_ca_kfunc_init(void)
> > > >   {
> > > > -    return register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS, 
> > > > &bpf_tcp_ca_kfunc_set);
> > > > +    int ret;
> > > > +
> > > > +    BTF_STRUCT_OPS_TYPE_EMIT(tcp_congestion_ops);
> > > > +
> > > > +    ret = register_btf_kfunc_id_set(BPF_PROG_TYPE_STRUCT_OPS, 
> > > > &bpf_tcp_ca_kfunc_set);
> > > > +    ret = ret ?: register_bpf_struct_ops(&bpf_tcp_congestion_ops);
> > > > +
> > > > +    return ret;
> > > >   }
> > > >   late_initcall(bpf_tcp_ca_kfunc_init);
> > >