Re: [PATCH bpf-next 1/2] bpf: Remove a WARN_ON_ONCE warning related to local kptr

[David Marchevsky <david.marchevsky@linux.dev>]

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@linux.dev>
> ---

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@fb.com>


>  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 :