Re: [PATCH bpf v1 3/8] bpf: Cancel special fields when recycling rhtab elements

["Kumar Kartikeya Dwivedi" <memxor@gmail.com>]

On Fri Sep 4, 2026 at 1:37 PM CEST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Replacing bpf_obj_free_fields() with bpf_obj_cancel_fields() during map updates and deletions violates BTF-001 and causes cross-key resource leakage.
> --
>
> commit 87b60bb947c49832ff2d2552730fd3c5124e6130
> Author: Nuoqi Gui <gnq25@mails.tsinghua.edu.cn>
>
> bpf: Cancel special fields when recycling rhtab elements
>
> This commit modifies rhtab map operations to use bpf_obj_cancel_fields()
> instead of bpf_obj_free_fields() when replacing or deleting elements. This is
> done to avoid releasing referenced kptrs and other complex fields in NMI
> context, aligning with array and hash map behavior.
>
>> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
>> index 527cc5716ee8c..cc60e99ffbe92 100644
>> --- a/kernel/bpf/hashtab.c
>> +++ b/kernel/bpf/hashtab.c
>
> [ ... ]
>
>> @@ -2967,8 +2957,8 @@ static int rhtab_delete_elem(struct bpf_rhtab *rhtab, struct rhtab_elem *elem, v
>>  		rhtab_read_elem_value(&rhtab->map, copy, elem, flags);
>>  		check_and_init_map_value(&rhtab->map, copy);
>>  	}
>> -	/* Release internal structs: kptr, bpf_timer, task_work, wq */
>> -	rhtab_check_and_free_fields(rhtab, elem);
>> +	bpf_obj_cancel_fields(&rhtab->map,
>> +			      rhtab_elem_value(elem, rhtab->map.key_size));
>>  	bpf_mem_cache_free_rcu(&rhtab->ma, elem);
>
> [Severity: High]
> Does changing this to bpf_obj_cancel_fields() leave stale special fields in
> the memory cache?
>
> Since bpf_obj_cancel_fields() does not drop kptrs or list heads, these
> references appear to remain in the element when it is freed to the cache.
> When a new element is later allocated from the cache in
> rhtab_map_update_elem():
>
>     elem = bpf_mem_cache_alloc(&rhtab->ma);
>     if (!elem)
>         return -ENOMEM;
>     memcpy(elem->data, key, map->key_size);
>     copy_map_value(map, rhtab_elem_value(elem, map->key_size), value);
>
> Because bpf_mem_cache_alloc() does not zero memory and copy_map_value()
> skips special fields, will this cause the new key to inherit the old key's
> populated list head or kptr?
>
>>  	return 0;
>>  }
>
> [ ... ]
>
>> @@ -3029,7 +3018,7 @@ static long rhtab_map_update_existing(struct bpf_map *map, struct rhtab_elem *el
>>  	 * kptrs/etc. still sit in the slot. Cancel them after the copy
>>  	 * to match arraymap's update semantics.
>>  	 */
>> -	rhtab_check_and_free_fields(rhtab, elem);
>> +	bpf_obj_cancel_fields(map, old_val);
>
> [Severity: High]
> Does replacing the free operation with bpf_obj_cancel_fields() here violate
> the requirement to clean up special BTF fields during map updates?
>
> When overwriting an existing map value in-place, failing to free existing
> kptrs or list heads seems like it causes them to outlive the update. Will
> this leave stale resources bound to the key indefinitely?
>

Both are deliberate changes.

>>  	return 0;
>>  }