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

[sashiko-bot@kernel.org]

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?

>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904104203.345917-1-memxor@gmail.com?part=3