lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <af3b92d434b1d85e9dc3e60c46ed7ed68dde438e.camel@gmail.com>
Date: Sat, 28 Oct 2023 01:02:45 +0300
From: Eduard Zingerman <eddyz87@...il.com>
To: Kui-Feng Lee <sinquersw@...il.com>, thinker.li@...il.com, 
 bpf@...r.kernel.org, ast@...nel.org, martin.lau@...ux.dev, song@...nel.org,
  kernel-team@...a.com, andrii@...nel.org, drosen@...gle.com
Cc: kuifeng@...a.com, netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next v6 07/10] bpf, net: switch to dynamic
 registration

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@...il.com wrote:
> > > > From: Kui-Feng Lee <thinker.li@...il.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);
> > > 


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