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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a5ffa6da-b58e-4b83-961e-23d730eb5f25@linux.dev>
Date: Mon, 29 Jul 2024 17:20:03 -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, xiyou.wangcong@...il.com,
 yepeilin.cs@...il.com, Stanislav Fomichev <sdf@...ichev.me>
Subject: Re: [RFC PATCH v9 07/11] bpf: net_sched: Allow more optional
 operators in Qdisc_ops

On 7/26/24 3:30 PM, Amery Hung wrote:
>> 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
>> 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.
>>
> I can see how that would work. The ability to add prologue, epilogue
> to struct_ops operators is one thing on my wish list.
> 
> Meanwhile, I am not sure whether that should be written in the kernel
> or rewritten by the verifier. An argument for keeping it in the kernel
> is that the prologue or epilogue can get quite complex and involves
> many kernel structures not exposed to the bpf program (pre-defined ops
> in Qdisc_ops in v8).

Can the v8 pre-defined ops be called as a kfunc? The qdisc_watchdog_cancel/init 
in v9 could be a kfunc and called by pro/epilogue.

For checking and/or resetting skb->dev, it should be simple enough without 
kfunc. e.g. when reusing the skb->rbnode in the future followup effort.

[ Unrelated to qdisc. bpf_tcp_ca can also use help to ensure the cwnd is sane. ]

> 
> Maybe we can keep the current approach in the initial version as they
> are not in the fast path, and then move to (gen_prologue,
> gen_epilogue) once the plumbing is done?

Sure. It can be improved when things are ready.

I am trying some ideas on how to do gen_epilogue and will share when I get some 
tests working.