| Message ID | 20230823225556.1292811-1-yonghong.song@linux.dev (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,1/2] bpf: Remove a WARN_ON_ONCE warning related to local kptr | expand |
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 :
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 :
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> --- 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.