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

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] [thread-next>] [day] [month] [year] [list]
Date: Fri, 27 Oct 2023 14:32:42 -0700
From: Kui-Feng Lee <sinquersw@...il.com>
To: Eduard Zingerman <eddyz87@...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 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().

>>
>>> +
>>> +    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);
>>