netdev - Re: [RFC PATCH v9 07/11] bpf: net_sched: Allow more optional operators in Qdisc

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]

Message-ID: <c97e9ab2-dd71-4ce2-8a64-501309b39122@linux.dev>
Date: Fri, 26 Jul 2024 11:30:42 -0700
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Amery Hung <ameryhung@...il.com>
Cc: bpf@...r.kernel.org, netdev@...r.kernel.org, yangpeihao@...u.edu.cn,
 daniel@...earbox.net, andrii@...nel.org, alexei.starovoitov@...il.com,
 martin.lau@...nel.org, sinquersw@...il.com, toke@...hat.com,
 jhs@...atatu.com, jiri@...nulli.us, sdf@...gle.com,
 xiyou.wangcong@...il.com, yepeilin.cs@...il.com
Subject: Re: [RFC PATCH v9 07/11] bpf: net_sched: Allow more optional
 operators in Qdisc_ops

On 7/25/24 6:15 PM, Martin KaFai Lau wrote:
>> diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
>> index 5064b6d2d1ec..9fb9375e2793 100644
>> --- a/net/sched/sch_api.c
>> +++ b/net/sched/sch_api.c
>> @@ -1352,6 +1352,13 @@ static struct Qdisc *qdisc_create(struct net_device *dev,
>>           rcu_assign_pointer(sch->stab, stab);
>>       }
>> +#if defined(CONFIG_BPF_SYSCALL) && defined(CONFIG_BPF_JIT)
>> +    if (sch->flags & TCQ_F_BPF) {
> 
> I can see the reason why this patch is needed. It is a few line changes and they 
> are not in the fast path... still weakly not excited about them but I know it 
> could be a personal preference.
> 
> I think at the very least, instead of adding a new TCQ_F_BPF, let see if the 
> "owner == BPF_MODULE_OWNER" test can be reused like how it is done in the 
> bpf_try_module_get().
> 
> 
> A rough direction I am spinning...
> 
> The pre/post is mainly to initialize and cleanup the "struct bpf_sched_data" 
> before/after calling the bpf prog.
> 
> For the pre (init), there is a ".gen_prologue(...., const struct bpf_prog 
> *prog)" in the "bpf_verifier_ops". Take a look at the tc_cls_act_prologue().
> It calls a BPF_FUNC_skb_pull_data helper. It potentially can call a kfunc 
> bpf_qdisc_watchdog_cancel. However, the gen_prologue is invoked too late in the 

typo. The kfunc should be s/qdisc_watchdog_cancel/qdisc_watchdog_init/ for the pre.

> verifier for kfunc calling now. This will need some thoughts and works.
> 
> For the post (destroy,reset), there is no "gen_epilogue" now. If 
> bpf_qdisc_watchdog_schedule() is not allowed to be called in the ".reset" and 
> ".destroy" bpf prog. I think it can be changed to pre also? There is a ".filter" 
> function in the "struct btf_kfunc_id_set" during the kfunc register.
> 
>> +        err = bpf_qdisc_init_pre_op(sch, tca[TCA_OPTIONS], extack);
>> +        if (err != 0)
>> +            goto err_out4;
>> +    }