On 1/11/24 11:08, Martin KaFai Lau wrote:
On 1/10/24 5:50 PM, Kui-Feng Lee wrote:
On 1/10/24 15:44, Martin KaFai Lau wrote:
On 1/10/24 2:17 PM, thinker.li@xxxxxxxxx wrote:
The proposed solution here is to add PTR_MAYBE_NULL annotations to
arguments
[ ... ]
== Future Work ==
We require an improved method for annotating arguments. Initially, we
anticipated annotating arguments by appending a suffix to argument
names,
such as arg1__maybe_null. However, this approach does not function for
function pointers due to compiler limitations. Nevertheless, it does
work
for functions. To resolve this, we need compiler support to enable the
inclusion of argument names in the DWARF for function pointer types.
After reading the high level of the patch,
while it needs compiler work to support decl tagging (or arg name) in
a struct_ops's func_proto, changing the info->reg_type of a
struct_ops's argument have been doable in the ".is_valid_access"
without new kernel code change in verifier/btf.c.
btf_ctx_access() mentioned in the original message is a help function
called by the implementation of .is_valid_access. So, just like you
said, they definitely can be handled by .is_valid_access it-self.
Do you prefer to let developers to handle it by themself instead of
handling by the helpers?
I would prefer one way to do the same thing. ".is_valid_access" should
be more flexible and straightforward. e.g. "bpf_tcp_ca_is_valid_access"
can promote all "struct sock" pointers to "struct tcp_sock" without
needing to specify them func by func.
It would be nice to eventually have both compilers support tagging in
the struct_ops's func_proto. I was trying to say ".is_valid_access" can
already add PTR_MAYBE_NULL now while waiting for the compiler support.
Got it! Let's wait for compiler supports.
If the sched_ext adds PTR_MAYBE_NULL in its ".is_valid_access", what
else is missing in the verifier.c and btf.c? I saw the patch has the
following changes in verifier.c. Is it needed?
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 60f08f468399..190735f3eaf5 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -8200,6 +8200,7 @@ static int check_reg_type(struct
bpf_verifier_env *env, u32 regno,
> case PTR_TO_BTF_ID | PTR_TRUSTED:
> case PTR_TO_BTF_ID | MEM_RCU:
> case PTR_TO_BTF_ID | PTR_MAYBE_NULL:
> + case PTR_TO_BTF_ID | PTR_MAYBE_NULL | PTR_TRUSTED:
> case PTR_TO_BTF_ID | PTR_MAYBE_NULL | MEM_RCU:
> {
> /* For bpf_sk_release, it needs to match against first member
This is not necessary. It can happen only if we add new helper functions
that accept trusted and maybe_null. But, we don't add helper functions
any more.
