On 8/23/23 6:55 PM, Yonghong Song wrote:
> Currently, in function bpf_obj_free_fields(), for local kptr,
> a warning will be issued if the struct does not contain any
> special fields. But actually the kernel seems totally okay
> with a local kptr without any special fields. Permitting
> no special fields also aligns with future percpu kptr which
> also allows no special fields.
>
> Signed-off-by: Yonghong Song <yonghong.song@xxxxxxxxx>
> ---
Weird. Looking at the WARN_ON_ONCE now, I can't understand why I added it,
and history of the series adding it doesn't have any clues. The same series
added pointee_struct_meta ? pointee_struct_meta->record : NULL two lines below,
so it's not clear what I was trying to protect against.
Anyways, I agree that:
* We can have a struct with a special __kptr field that points to some
local kptr type
* That local kptr 'pointee' type doesn't need to have any special fields, in
which case pointee_struct_meta will rightly be NULL, a NULL record will be
passed to __bpf_obj_drop_impl, which will handle it correctly.
* In fact this is the same logic that bpf_obj_drop_impl does before calling
its double-underscore cousin
LGTM
Acked-by: Dave Marchevsky <davemarchevsky@xxxxxx>
> kernel/bpf/syscall.c | 1 -
> 1 file changed, 1 deletion(-)
>
> NOTE: I didn't put a fix tag since except the warning
> there is no correctness issue here.
>
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 10666d17b9e3..ebeb0695305a 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -657,7 +657,6 @@ void bpf_obj_free_fields(const struct btf_record *rec, void *obj)
> if (!btf_is_kernel(field->kptr.btf)) {
> pointee_struct_meta = btf_find_struct_meta(field->kptr.btf,
> field->kptr.btf_id);
> - WARN_ON_ONCE(!pointee_struct_meta);
> migrate_disable();
> __bpf_obj_drop_impl(xchgd_field, pointee_struct_meta ?
> pointee_struct_meta->record :
