On 8/28/24 4:24 PM, Alexei Starovoitov wrote:
@@ -714,6 +869,11 @@ void bpf_obj_free_fields(const struct btf_record *rec, void *obj) field->kptr.dtor(xchgd_field); } break; + case BPF_UPTR: + if (*(void **)field_ptr) + bpf_obj_unpin_uptr(field, *(void **)field_ptr); + *(void **)field_ptr = NULL;This one will be called from task_storage_delete->bpf_selem_free->bpf_obj_free_fields and even if upin was safe to do from that context we cannot just do: *(void **)field_ptr = NULL; since bpf prog might be running in parallel, it could have just read that addr and now is using it. The first thought of a way to fix this was to split bpf_obj_free_fields() into the current one plus bpf_obj_free_fields_after_gp() that will do the above unpin bit. and call the later one from bpf_selem_free_rcu() while bpf_obj_free_fields() from bpf_selem_free() will not touch uptr. But after digging further I realized that task_storage already switched to use bpf_ma, so the above won't work. So we need something similar to BPF_KPTR_REF logic: xchgd_field = (void *)xchg((unsigned long *)field_ptr, 0); and then delay of uptr unpin for that address into call_rcu. Any better ideas?
Many thanks to Kui-Feng starting this useful work on task storage. I will think about it and respin the set.
