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@xxxxxxxxx wrote:
> > > > From: Kui-Feng Lee <thinker.li@xxxxxxxxx>
> [...]
> > > > +
> > > > + 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);
> > >
