Re: [PATCH bpf v3 3/4] bpf: Free special fields when update local storage maps

[Leon Hwang]

On 2025/10/28 01:04, Amery Hung wrote:
> On Mon, Oct 27, 2025 at 9:15 AM Leon Hwang <leon.hwang@xxxxxxxxx> wrote:
>>
>> Hi Amery,
>>
>> On 2025/10/27 23:44, Amery Hung wrote:
>>> On Sun, Oct 26, 2025 at 8:41 AM Leon Hwang <leon.hwang@xxxxxxxxx> wrote:

[...]

>>>>                 selem = SELEM(old_sdata);
>>>>                 goto unlock;
>>>>         }
>>>> @@ -654,6 +656,7 @@ bpf_local_storage_update(void *owner, struct bpf_local_storage_map *smap,
>>>>
>>>>         /* Third, remove old selem, SELEM(old_sdata) */
>>>>         if (old_sdata) {
>>>> +               bpf_obj_free_fields(smap->map.record, old_sdata->data);
>>>
>>> Is this really needed? bpf_selem_free_list() later should free special
>>> fields in this selem.
>>>
>>
>> Yes, it’s needed. The new selftest confirms that the special fields are
>> not freed when updating a local storage map.
>>
> 
> Hmmm. I don't think so.
> 
>> Also, bpf_selem_unlink_storage_nolock() doesn’t invoke
>> bpf_selem_free_list(), unlike bpf_selem_unlink_storage(). So we need to
>> call bpf_obj_free_fields() here explicitly to free those fields.
>>
> 
> bpf_selem_unlink_storage_nolock() unlinks the old selem and adds it to
> old_selem_free_list. Later, bpf_selem_free_list() will call
> bpf_selem_free() to free selem in bpf_selem_free_list, which should
> also free special fields in the selem.
> 
> The selftests may have checked the refcount before an task trace RCU
> gp and thought it is a leak. I added a 300ms delay before the checking
> program runs and the test did not detect any leak even without this
> specific bpf_obj_free_fields().

Yeah, you're right. Thanks for the clear explanation.

I also verified it by adding a 300ms delay.

So this bpf_obj_free_fields() call isn't needed — I'll drop it in the
next revision.

Thanks,
Leon