diff mbox series

[RFC,bpf-next] bpf, selftests/bpf: Support PTR_MAYBE_NULL for struct_ops arguments.

Message ID 20240110221750.798813-1-thinker.li@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series [RFC,bpf-next] bpf, selftests/bpf: Support PTR_MAYBE_NULL for struct_ops arguments. | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/apply success Patch already applied to bpf-next
bpf/vmtest-bpf-next-PR fail merge-conflict
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-13 pending Logs for s390x-gcc / test (test_maps, false, 360) / test_maps on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18 and -O2 optimization
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-42 success Logs for x86_64-llvm-18 / veristat

Commit Message

Kui-Feng Lee Jan. 10, 2024, 10:17 p.m. UTC 
From: Kui-Feng Lee <thinker.li@gmail.com>

Allow passing a null pointer to the operators provided by a struct_ops
object. This is an RFC to collect feedbacks/opinions.

The function pointers that are passed to struct_ops operators (the function
pointers) are always considered reliable until now. They cannot be
null. However, in certain scenarios, it should be possible to pass null
pointers to these operators. For instance, sched_ext may pass a null
pointer in the struct task type to an operator that is provided by its
struct_ops objects.

The proposed solution here is to add PTR_MAYBE_NULL annotations to
arguments and create instances of struct bpf_ctx_arg_aux (arg_info) for
these arguments. These arg_infos will be installed at
prog->aux->ctx_arg_info and will be checked by the BPF verifier when
loading the programs. When a struct_ops program accesses arguments in the
ctx, the verifier will call btf_ctx_access() (through
bpf_verifier_ops->is_valid_access) to verify the access. btf_ctx_access()
will check arg_info and use the information of the matched arg_info to
properly set reg_type.

For nullable arguments, it sets an arg_info to annotate them with
PTR_TO_BTF_ID | PTR_TRUSTED | PTR_MAYBE_NULL. This enforces the verifier to
check programs and ensure that they properly check the pointer. The
programs should check if the pointer is null before reading/writing the
pointed memory.

The implementer of a struct_ops should annotate the arguments that is
PTR_MAYBE_NULL. Here is the example in bpf_testmod.c.

  struct bpf_struct_ops_arg_info testmod_arg_info[] = {
          ST_OPS_ARG_MAYBE_NULL(struct bpf_testmod_ops, test_maybe_null, 1),
  };

This means that the argument 1 (2nd) of bpf_testmod_ops->test_maybe_null,
which is a function pointer, should be PTR_MAYBE_NULL. With this
annotation, the verifier will understand how to check programs using this
arguments.

The struct bpf_struct_ops_arg_info above should be set in the
struct bpf_struct_ops. For example,

  struct bpf_struct_ops bpf_bpf_testmod_ops = {
            ……
            .arg_info = testmod_arg_info,
            .arg_info_cnt = ARRAY_SIZE(testmod_arg_info),
  };

== 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.
---
 include/linux/bpf.h                           |  20 ++++
 kernel/bpf/btf.c                              | 106 +++++++++++++++++-
 kernel/bpf/verifier.c                         |  14 ++-
 .../selftests/bpf/bpf_testmod/bpf_testmod.c   |  25 ++++-
 .../selftests/bpf/bpf_testmod/bpf_testmod.h   |   4 +
 .../prog_tests/test_struct_ops_maybe_null.c   |  39 +++++++
 .../bpf/progs/struct_ops_maybe_null.c         |  27 +++++
 .../bpf/progs/struct_ops_maybe_null_fail.c    |  25 +++++
 8 files changed, 255 insertions(+), 5 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/prog_tests/test_struct_ops_maybe_null.c
 create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_maybe_null.c
 create mode 100644 tools/testing/selftests/bpf/progs/struct_ops_maybe_null_fail.c

Comments

Martin KaFai Lau Jan. 10, 2024, 11:44 p.m. UTC | #1 
On 1/10/24 2:17 PM, thinker.li@gmail.com 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.

Take a look at the bpf_tcp_ca_is_valid_access() which promotes the info->btf_id 
to "struct tcp_sock". The same could be done for info->reg_type (e.g. adding 
PTR_MAYBE_NULL).
Kui-Feng Lee Jan. 11, 2024, 1:50 a.m. UTC | #2 
On 1/10/24 15:44, Martin KaFai Lau wrote:
> On 1/10/24 2:17 PM, thinker.li@gmail.com 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?

