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: <971495da-bc0e-46d4-bda4-5e9b8310ca3e@linux.dev>
Date: Tue, 28 Oct 2025 22:48:39 +0800
From: Leon Hwang <leon.hwang@...ux.dev>
To: Amery Hung <ameryhung@...il.com>
Cc: 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 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.

Thanks,
Leon