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

[Andrii Nakryiko]

On Tue, Oct 28, 2025 at 7:48 AM Leon Hwang <leon.hwang@xxxxxxxxx> wrote:
>
>
>
> 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.

I've dropped it while applying, no need to resend.

>
> Thanks,
> Leon
>