> 
> Take a look at the bpf_tcp_ca_is_valid_access() which promotes the 
> info->btf_id to "struct tcp_sock". The same could be done for 
> info->reg_type (e.g. adding PTR_MAYBE_NULL).
Martin KaFai Lau Jan. 11, 2024, 7:08 p.m. UTC | #3 
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@gmail.com 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.

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
Yonghong Song Jan. 12, 2024, 12:30 a.m. UTC | #4 
On 1/11/24 11:08 AM, 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@gmail.com 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.

Considering gcc side does not support decl tag yet, after discussing with Alexei, we think we
should delay to implement this func proto argument name/decl_tag in dwarf thing since we
need to hack with gcc compiler. Adding PTR_MAYBE_NULL in the kernel should be good enough.
Note that similarly, a lot of bpf_iter already adopts the same approach.

For example, map_iter.c, we have
   static const struct bpf_iter_reg bpf_map_elem_reg_info = {
         .target                 = "bpf_map_elem",
         .attach_target          = bpf_iter_attach_map,
         .detach_target          = bpf_iter_detach_map,
         .show_fdinfo            = bpf_iter_map_show_fdinfo,
         .fill_link_info         = bpf_iter_map_fill_link_info,
         .ctx_arg_info_size      = 2,
         .ctx_arg_info           = {
                 { offsetof(struct bpf_iter__bpf_map_elem, key),
                   PTR_TO_BUF | PTR_MAYBE_NULL | MEM_RDONLY },
                 { offsetof(struct bpf_iter__bpf_map_elem, value),
                   PTR_TO_BUF | PTR_MAYBE_NULL },
         },
   };

Or cgroup_iter.c
   static struct bpf_iter_reg bpf_cgroup_reg_info = {
         .target                 = "cgroup",
         .feature                = BPF_ITER_RESCHED,
         .attach_target          = bpf_iter_attach_cgroup,
         .detach_target          = bpf_iter_detach_cgroup,
         .show_fdinfo            = bpf_iter_cgroup_show_fdinfo,
         .fill_link_info         = bpf_iter_cgroup_fill_link_info,
         .ctx_arg_info_size      = 1,
         .ctx_arg_info           = {
                 { offsetof(struct bpf_iter__cgroup, cgroup),
                   PTR_TO_BTF_ID_OR_NULL | PTR_TRUSTED },
         },
         .seq_info               = &cgroup_iter_seq_info,
   };

Eventually types in the above ctx_arg_info are assigned to
reg type and used by the verifier.

Martin proposed change in is_valid_access() callback can also
get modified register type. It should work well.

Although lacking of kernel maybe_null decl, we could still
verify based on program BTF. Martin told me that
current struct_ops func parameter validation does not
check prog BTF. But if we truely want to verify,
we can check program BTF as well to check whether
the corresponding decl tag exists or not.
For example, in tools/lib/bpf/bpf_helpers.h, we have
#define __arg_nonnull __attribute((btf_decl_tag("arg:nonnull")))

we can introduce
#define __arg_maybe_null __attribute((btf_decl_tag("arg:maybe_null")))

and the kernel can check bpf program argument decl tag
to ensure its has __arg_maybe_null to be consistent
with kernel PTR_MAYBE_NULL marking.

>
> 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
>
Kui-Feng Lee Jan. 12, 2024, 12:35 a.m. UTC | #5 
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@gmail.com 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.
diff mbox series

