diff mbox series

[bpf-next,v13,07/14] bpf: pass attached BTF to the bpf_struct_ops subsystem

Message ID 20231209002709.535966-8-thinker.li@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series Registrating struct_ops types from modules | expand

Checks

Context Check Description
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/ynl success SINGLE THREAD; Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 7875 this patch: 7875
netdev/cc_maintainers warning 8 maintainers not CCed: kpsingh@kernel.org daniel@iogearbox.net netdev@vger.kernel.org yonghong.song@linux.dev sdf@google.com haoluo@google.com jolsa@kernel.org john.fastabend@gmail.com
netdev/build_clang success Errors and warnings before: 2579 this patch: 2579
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 8410 this patch: 8410
netdev/checkpatch warning WARNING: ENOTSUPP is not a SUSV4 error code, prefer EOPNOTSUPP WARNING: line length of 86 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-PR success PR summary
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-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
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-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-13 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-15 success Logs for x86_64-gcc / build-release
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-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
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-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-14 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-18 success Logs for set-matrix
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-20 success Logs for x86_64-gcc / build-release
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-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-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-42 success Logs for x86_64-llvm-18 / veristat
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-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-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-27 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
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-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-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-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-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-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-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-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-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-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-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-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

Commit Message

Kui-Feng Lee Dec. 9, 2023, 12:27 a.m. UTC 
From: Kui-Feng Lee <thinker.li@gmail.com>

Pass the fd of a btf from the userspace to the bpf() syscall, and then
convert the fd into a btf. The btf is generated from the module that
defines the target BPF struct_ops type.

In order to inform the kernel about the module that defines the target
struct_ops type, the userspace program needs to provide a btf fd for the
respective module's btf. This btf contains essential information on the
types defined within the module, including the target struct_ops type.

A btf fd must be provided to the kernel for struct_ops maps struct_ops and
for the bpf programs attached to those maps.

In the case of the bpf programs, the attach_btf_obj_fd parameter is passed
as part of the bpf_attr and is converted into a btf. This btf is then
stored in the prog->aux->attach_btf field. Here, it just let the verifier
access attach_btf directly.

In the case of struct_ops maps, a btf fd is passed as value_type_btf_obj_fd
of bpf_attr. The bpf_struct_ops_map_alloc() function converts the fd to a
btf and stores it as st_map->btf.

Signed-off-by: Kui-Feng Lee <thinker.li@gmail.com>
---
 include/uapi/linux/bpf.h       |  5 +++
 kernel/bpf/bpf_struct_ops.c    | 56 +++++++++++++++++++++++-----------
 kernel/bpf/syscall.c           |  2 +-
 kernel/bpf/verifier.c          |  9 ++++--
 tools/include/uapi/linux/bpf.h |  5 +++
 5 files changed, 56 insertions(+), 21 deletions(-)

Comments

