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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEf4BzbRA1Ka1piTiQjBfG2z8tp2ucJh_YuazFWNqAVtOhD48g@mail.gmail.com>
Date: Tue, 28 Oct 2025 11:03:13 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Leon Hwang <leon.hwang@...ux.dev>
Cc: Amery Hung <ameryhung@...il.com>, bpf@...r.kernel.org, ast@...nel.org, 
	andrii@...nel.org, daniel@...earbox.net, martin.lau@...ux.dev, 
	eddyz87@...il.com, song@...nel.org, yonghong.song@...ux.dev, 
	john.fastabend@...il.com, kpsingh@...nel.org, sdf@...ichev.me, 
	haoluo@...gle.com, jolsa@...nel.org, memxor@...il.com, 
	linux-kernel@...r.kernel.org, kernel-patches-bot@...com
Subject: Re: [PATCH bpf v3 3/4] bpf: Free special fields when update local
 storage maps

On Tue, Oct 28, 2025 at 7:48 AM Leon Hwang <leon.hwang@...ux.dev> wrote:
>
>
>
> On 2025/10/28 01:04, Amery Hung wrote:
> > On Mon, Oct 27, 2025 at 9:15 AM Leon Hwang <leon.hwang@...ux.dev> 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@...ux.dev> 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
>