Patch

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index fd49a9a5ae5c..6cbd5d5ef415 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -1413,6 +1413,7 @@  struct bpf_ctx_arg_aux {
 	u32 offset;
 	enum bpf_reg_type reg_type;
 	u32 btf_id;
+	struct btf *btf;
 };
 
 struct btf_mod_pair {
@@ -1609,6 +1610,12 @@  struct bpf_link_primer {
 	u32 id;
 };
 
+struct bpf_struct_ops_arg_info {
+	u32 op_off;
+	u32 arg_no;
+	enum bpf_reg_type reg_type;
+};
+
 struct bpf_struct_ops_value;
 struct btf_member;
 
@@ -1677,6 +1684,9 @@  struct bpf_struct_ops {
 	struct module *owner;
 	const char *name;
 	struct btf_func_model func_models[BPF_STRUCT_OPS_MAX_NR_MEMBERS];
+
+	u32 arg_info_cnt;
+	struct bpf_struct_ops_arg_info *arg_info;
 };
 
 struct bpf_struct_ops_desc {
@@ -1686,6 +1696,8 @@  struct bpf_struct_ops_desc {
 	const struct btf_type *value_type;
 	u32 type_id;
 	u32 value_id;
+
+	struct bpf_ctx_arg_aux *ctx_arg_info;
 };
 
 enum bpf_struct_ops_state {
@@ -3302,4 +3314,12 @@  static inline bool bpf_is_subprog(const struct bpf_prog *prog)
 	return prog->aux->func_idx != 0;
 }
 
+#define ST_OPS_ARG_FLAGS(type, fptr, argno, flags)			\
+	{ .op_off = offsetof(type, fptr) * 8, .arg_no = argno, .reg_type = flags, }
+
+#define ST_OPS_ARG_MAYBE_NULL(type, fptr, argno)			\
+	ST_OPS_ARG_FLAGS(type, fptr, argno,				\
+			 PTR_TO_BTF_ID | PTR_TRUSTED | PTR_MAYBE_NULL)
+
+
 #endif /* _LINUX_BPF_H */
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 81db591b4a22..dae8c3ad35be 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -1699,6 +1699,11 @@  static void btf_free_struct_meta_tab(struct btf *btf)
 static void btf_free_struct_ops_tab(struct btf *btf)
 {
 	struct btf_struct_ops_tab *tab = btf->struct_ops_tab;
+	int i;
+
+	if (tab)
+		for (i = 0; i < tab->cnt; i++)
+			kfree(tab->ops[i].ctx_arg_info);
 
 	kfree(tab);
 	btf->struct_ops_tab = NULL;
@@ -6086,7 +6091,7 @@  bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 			}
 
 			info->reg_type = ctx_arg_info->reg_type;
-			info->btf = btf_vmlinux;
+			info->btf = ctx_arg_info->btf ? ctx_arg_info->btf : btf_vmlinux;
 			info->btf_id = ctx_arg_info->btf_id;
 			return true;
 		}
@@ -8488,11 +8493,62 @@  bool btf_type_ids_nocast_alias(struct bpf_verifier_log *log,
 	return !strncmp(reg_name, arg_name, cmp_len);
 }
 
+static s32 find_member_type_offset(struct btf *btf, u32 btf_id, u32 offset)
+{
+	const struct btf_type *t;
+	const struct btf_member *m;
+	u32 i, n;
+
+	t = btf_type_by_id(btf, btf_id);
+	if (!t)
+		return 0;
+
+	if (!btf_type_is_struct(t))
+		return 0;
+
+	n = btf_type_vlen(t);
+	m = btf_members(t);
+	for (i = 0; i < n; i++) {
+		if (m[i].offset == offset)
+			return m[i].type;
+	}
+
+	return 0;
+}
+
+static s32 find_arg_id_offset(struct btf *btf, u32 btf_id, u32 offset,
+			      u32 arg_no)
+{
+	const struct btf_type *func_proto;
+	s32 member_type_id, arg_type_id;
+	const struct btf_param *args;
+	u32 nargs;
+
+	member_type_id = find_member_type_offset(btf,
+						 btf_id,
+						 offset);
+	if (member_type_id < 0)
+		return -EINVAL;
+	func_proto = btf_type_resolve_func_ptr(btf, member_type_id, NULL);
+	if (!func_proto)
+		return -EINVAL;
+	nargs = btf_type_vlen(func_proto);
+	args = btf_params(func_proto);
+
+	if (!btf_type_resolve_ptr(btf, args[arg_no].type, &arg_type_id))
+		return -EINVAL;
+
+	return arg_type_id;
+}
+
 static int
 btf_add_struct_ops(struct btf *btf, struct bpf_struct_ops *st_ops,
 		   struct bpf_verifier_log *log)
 {
+	struct bpf_struct_ops_arg_info *prev, *arg_info;
+	struct bpf_ctx_arg_aux *arg_aux, *ctx_arg_info;
 	struct btf_struct_ops_tab *tab, *new_tab;
+	s32 btf_id;
 	int i, err;
 
 	if (!btf)
@@ -8530,13 +8586,56 @@  btf_add_struct_ops(struct btf *btf, struct bpf_struct_ops *st_ops,
 
 	tab->ops[btf->struct_ops_tab->cnt].st_ops = st_ops;
 
+	btf_id = btf_find_by_name_kind(btf, st_ops->name, BTF_KIND_STRUCT);
+	if (btf_id < 0)
+		return -EINVAL;
+
+	if (st_ops->arg_info_cnt) {
+		ctx_arg_info = kmalloc(sizeof(*ctx_arg_info) *
+				       st_ops->arg_info_cnt,
+				       GFP_KERNEL);
+		if (!ctx_arg_info)
+			return -ENOMEM;
+
+		arg_aux = ctx_arg_info;
+		for (i = 0; i < st_ops->arg_info_cnt; i++) {
+			/* Make sure all arg_info are in order */
+			prev = &st_ops->arg_info[i - 1];
+			arg_info = &st_ops->arg_info[i];
+			if (i && (arg_info->op_off < prev->op_off ||
+			    (arg_info->op_off == prev->op_off &&
+			     arg_info->arg_no <= prev->arg_no))) {
+				err = -EINVAL;
+				goto errout;
+			}
+			arg_aux->reg_type = arg_info->reg_type;
+			arg_aux->btf_id = find_arg_id_offset(btf,
+							     btf_id,
+							     arg_info->op_off,
+							     arg_info->arg_no);
+			if (arg_aux->btf_id < 0) {
+				err = -EINVAL;
+				goto errout;
+			}
+			arg_aux->offset = arg_info->arg_no * 8;
+			arg_aux->btf = btf;
+			arg_aux++;
+		}
+	}
+	tab->ops[btf->struct_ops_tab->cnt].ctx_arg_info = ctx_arg_info;
+
 	err = bpf_struct_ops_desc_init(&tab->ops[btf->struct_ops_tab->cnt], btf, log);
 	if (err)
-		return err;
+		goto errout;
 
 	btf->struct_ops_tab->cnt++;
 
 	return 0;
+
+errout:
+	kfree(ctx_arg_info);
+
+	return err;
 }
 
 const struct bpf_struct_ops_desc *
@@ -8587,7 +8686,7 @@  int register_bpf_struct_ops(struct bpf_struct_ops *st_ops)
 {
 	struct bpf_verifier_log *log;
 	struct btf *btf;
-	int err = 0;
+	int err;
 
 	btf = btf_get_module_btf(st_ops->owner);
 	if (!btf)
@@ -8609,4 +8708,5 @@  int register_bpf_struct_ops(struct bpf_struct_ops *st_ops)
 
 	return err;
 }
+
 EXPORT_SYMBOL_GPL(register_bpf_struct_ops);
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
@@ -20231,11 +20232,12 @@  static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
 	const struct btf_type *t, *func_proto;
 	const struct bpf_struct_ops_desc *st_ops_desc;
 	const struct bpf_struct_ops *st_ops;
-	const struct btf_member *member;
 	struct bpf_prog *prog = env->prog;
+	const struct btf_member *member;
 	u32 btf_id, member_idx;
 	struct btf *btf;
 	const char *mname;
+	int i, j;
 
 	if (!prog->gpl_compatible) {
 		verbose(env, "struct ops programs must have a GPL compatible license\n");
@@ -20289,6 +20291,16 @@  static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
 		}
 	}
 
+	for (i = 0; i < st_ops->arg_info_cnt; i++) {
+		if (st_ops->arg_info[i].op_off != member->offset)
+			continue;
+		prog->aux->ctx_arg_info = &st_ops_desc->ctx_arg_info[i];
+		for (j = i + 1; j < st_ops->arg_info_cnt; j++)
+			if (st_ops->arg_info[j].op_off != member->offset)
+				break;
+		prog->aux->ctx_arg_info_size = j - i;
+	}
+
 	prog->aux->attach_func_proto = func_proto;
 	prog->aux->attach_func_name = mname;
 	env->ops = st_ops->verifier_ops;
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
index fe945d093378..64e966acfb1b 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c
@@ -555,7 +555,12 @@  static int bpf_dummy_reg(void *kdata)
 	struct bpf_testmod_ops *ops = kdata;
 	int r;
 
-	r = ops->test_2(4, 3);
+	if (ops->test_maybe_null)
+		r = ops->test_maybe_null(0, NULL);
+	else if (ops->test_non_maybe_null)
+		r = ops->test_non_maybe_null(0, NULL);
+	else
+		r = ops->test_2(4, 3);
 
 	return 0;
 }
