Re: [PATCH bpf-next v2 2/4] bpf: Cancel special fields on map value recycle

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Replacing `bpf_obj_free_fields()` with `bpf_obj_cancel_fields()` fails to zero or free kernel pointers on map element deletion and update, causing new hash map keys to inherit valid kptrs from deleted elements (cross-key contamination).
--

commit a156fa02499cc2be66e3982937fa823e00008e97
Author: Justin Suess <utilityemal77@gmail.com>

    bpf: Cancel special fields on map value recycle

This commit introduces bpf_obj_cancel_fields() for NMI-safe cleanup of timer,
workqueue, and task_work fields on map value recycle. It replaces
bpf_obj_free_fields() in the array and hashtab update/delete/recycle paths.

> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
> index e6271a2bf6d64..4e8d6fe6999bf 100644
> --- a/kernel/bpf/arraymap.c
> +++ b/kernel/bpf/arraymap.c
[ ... ]
> @@ -392,7 +392,7 @@ static long array_map_update_elem(struct bpf_map *map, void *key, void *value,
>  			copy_map_value_locked(map, val, value, false);
>  		else
>  			copy_map_value(map, val, value);
> -		bpf_obj_free_fields(array->map.record, val);
> +		bpf_obj_cancel_fields(array->map.record, val);
>  	}
>  	return 0;

[Severity: Critical]
Does replacing bpf_obj_free_fields() with bpf_obj_cancel_fields() fail to clear
old kptrs in the array slot?

Since the new bpf_obj_cancel_fields() helper only processes timers and
workqueues, the old kernel pointers (like BPF_KPTR_REF) seem to remain valid
and unzeroed in the slot indefinitely.

[ ... ]
> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index b4366cad3cfa5..a9b2b882b90f2 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c
[ ... ]
> @@ -844,11 +848,11 @@ static void check_and_free_fields(struct bpf_htab *htab,
>  		int cpu;
>  
>  		for_each_possible_cpu(cpu)
> -			bpf_obj_free_fields(htab->map.record, per_cpu_ptr(pptr, cpu));
> +			bpf_obj_cancel_fields(htab->map.record, per_cpu_ptr(pptr, cpu));
>  	} else {
>  		void *map_value = htab_elem_value(elem, htab->map.key_size);
>  
> -		bpf_obj_free_fields(htab->map.record, map_value);
> +		bpf_obj_cancel_fields(htab->map.record, map_value);
>  	}
>  }

[Severity: Critical]
Can this leak kernel pointers between different hash keys?

When a hash map element is deleted, check_and_cancel_fields() is called.
Because it doesn't clear kptrs, the element is returned to the memory cache or
preallocated freelist with its kernel pointers still intact.

When a new hash map element is inserted for a different key, alloc_htab_elem()
reuses this memory. Because copy_map_value() safely skips writing to special
BTF field offsets:

alloc_htab_elem() {
    ...
    copy_map_value(&htab->map, htab_elem_value(l_new, key_size), value);
    ...
}

won't the new element silently inherit the valid kernel pointers from the
previously deleted key?

[ ... ]
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index d4188a992bd8b..8d12aa3728ea9 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
[ ... ]
> +void bpf_obj_cancel_fields(const struct btf_record *rec, void *obj)
> +{
> +	const struct btf_field *fields;
> +	int i;
> +
> +	if (IS_ERR_OR_NULL(rec))
> +		return;
> +	fields = rec->fields;
> +	for (i = 0; i < rec->cnt; i++) {
> +		void *field_ptr = obj + fields[i].offset;
> +
> +		switch (fields[i].type) {
> +		case BPF_TIMER:
> +			bpf_timer_cancel_and_free(field_ptr);
> +			break;
> +		case BPF_WORKQUEUE:
> +			bpf_wq_cancel_and_free(field_ptr);
> +			break;
> +		case BPF_TASK_WORK:
> +			bpf_task_work_cancel_and_free(field_ptr);
> +			break;
> +		default:
> +			break;
> +		}
> +	}
> +}

[Severity: Critical]
Since this new helper handles BPF_TIMER, BPF_WORKQUEUE, and BPF_TASK_WORK,
doesn't it leave fields like BPF_KPTR_REF, BPF_LIST_HEAD, BPF_UPTR and
BPF_RB_ROOT completely intact without zeroing them?

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