Re: [RFC bpf-next v4 4/6] bpf: pin, translate, and unpin __uptr from syscalls.

Martin KaFai Lau
2024-09-04T22:21:58Z

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.