@@ -574,9 +579,25 @@  static int bpf_testmod_test_2(int a, int b)
 	return 0;
 }
 
+static int bpf_testmod_test_maybe_null(int dummy, struct task_struct *task)
+{
+	return 0;
+}
+
+static int bpf_testmod_test_non_maybe_null(int dummy, struct task_struct *task)
+{
+	return 0;
+}
+
 static struct bpf_testmod_ops __bpf_testmod_ops = {
 	.test_1 = bpf_testmod_test_1,
 	.test_2 = bpf_testmod_test_2,
+	.test_maybe_null = bpf_testmod_test_maybe_null,
+	.test_non_maybe_null = bpf_testmod_test_non_maybe_null,
+};
+
+struct bpf_struct_ops_arg_info testmod_arg_info[] = {
+	ST_OPS_ARG_MAYBE_NULL(struct bpf_testmod_ops, test_maybe_null, 1),
 };
 
 struct bpf_struct_ops bpf_bpf_testmod_ops = {
@@ -588,6 +609,8 @@  struct bpf_struct_ops bpf_bpf_testmod_ops = {
 	.cfi_stubs = &__bpf_testmod_ops,
 	.name = "bpf_testmod_ops",
 	.owner = THIS_MODULE,
+	.arg_info = testmod_arg_info,
+	.arg_info_cnt = ARRAY_SIZE(testmod_arg_info),
 };
 
 extern int bpf_fentry_test1(int a);
diff --git a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
index ca5435751c79..846b472e4810 100644
--- a/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
+++ b/tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.h
@@ -5,6 +5,8 @@ 
 
 #include <linux/types.h>
 
+struct task_struct;
+
 struct bpf_testmod_test_read_ctx {
 	char *buf;
 	loff_t off;
@@ -31,6 +33,8 @@  struct bpf_iter_testmod_seq {
 struct bpf_testmod_ops {
 	int (*test_1)(void);
 	int (*test_2)(int a, int b);
+	int (*test_maybe_null)(int dummy, struct task_struct *task);
+	int (*test_non_maybe_null)(int dummy, struct task_struct *task);
 };
 
 #endif /* _BPF_TESTMOD_H */
diff --git a/tools/testing/selftests/bpf/prog_tests/test_struct_ops_maybe_null.c b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_maybe_null.c
new file mode 100644
index 000000000000..4477dfcf1cd7
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/test_struct_ops_maybe_null.c
@@ -0,0 +1,39 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
+#include <test_progs.h>
+#include <time.h>
+
+#include "struct_ops_maybe_null.skel.h"
+#include "struct_ops_maybe_null_fail.skel.h"
+
+static void maybe_null(void)
+{
+	struct struct_ops_maybe_null *skel;
+
+	skel = struct_ops_maybe_null__open_and_load();
+	if (!ASSERT_OK_PTR(skel, "struct_ops_module_open_and_load"))
+		return;
+	
+	struct_ops_maybe_null__destroy(skel);
+}
+
+static void maybe_null_fail(void)
+{
+	struct struct_ops_maybe_null_fail *skel;
+
+	skel = struct_ops_maybe_null_fail__open_and_load();
+	if (!ASSERT_ERR_PTR(skel, "struct_ops_module_fail__open_and_load"))
+		return;
+	
+	struct_ops_maybe_null_fail__destroy(skel);
+}
+
+void test_struct_ops_maybe_null(void)
+{
+	if (test__start_subtest("maybe_null"))
+		maybe_null();
+	if (test__start_subtest("maybe_null_fail"))
+		maybe_null_fail();
+}
+
+
diff --git a/tools/testing/selftests/bpf/progs/struct_ops_maybe_null.c b/tools/testing/selftests/bpf/progs/struct_ops_maybe_null.c
new file mode 100644
index 000000000000..adbbb17865fb
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/struct_ops_maybe_null.c
@@ -0,0 +1,27 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
+#include <vmlinux.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include "../bpf_testmod/bpf_testmod.h"
+
+char _license[] SEC("license") = "GPL";
+
+u64 tgid = 0;
+
+SEC("struct_ops/test_maybe_null")
+int BPF_PROG(test_maybe_null, int dummy, struct task_struct *task)
+{
+	if (task == NULL)
+		tgid = 0;
+	else
+		tgid = task->tgid;
+
+	return 0;
+}
+
+SEC(".struct_ops.link")
+struct bpf_testmod_ops testmod_1 = {
+	.test_maybe_null = (void *)test_maybe_null,
+};
+
diff --git a/tools/testing/selftests/bpf/progs/struct_ops_maybe_null_fail.c b/tools/testing/selftests/bpf/progs/struct_ops_maybe_null_fail.c
new file mode 100644
index 000000000000..2f7a9b6ef864
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/struct_ops_maybe_null_fail.c
@@ -0,0 +1,25 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2023 Meta Platforms, Inc. and affiliates. */
+#include <vmlinux.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include "../bpf_testmod/bpf_testmod.h"
+
+char _license[] SEC("license") = "GPL";
+
+int tgid = 0;
+
+SEC("struct_ops/test_maybe_null")
+int BPF_PROG(test_maybe_null, int dummy, struct task_struct *task)
+{
+	tgid = task->tgid;
+
+	return 0;
+}
+
+SEC(".struct_ops.link")
+struct bpf_testmod_ops testmod_1 = {
+	.test_maybe_null = (void *)test_maybe_null,
+};
+
+