Martin KaFai Lau Dec. 15, 2023, 2:44 a.m. UTC | #1 
On 12/8/23 4:27 PM, thinker.li@gmail.com wrote:
> @@ -681,15 +682,30 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>   	struct bpf_struct_ops_map *st_map;
>   	const struct btf_type *t, *vt;
>   	struct bpf_map *map;
> +	struct btf *btf;
>   	int ret;
>   
> -	st_ops_desc = bpf_struct_ops_find_value(btf_vmlinux, attr->btf_vmlinux_value_type_id);
> -	if (!st_ops_desc)
> -		return ERR_PTR(-ENOTSUPP);
> +	if (attr->value_type_btf_obj_fd) {
> +		/* The map holds btf for its whole life time. */
> +		btf = btf_get_by_fd(attr->value_type_btf_obj_fd);
> +		if (IS_ERR(btf))
> +			return ERR_PTR(PTR_ERR(btf));

			return ERR_CAST(btf);

It needs to check for btf_is_module:

		if (!btf_is_module(btf)) {
			btf_put(btf);
			return ERR_PTR(-EINVAL);
		}

> +	} else {
> +		btf = btf_vmlinux;
> +		btf_get(btf);

btf_vmlinux could be NULL or a ERR_PTR? Lets take this chance to use 
bpf_get_btf_vmlinux():
		btf = bpf_get_btf_vmlinux();
	        if (IS_ERR(btf))
         	        return ERR_CAST(btf);

Going back to patch 4. This should be the only case that btf will be NULL or 
ERR_PTR?

will continue the remaining review later tonight.

pw-bot: cr

> +	}
> +
> +	st_ops_desc = bpf_struct_ops_find_value(btf, attr->btf_vmlinux_value_type_id);
> +	if (!st_ops_desc) {
> +		ret = -ENOTSUPP;
> +		goto errout;
> +	}
>   
>   	vt = st_ops_desc->value_type;
> -	if (attr->value_size != vt->size)
> -		return ERR_PTR(-EINVAL);
> +	if (attr->value_size != vt->size) {
> +		ret = -EINVAL;
> +		goto errout;
> +	}
>   
>   	t = st_ops_desc->type;
>
Kui-Feng Lee Dec. 15, 2023, 10:10 p.m. UTC | #2 
On 12/14/23 18:44, Martin KaFai Lau wrote:
> On 12/8/23 4:27 PM, thinker.li@gmail.com wrote:
>> @@ -681,15 +682,30 @@ static struct bpf_map 
>> *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>>       struct bpf_struct_ops_map *st_map;
>>       const struct btf_type *t, *vt;
>>       struct bpf_map *map;
>> +    struct btf *btf;
>>       int ret;
>> -    st_ops_desc = bpf_struct_ops_find_value(btf_vmlinux, 
>> attr->btf_vmlinux_value_type_id);
>> -    if (!st_ops_desc)
>> -        return ERR_PTR(-ENOTSUPP);
>> +    if (attr->value_type_btf_obj_fd) {
>> +        /* The map holds btf for its whole life time. */
>> +        btf = btf_get_by_fd(attr->value_type_btf_obj_fd);
>> +        if (IS_ERR(btf))
>> +            return ERR_PTR(PTR_ERR(btf));
> 
>              return ERR_CAST(btf);
> 
> It needs to check for btf_is_module:
> 
>          if (!btf_is_module(btf)) {
>              btf_put(btf);
>              return ERR_PTR(-EINVAL);
>          }

Even btf is btf_vmlinux the kernel's btf, it still works.
Although libbpf pass 0 as the value of value_type_btf_obj_fd for
btf_vmlinux now, it should be OK for a user space loader to
pass a fd of btf_vmlinux.

WDYT?

> 
>> +    } else {
>> +        btf = btf_vmlinux;
>> +        btf_get(btf);
> 
> btf_vmlinux could be NULL or a ERR_PTR? Lets take this chance to use 
> bpf_get_btf_vmlinux():
>          btf = bpf_get_btf_vmlinux();
>              if (IS_ERR(btf))
>                      return ERR_CAST(btf);

Sure!

> 
> Going back to patch 4. This should be the only case that btf will be 
> NULL or ERR_PTR?
> 
> will continue the remaining review later tonight.
> 
> pw-bot: cr
> 
>> +    }
>> +
>> +    st_ops_desc = bpf_struct_ops_find_value(btf, 
>> attr->btf_vmlinux_value_type_id);
>> +    if (!st_ops_desc) {
>> +        ret = -ENOTSUPP;
>> +        goto errout;
>> +    }
>>       vt = st_ops_desc->value_type;
>> -    if (attr->value_size != vt->size)
>> -        return ERR_PTR(-EINVAL);
>> +    if (attr->value_size != vt->size) {
>> +        ret = -EINVAL;
>> +        goto errout;
>> +    }
>>       t = st_ops_desc->type;
>
Martin KaFai Lau Dec. 16, 2023, 12:19 a.m. UTC | #3 
On 12/15/23 2:10 PM, Kui-Feng Lee wrote:
> 
> 
> On 12/14/23 18:44, Martin KaFai Lau wrote:
>> On 12/8/23 4:27 PM, thinker.li@gmail.com wrote:
>>> @@ -681,15 +682,30 @@ static struct bpf_map *bpf_struct_ops_map_alloc(union 
>>> bpf_attr *attr)
>>>       struct bpf_struct_ops_map *st_map;
>>>       const struct btf_type *t, *vt;
>>>       struct bpf_map *map;
>>> +    struct btf *btf;
>>>       int ret;
>>> -    st_ops_desc = bpf_struct_ops_find_value(btf_vmlinux, 
>>> attr->btf_vmlinux_value_type_id);
>>> -    if (!st_ops_desc)
>>> -        return ERR_PTR(-ENOTSUPP);
>>> +    if (attr->value_type_btf_obj_fd) {
>>> +        /* The map holds btf for its whole life time. */
>>> +        btf = btf_get_by_fd(attr->value_type_btf_obj_fd);
>>> +        if (IS_ERR(btf))
>>> +            return ERR_PTR(PTR_ERR(btf));
>>
>>              return ERR_CAST(btf);
>>
>> It needs to check for btf_is_module:
>>
>>          if (!btf_is_module(btf)) {
>>              btf_put(btf);
>>              return ERR_PTR(-EINVAL);
>>          }
> 
> Even btf is btf_vmlinux the kernel's btf, it still works.

btf could be a bpf program's btf. It needs to ensure it is a kernel module btf 
here.

> Although libbpf pass 0 as the value of value_type_btf_obj_fd for
> btf_vmlinux now, it should be OK for a user space loader to
> pass a fd of btf_vmlinux.
> 
> WDYT?
Kui-Feng Lee Dec. 16, 2023, 5:55 a.m. UTC | #4 
On 12/15/23 16:19, Martin KaFai Lau wrote:
> On 12/15/23 2:10 PM, Kui-Feng Lee wrote:
>>
>>
>> On 12/14/23 18:44, Martin KaFai Lau wrote:
>>> On 12/8/23 4:27 PM, thinker.li@gmail.com wrote:
>>>> @@ -681,15 +682,30 @@ static struct bpf_map 
>>>> *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>>>>       struct bpf_struct_ops_map *st_map;
>>>>       const struct btf_type *t, *vt;
>>>>       struct bpf_map *map;
>>>> +    struct btf *btf;
>>>>       int ret;
>>>> -    st_ops_desc = bpf_struct_ops_find_value(btf_vmlinux, 
>>>> attr->btf_vmlinux_value_type_id);
>>>> -    if (!st_ops_desc)
>>>> -        return ERR_PTR(-ENOTSUPP);
>>>> +    if (attr->value_type_btf_obj_fd) {
>>>> +        /* The map holds btf for its whole life time. */
>>>> +        btf = btf_get_by_fd(attr->value_type_btf_obj_fd);
>>>> +        if (IS_ERR(btf))
>>>> +            return ERR_PTR(PTR_ERR(btf));
>>>
>>>              return ERR_CAST(btf);
>>>
>>> It needs to check for btf_is_module:
>>>
>>>          if (!btf_is_module(btf)) {
>>>              btf_put(btf);
>>>              return ERR_PTR(-EINVAL);
>>>          }
>>
>> Even btf is btf_vmlinux the kernel's btf, it still works.
> 
> btf could be a bpf program's btf. It needs to ensure it is a kernel 
> module btf here.

Got it!

> 
>> Although libbpf pass 0 as the value of value_type_btf_obj_fd for
>> btf_vmlinux now, it should be OK for a user space loader to
>> pass a fd of btf_vmlinux.
>>
>> WDYT?
>
Kui-Feng Lee Dec. 16, 2023, 6:07 a.m. UTC | #5 
On 12/15/23 21:55, Kui-Feng Lee wrote:
> 
> 
> On 12/15/23 16:19, Martin KaFai Lau wrote:
>> On 12/15/23 2:10 PM, Kui-Feng Lee wrote:
>>>
>>>
>>> On 12/14/23 18:44, Martin KaFai Lau wrote:
>>>> On 12/8/23 4:27 PM, thinker.li@gmail.com wrote:
>>>>> @@ -681,15 +682,30 @@ static struct bpf_map 
>>>>> *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>>>>>       struct bpf_struct_ops_map *st_map;
>>>>>       const struct btf_type *t, *vt;
>>>>>       struct bpf_map *map;
>>>>> +    struct btf *btf;
>>>>>       int ret;
>>>>> -    st_ops_desc = bpf_struct_ops_find_value(btf_vmlinux, 
>>>>> attr->btf_vmlinux_value_type_id);
>>>>> -    if (!st_ops_desc)
>>>>> -        return ERR_PTR(-ENOTSUPP);
>>>>> +    if (attr->value_type_btf_obj_fd) {
>>>>> +        /* The map holds btf for its whole life time. */
>>>>> +        btf = btf_get_by_fd(attr->value_type_btf_obj_fd);
>>>>> +        if (IS_ERR(btf))
>>>>> +            return ERR_PTR(PTR_ERR(btf));
>>>>
>>>>              return ERR_CAST(btf);
>>>>
>>>> It needs to check for btf_is_module:
>>>>
>>>>          if (!btf_is_module(btf)) {
>>>>              btf_put(btf);
>>>>              return ERR_PTR(-EINVAL);
>>>>          }
>>>
>>> Even btf is btf_vmlinux the kernel's btf, it still works.
>>
>> btf could be a bpf program's btf. It needs to ensure it is a kernel 
>> module btf here.
> 
> Got it!

Isn't btf_is_kernel() better here?
User space may pass a fd to btf_vmlinux.

> 
>>
>>> Although libbpf pass 0 as the value of value_type_btf_obj_fd for
>>> btf_vmlinux now, it should be OK for a user space loader to
>>> pass a fd of btf_vmlinux.
>>>
>>> WDYT?
>>
Martin KaFai Lau Dec. 16, 2023, 4:41 p.m. UTC | #6 
On 12/15/23 10:07 PM, Kui-Feng Lee wrote:
> 
> 
> On 12/15/23 21:55, Kui-Feng Lee wrote:
>>
>>
>> On 12/15/23 16:19, Martin KaFai Lau wrote:
>>> On 12/15/23 2:10 PM, Kui-Feng Lee wrote:
>>>>
>>>>
>>>> On 12/14/23 18:44, Martin KaFai Lau wrote:
>>>>> On 12/8/23 4:27 PM, thinker.li@gmail.com wrote:
>>>>>> @@ -681,15 +682,30 @@ static struct bpf_map 
>>>>>> *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>>>>>>       struct bpf_struct_ops_map *st_map;
>>>>>>       const struct btf_type *t, *vt;
>>>>>>       struct bpf_map *map;
>>>>>> +    struct btf *btf;
>>>>>>       int ret;
>>>>>> -    st_ops_desc = bpf_struct_ops_find_value(btf_vmlinux, 
>>>>>> attr->btf_vmlinux_value_type_id);
>>>>>> -    if (!st_ops_desc)
>>>>>> -        return ERR_PTR(-ENOTSUPP);
>>>>>> +    if (attr->value_type_btf_obj_fd) {
>>>>>> +        /* The map holds btf for its whole life time. */
>>>>>> +        btf = btf_get_by_fd(attr->value_type_btf_obj_fd);
>>>>>> +        if (IS_ERR(btf))
>>>>>> +            return ERR_PTR(PTR_ERR(btf));
>>>>>
>>>>>              return ERR_CAST(btf);
>>>>>
>>>>> It needs to check for btf_is_module:
>>>>>
>>>>>          if (!btf_is_module(btf)) {
>>>>>              btf_put(btf);
>>>>>              return ERR_PTR(-EINVAL);
>>>>>          }
>>>>
>>>> Even btf is btf_vmlinux the kernel's btf, it still works.
>>>
>>> btf could be a bpf program's btf. It needs to ensure it is a kernel module 
>>> btf here.
>>
>> Got it!
> 
> Isn't btf_is_kernel() better here?
> User space may pass a fd to btf_vmlinux.

Limit it to btf_is_module. What is the benefit of supporting btf_vmlinux as a fd 
while fd 0 already means btf_vmlinux?

kfunc does not support the btf_vmlinux as fd also, supporting in struct_ops 
alone is confusing. I don't think the major user (libbpf) does this either, so 
really not much usage other than a confusing API to have both fd == 0 and a 
btf_vmlinux's fd to mean btf_vmlinux.

> 
>>
>>>
>>>> Although libbpf pass 0 as the value of value_type_btf_obj_fd for
>>>> btf_vmlinux now, it should be OK for a user space loader to
>>>> pass a fd of btf_vmlinux.
>>>>
>>>> WDYT?
>>>
Kui-Feng Lee Dec. 16, 2023, 7:38 p.m. UTC | #7 
On 12/16/23 08:41, Martin KaFai Lau wrote:
> On 12/15/23 10:07 PM, Kui-Feng Lee wrote:
>>
>>
>> On 12/15/23 21:55, Kui-Feng Lee wrote:
>>>
>>>
>>> On 12/15/23 16:19, Martin KaFai Lau wrote:
>>>> On 12/15/23 2:10 PM, Kui-Feng Lee wrote:
>>>>>
>>>>>
>>>>> On 12/14/23 18:44, Martin KaFai Lau wrote:
>>>>>> On 12/8/23 4:27 PM, thinker.li@gmail.com wrote:
>>>>>>> @@ -681,15 +682,30 @@ static struct bpf_map 
>>>>>>> *bpf_struct_ops_map_alloc(union bpf_attr *attr)
>>>>>>>       struct bpf_struct_ops_map *st_map;
>>>>>>>       const struct btf_type *t, *vt;
>>>>>>>       struct bpf_map *map;
>>>>>>> +    struct btf *btf;
>>>>>>>       int ret;
>>>>>>> -    st_ops_desc = bpf_struct_ops_find_value(btf_vmlinux, 
>>>>>>> attr->btf_vmlinux_value_type_id);
>>>>>>> -    if (!st_ops_desc)
>>>>>>> -        return ERR_PTR(-ENOTSUPP);
>>>>>>> +    if (attr->value_type_btf_obj_fd) {
>>>>>>> +        /* The map holds btf for its whole life time. */
>>>>>>> +        btf = btf_get_by_fd(attr->value_type_btf_obj_fd);
>>>>>>> +        if (IS_ERR(btf))
>>>>>>> +            return ERR_PTR(PTR_ERR(btf));
>>>>>>
>>>>>>              return ERR_CAST(btf);
>>>>>>
>>>>>> It needs to check for btf_is_module:
>>>>>>
>>>>>>          if (!btf_is_module(btf)) {
>>>>>>              btf_put(btf);
>>>>>>              return ERR_PTR(-EINVAL);
>>>>>>          }
>>>>>
>>>>> Even btf is btf_vmlinux the kernel's btf, it still works.
>>>>
>>>> btf could be a bpf program's btf. It needs to ensure it is a kernel 
>>>> module btf here.
>>>
>>> Got it!
>>
>> Isn't btf_is_kernel() better here?
>> User space may pass a fd to btf_vmlinux.
> 
> Limit it to btf_is_module. What is the benefit of supporting btf_vmlinux 
> as a fd while fd 0 already means btf_vmlinux?
> 
> kfunc does not support the btf_vmlinux as fd also, supporting in 
> struct_ops alone is confusing. I don't think the major user (libbpf) 
> does this either, so really not much usage other than a confusing API to 
> have both fd == 0 and a btf_vmlinux's fd to mean btf_vmlinux.

It is fair.

> 
>>
>>>
>>>>
>>>>> Although libbpf pass 0 as the value of value_type_btf_obj_fd for
>>>>> btf_vmlinux now, it should be OK for a user space loader to
>>>>> pass a fd of btf_vmlinux.
>>>>>
>>>>> WDYT?
>>>>
>
diff mbox series

Patch

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index e0545201b55f..5c3838a97554 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1438,6 +1438,11 @@  union bpf_attr {
 		 */
 		__u64	map_extra;
 		__u32	map_token_fd;
+
+		__u32   value_type_btf_obj_fd;	/* fd pointing to a BTF
+						 * type data for
+						 * btf_vmlinux_value_type_id.
+						 */
 	};
 
 	struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */
diff --git a/kernel/bpf/bpf_struct_ops.c b/kernel/bpf/bpf_struct_ops.c
index ed4d84a8437c..f943f8378e76 100644
--- a/kernel/bpf/bpf_struct_ops.c
+++ b/kernel/bpf/bpf_struct_ops.c
@@ -641,6 +641,7 @@  static void __bpf_struct_ops_map_free(struct bpf_map *map)
 		bpf_jit_uncharge_modmem(PAGE_SIZE);
 	}
 	bpf_map_area_free(st_map->uvalue);
+	btf_put(st_map->btf);
 	bpf_map_area_free(st_map);
 }
 
@@ -681,15 +682,30 @@  static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
 	struct bpf_struct_ops_map *st_map;
 	const struct btf_type *t, *vt;
 	struct bpf_map *map;
+	struct btf *btf;
 	int ret;
 
-	st_ops_desc = bpf_struct_ops_find_value(btf_vmlinux, attr->btf_vmlinux_value_type_id);
-	if (!st_ops_desc)
-		return ERR_PTR(-ENOTSUPP);
+	if (attr->value_type_btf_obj_fd) {
+		/* The map holds btf for its whole life time. */
+		btf = btf_get_by_fd(attr->value_type_btf_obj_fd);
+		if (IS_ERR(btf))
+			return ERR_PTR(PTR_ERR(btf));
+	} else {
+		btf = btf_vmlinux;
+		btf_get(btf);
+	}
+
+	st_ops_desc = bpf_struct_ops_find_value(btf, attr->btf_vmlinux_value_type_id);
+	if (!st_ops_desc) {
+		ret = -ENOTSUPP;
+		goto errout;
+	}
 
 	vt = st_ops_desc->value_type;
-	if (attr->value_size != vt->size)
-		return ERR_PTR(-EINVAL);
+	if (attr->value_size != vt->size) {
+		ret = -EINVAL;
+		goto errout;
+	}
 
 	t = st_ops_desc->type;
 
@@ -700,17 +716,17 @@  static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
 		(vt->size - sizeof(struct bpf_struct_ops_value));
 
 	st_map = bpf_map_area_alloc(st_map_size, NUMA_NO_NODE);
-	if (!st_map)
-		return ERR_PTR(-ENOMEM);
+	if (!st_map) {
+		ret = -ENOMEM;
+		goto errout;
+	}
 
 	st_map->st_ops_desc = st_ops_desc;
 	map = &st_map->map;
 
 	ret = bpf_jit_charge_modmem(PAGE_SIZE);
-	if (ret) {
-		__bpf_struct_ops_map_free(map);
-		return ERR_PTR(ret);
-	}
+	if (ret)
+		goto errout_free;
 
 	st_map->image = arch_alloc_bpf_trampoline(PAGE_SIZE);
 	if (!st_map->image) {
@@ -719,24 +735,30 @@  static struct bpf_map *bpf_struct_ops_map_alloc(union bpf_attr *attr)
 		 * here.
 		 */
 		bpf_jit_uncharge_modmem(PAGE_SIZE);
-		__bpf_struct_ops_map_free(map);
-		return ERR_PTR(-ENOMEM);
+		ret = -ENOMEM;
+		goto errout_free;
 	}
 	st_map->uvalue = bpf_map_area_alloc(vt->size, NUMA_NO_NODE);
 	st_map->links =
 		bpf_map_area_alloc(btf_type_vlen(t) * sizeof(struct bpf_links *),
 				   NUMA_NO_NODE);
 	if (!st_map->uvalue || !st_map->links) {
-		__bpf_struct_ops_map_free(map);
-		return ERR_PTR(-ENOMEM);
+		ret = -ENOMEM;
+		goto errout_free;
 	}
-
-	st_map->btf = btf_vmlinux;
+	st_map->btf = btf;
 
 	mutex_init(&st_map->lock);
 	bpf_map_init_from_attr(map, attr);
 
 	return map;
+
+errout_free:
+	__bpf_struct_ops_map_free(map);
+errout:
+	btf_put(btf);
+
+	return ERR_PTR(ret);
 }
 
 static u64 bpf_struct_ops_map_mem_usage(const struct bpf_map *map)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index aff045eed375..4aced7e58904 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1126,7 +1126,7 @@  static bool bpf_net_capable(void)
 	return capable(CAP_NET_ADMIN) || capable(CAP_SYS_ADMIN);
 }
 
-#define BPF_MAP_CREATE_LAST_FIELD map_token_fd
+#define BPF_MAP_CREATE_LAST_FIELD value_type_btf_obj_fd
 /* called via syscall */
 static int map_create(union bpf_attr *attr)
 {
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 6b45f56f8d4c..795c16f9cf57 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -20070,6 +20070,7 @@  static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
 	const struct btf_member *member;
 	struct bpf_prog *prog = env->prog;
 	u32 btf_id, member_idx;
+	struct btf *btf;
 	const char *mname;
 
 	if (!prog->gpl_compatible) {
@@ -20077,8 +20078,10 @@  static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
 		return -EINVAL;
 	}
 
+	btf = prog->aux->attach_btf;
+
 	btf_id = prog->aux->attach_btf_id;
-	st_ops_desc = bpf_struct_ops_find(btf_vmlinux, btf_id);
+	st_ops_desc = bpf_struct_ops_find(btf, btf_id);
 	if (!st_ops_desc) {
 		verbose(env, "attach_btf_id %u is not a supported struct\n",
 			btf_id);
@@ -20095,8 +20098,8 @@  static int check_struct_ops_btf_id(struct bpf_verifier_env *env)
 	}
 
 	member = &btf_type_member(t)[member_idx];
-	mname = btf_name_by_offset(btf_vmlinux, member->name_off);
-	func_proto = btf_type_resolve_func_ptr(btf_vmlinux, member->type,
+	mname = btf_name_by_offset(btf, member->name_off);
+	func_proto = btf_type_resolve_func_ptr(btf, member->type,
 					       NULL);
 	if (!func_proto) {
 		verbose(env, "attach to invalid member %s(@idx %u) of struct %s\n",
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index e0545201b55f..5c3838a97554 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1438,6 +1438,11 @@  union bpf_attr {
 		 */
 		__u64	map_extra;
 		__u32	map_token_fd;
+
+		__u32   value_type_btf_obj_fd;	/* fd pointing to a BTF
+						 * type data for
+						 * btf_vmlinux_value_type_id.
+						 */
 	};
 
 	struct { /* anonymous struct used by BPF_MAP_*_ELEM commands */