Skip to content

Conversation

kernel-patches-daemon-bpf[bot]
Copy link

Pull request for series with
subject: bpf: Fix possible memleak when updating hash maps
version: 1
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=1012411

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 5fb750e
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1012411
version: 1

@kernel-patches-review-bot
Copy link

commit c50bee1f62c487dddb09bd7cb2328c082c0c7244
Author: Leon Hwang <leon.hwang@linux.dev>

bpf: Fix possible memleak when updating hash maps with BPF_F_LOCK

This patch fixes a memory leak in hash map updates with BPF_F_LOCK by
adding check_and_free_fields() calls after copy_map_value_locked() to
properly free old kptr field values that were replaced.

> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index 26308adc9..65009ea3e 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c
> @@ -1124,6 +1124,7 @@ static long htab_map_update_elem(struct bpf_map *map, void *key, void *value,
>  			copy_map_value_locked(map,
>  					      htab_elem_value(l_old, key_size),
>  					      value, false);
> +			check_and_free_fields(htab, l_old);
>  			return 0;
>  		}
>  		/* fall through, grab the bucket lock and lookup again.
> @@ -1152,6 +1153,7 @@ static long htab_map_update_elem(struct bpf_map *map, void *key, void *value,
>  		copy_map_value_locked(map,
>  				      htab_elem_value(l_old, key_size),
>  				      value, false);
> +		check_and_free_fields(htab, l_old);
>  		ret = 0;
>  		goto err;
>  	}

The fix looks correct for hash maps. However, does the same issue exist
in other map types that use copy_map_value_locked() with BPF_F_LOCK?

In bpf_local_storage_update() at kernel/bpf/bpf_local_storage.c, there
are two code paths that perform in-place updates with BPF_F_LOCK:

1. Fast path without bucket lock (around line 610):

	if (old_sdata && selem_linked_to_storage_lockless(SELEM(old_sdata))) {
		copy_map_value_locked(&smap->map, old_sdata->data,
				      value, false);
		return old_sdata;
	}

Can the old kptr fields leak here when copy_map_value_locked() replaces
them without calling bpf_obj_free_fields() on old_sdata->data?

2. Slow path with bucket lock held (around line 642):

	if (old_sdata && (map_flags & BPF_F_LOCK)) {
		copy_map_value_locked(&smap->map, old_sdata->data, value,
				      false);
		selem = SELEM(old_sdata);
		goto unlock;
	}

Does this path also leak the old kptr fields in old_sdata->data?

Note that other cleanup paths in bpf_local_storage.c do call
bpf_obj_free_fields() properly (lines 217, 240, 252), which suggests
these in-place update paths should too.

Similarly, in cgroup_storage_update_elem() at kernel/bpf/local_storage.c
around line 163:

	if (flags & BPF_F_LOCK) {
		copy_map_value_locked(map, storage->buf->data, value, false);
		return 0;
	}

Can the old kptr fields in storage->buf->data leak here as well?

For reference, arraymap.c already has the correct pattern at line 391
where it calls bpf_obj_free_fields() after copy_map_value_locked().



AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

In-Reply-To-Subject: bpf: Fix possible memleak when updating hash maps with BPF_F_LOCK
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/18565916076

@kernel-patches-daemon-bpf
Copy link
Author

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 0c1999e
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1012411
version: 1

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 8adc470
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1012411
version: 1

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: f6fddc6
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1012411
version: 1

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: f6fddc6
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1012411
version: 1

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: a1e83d4
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1012411
version: 1

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: a1e83d4
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1012411
version: 1

As [lru_,]percpu_hash maps support BPF_KPTR_{REF,PERCPU}, missing
calls to 'bpf_obj_free_fields()' in 'pcpu_copy_value()' can leak memory
referenced by BPF_KPTR_{REF,PERCPU} fields.

Fix this by calling 'bpf_obj_free_fields()' after
'copy_map_value[,_long]()' in 'pcpu_copy_value()'.

Fixes: 65334e6 ("bpf: Support kptrs in percpu hashmap and percpu LRU hashmap")
Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
When updating hash maps with BPF_F_LOCK, the special fields were not
freed after being replaced. This could cause memory referenced by
BPF_KPTR_{REF,PERCPU} fields to leak.

Fix this by calling 'check_and_free_fields()' after
'copy_map_value_locked()' to properly release the old fields.

Fixes: 14a324f ("bpf: Wire up freeing of referenced kptr")
Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
Add two tests to verify that updating hash maps does not leak memory
when BPF_KPTR_REF objects are involved.

The test performs the following steps:

1. Call update_elem() to insert an initial value.
2. Use bpf_refcount_acquire() to increment the refcount.
3. Store the node pointer in the map value.
4. Add the node to a linked list.
5. Probe-read the refcount and verify it is *2*.
6. Call update_elem() again to trigger refcount decrement.
7. Verify that the field has been reset.

Signed-off-by: Leon Hwang <leon.hwang@linux.dev>
@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 1c64efc
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1012411
version: 1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant