bpf: Free special fields when update hash and local storage maps

When updating local storage maps with BPF_F_LOCK on the fast path, the special fields were not freed after being replaced. This could cause memory referenced by BPF_KPTR_{REF,PERCPU} fields to be held until the map gets freed.

Similarly, on the other path, the old sdata's special fields were never freed when BPF_F_LOCK was specified, causing the same issue.

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

Fixes 9db44fdd8105 ("bpf: Support kptrs in local storage maps")

Signed-off-by Leon Hwang <[email protected]>

---
 kernel/bpf/bpf_local_storage.c | 2 ++
 1 file changed, 2 insertions(+)

kernel/bpf/bpf_local_storage.c +1 -0

index b931fbceb54da..9f447530f9564 100644
@@ -609,6 +609,7 @@ bpf_local_storage_update(void *owner, struct bpf_local_storage_map *smap,
                if (old_sdata && selem_linked_to_storage_lockless(SELEM(old_sdata))) {
                        copy_map_value_locked(&smap->map, old_sdata->data,
                                              value, false);
+                       bpf_obj_free_fields(smap->map.record, old_sdata->data);
                        return old_sdata;
                }
        }

Quoted 1 line · Leon Hwang

kernel/bpf/bpf_local_storage.c bpf_local_storage_update(void *owner, struct bpf_local_storage_map *smap, · line 648

Quoted 8 lines

kernel/bpf/bpf_local_storage.c +1 -0

@@ -641,6 +642,7 @@ bpf_local_storage_update(void *owner, struct bpf_local_storage_map *smap,
        if (old_sdata && (map_flags & BPF_F_LOCK)) {
                copy_map_value_locked(&smap->map, old_sdata->data, value,
                                      false);
+               bpf_obj_free_fields(smap->map.record, old_sdata->data);
                selem = SELEM(old_sdata);
                goto unlock;
        }

Even with rqspinlock I feel this is a can of worms and recursion issues.

I think it's better to disallow special fields and BPF_F_LOCK combination. We already do that for uptr:

        if ((map_flags & BPF_F_LOCK) &&
btf_record_has_field(map->record, BPF_UPTR))
                return -EOPNOTSUPP;

let's do it for all special types.
So patches 2 and 3 will change to -EOPNOTSUPP.

Quoted 2 lines · Alexei Starovoitov

Quoted 1 line · Leon Hwang

[...]

Quoted 18 lines

kernel/bpf/bpf_local_storage.c bpf_local_storage_update(void *owner, struct bpf_local_storage_map *smap, · line 648

Quoted 8 lines

kernel/bpf/bpf_local_storage.c +1 -0

@@ -641,6 +642,7 @@ bpf_local_storage_update(void *owner, struct bpf_local_storage_map *smap,
        if (old_sdata && (map_flags & BPF_F_LOCK)) {
                copy_map_value_locked(&smap->map, old_sdata->data, value,
                                      false);
+               bpf_obj_free_fields(smap->map.record, old_sdata->data);
                selem = SELEM(old_sdata);
                goto unlock;
        }

Even with rqspinlock I feel this is a can of worms and recursion issues.

I think it's better to disallow special fields and BPF_F_LOCK combination. We already do that for uptr:

        if ((map_flags & BPF_F_LOCK) &&
btf_record_has_field(map->record, BPF_UPTR))
                return -EOPNOTSUPP;

Quoted 3 lines

let's do it for all special types.
So patches 2 and 3 will change to -EOPNOTSUPP.

Do you mean disallowing the combination of BPF_F_LOCK with other special fields (except for BPF_SPIN_LOCK) on the UAPI side — for example, in lookup_elem() and update_elem()?

If so, I'd like to send a separate patch set to implement that after the series
“bpf: Introduce BPF_F_CPU and BPF_F_ALL_CPUS flags for percpu maps” is applied.

After that, we can easily add the check in bpf_map_check_op_flags() for the UAPI side, like this:

static inline int bpf_map_check_op_flags(...)

{
        if ((flags & BPF_F_LOCK) && !btf_record_has_field(map->record,
BPF_SPIN_LOCK))
                return -EINVAL;

        if ((flags & BPF_F_LOCK) && btf_record_has_field(map->record,
~BPF_SPIN_LOCK))
                return -EOPNOTSUPP;
}

Then we can clean up some code, including the bpf_obj_free_fields() calls that follow copy_map_value_locked(), as well as the existing UPTR check.

Add tests to verify that updating hash and local storage maps decrements refcount when BPF_KPTR_REF objects are involved.

The tests perform 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. Probe-read the refcount and verify it is *1*.

Signed-off-by Leon Hwang <[email protected]>

---
  .../bpf/prog_tests/refcounted_kptr.c          | 134 +++++++++++++++++-
  .../selftests/bpf/progs/refcounted_kptr.c     | 129 +++++++++++++++++
  2 files changed, 262 insertions(+), 1 deletion(-)

tools/testing/selftests/bpf/prog_tests/refcounted_kptr.c +29 -1

index d6bd5e16e6372..0ec91ff914af7 100644
@@ -3,7 +3,7 @@
 
  #include <test_progs.h>
  #include <network_helpers.h>
-
+#include "cgroup_helpers.h"
  #include "refcounted_kptr.skel.h"
  #include "refcounted_kptr_fail.skel.h"
 

@@ -44,3 +44,135 @@ void test_refcounted_kptr_wrong_owner(void)
  	ASSERT_OK(opts.retval, "rbtree_wrong_owner_remove_fail_a2 retval");
  	refcounted_kptr__destroy(skel);
  }
+
+static void test_refcnt_leak(struct refcounted_kptr *skel, int key, void *values, size_t values_sz,
+			     u64 flags, struct bpf_map *map, struct bpf_program *prog_leak,
+			     struct bpf_program *prog_check, struct bpf_test_run_opts *opts)
+{
+	int ret, fd;
+
+	ret = bpf_map__update_elem(map, &key, sizeof(key), values, values_sz, flags);
+	if (!ASSERT_OK(ret, "bpf_map__update_elem init"))
+		return;
+
+	fd = bpf_program__fd(prog_leak);
+	ret = bpf_prog_test_run_opts(fd, opts);
+	if (!ASSERT_OK(ret, "bpf_prog_test_run_opts"))
+		return;
+	if (!ASSERT_EQ(skel->bss->kptr_refcount, 2, "refcount"))
+		return;
+
+	ret = bpf_map__update_elem(map, &key, sizeof(key), values, values_sz, flags);
+	if (!ASSERT_OK(ret, "bpf_map__update_elem dec refcount"))
+		return;
+
+	fd = bpf_program__fd(prog_check);
+	ret = bpf_prog_test_run_opts(fd, opts);
+	ASSERT_OK(ret, "bpf_prog_test_run_opts");
+	ASSERT_EQ(skel->bss->kptr_refcount, 1, "refcount");
+}
+

Quoted 13 lines · Leon Hwang

Add tests to verify that updating hash and local storage maps decrements refcount when BPF_KPTR_REF objects are involved.

The tests perform 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. Probe-read the refcount and verify it is *1*.

Quoted 1 line

Signed-off-by Leon Hwang <[email protected]>

I applied this patch only (i.e., not including patches 1/2/3) to master branch and do bpf selftest and all tests succeeded.

[root@arch-fb-vm1 bpf]# ./test_progs -t refcounted_kptr #294/1   refcounted_kptr/insert_read_both: remove from tree + list:OK
...
#294/18  refcounted_kptr/pcpu_hash_refcount_leak:OK #294/19  refcounted_kptr/check_pcpu_hash_refcount:OK #294/20  refcounted_kptr/hash_lock_refcount_leak:OK #294/21  refcounted_kptr/check_hash_lock_refcount:OK #294/22  refcounted_kptr/rbtree_sleepable_rcu:OK
#294/23  refcounted_kptr/rbtree_sleepable_rcu_no_explicit_rcu_lock:OK #294/24  refcounted_kptr/cgroup_storage_lock_refcount_leak:OK #294/25  refcounted_kptr/check_cgroup_storage_lock_refcount:OK
...

Did I miss anything?

Quoted 32 lines · Yonghong Song

Quoted 13 lines · Leon Hwang

Add tests to verify that updating hash and local storage maps decrements refcount when BPF_KPTR_REF objects are involved.

The tests perform 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. Probe-read the refcount and verify it is *1*.

Quoted 1 line

Signed-off-by Leon Hwang <[email protected]>

I applied this patch only (i.e., not including patches 1/2/3) to master branch and do bpf selftest and all tests succeeded.

[root@arch-fb-vm1 bpf]# ./test_progs -t refcounted_kptr #294/1   refcounted_kptr/insert_read_both: remove from tree + list:OK
...
#294/18  refcounted_kptr/pcpu_hash_refcount_leak:OK #294/19  refcounted_kptr/check_pcpu_hash_refcount:OK #294/20  refcounted_kptr/hash_lock_refcount_leak:OK #294/21  refcounted_kptr/check_hash_lock_refcount:OK #294/22  refcounted_kptr/rbtree_sleepable_rcu:OK
#294/23  refcounted_kptr/rbtree_sleepable_rcu_no_explicit_rcu_lock:OK #294/24  refcounted_kptr/cgroup_storage_lock_refcount_leak:OK #294/25  refcounted_kptr/check_cgroup_storage_lock_refcount:OK
...

Quoted 2 lines

Did I miss anything?

Oops.

You should run:
./test_progs -t kptr_refcount

The results are as follows:

test_percpu_hash_refcount_leak:PASS:libbpf_num_possible_cpus 0 nsec test_percpu_hash_refcount_leak:PASS:calloc values 0 nsec test_percpu_hash_refcount_leak:PASS:refcounted_kptr__open_and_load 0 nsec test_refcnt_leak:PASS:bpf_map__update_elem init 0 nsec test_refcnt_leak:PASS:bpf_prog_test_run_opts 0 nsec test_refcnt_leak:PASS:refcount 0 nsec test_refcnt_leak:PASS:bpf_map__update_elem dec refcount 0 nsec test_refcnt_leak:PASS:bpf_prog_test_run_opts 0 nsec test_refcnt_leak:FAIL:refcount unexpected refcount: actual 2 != expected 1
#158/1 kptr_refcount_leak/percpu_hash_refcount_leak:FAIL
test_hash_lock_refcount_leak:PASS:refcounted_kptr__open_and_load 0 nsec test_refcnt_leak:PASS:bpf_map__update_elem init 0 nsec test_refcnt_leak:PASS:bpf_prog_test_run_opts 0 nsec test_refcnt_leak:PASS:refcount 0 nsec test_refcnt_leak:PASS:bpf_map__update_elem dec refcount 0 nsec test_refcnt_leak:PASS:bpf_prog_test_run_opts 0 nsec test_refcnt_leak:FAIL:refcount unexpected refcount: actual 2 != expected 1
#158/2 kptr_refcount_leak/hash_lock_refcount_leak:FAIL
test_cgroup_storage_lock_refcount_leak:PASS:setup_cgroup_environment 0 nsec test_cgroup_storage_lock_refcount_leak:PASS:get_root_cgroup 0 nsec test_cgroup_storage_lock_refcount_leak:PASS:refcounted_kptr__open_and_load 0 nsec
test_refcnt_leak:PASS:bpf_map__update_elem init 0 nsec test_refcnt_leak:PASS:bpf_prog_test_run_opts 0 nsec test_refcnt_leak:PASS:refcount 0 nsec test_refcnt_leak:PASS:bpf_map__update_elem dec refcount 0 nsec test_refcnt_leak:PASS:bpf_prog_test_run_opts 0 nsec test_refcnt_leak:FAIL:refcount unexpected refcount: actual 2 != expected 1
#158/3 kptr_refcount_leak/cgroup_storage_lock_refcount_leak:FAIL
#158 kptr_refcount_leak:FAIL

All error logs:
test_percpu_hash_refcount_leak:PASS:libbpf_num_possible_cpus 0 nsec test_percpu_hash_refcount_leak:PASS:calloc values 0 nsec test_percpu_hash_refcount_leak:PASS:refcounted_kptr__open_and_load 0 nsec test_refcnt_leak:PASS:bpf_map__update_elem init 0 nsec test_refcnt_leak:PASS:bpf_prog_test_run_opts 0 nsec test_refcnt_leak:PASS:refcount 0 nsec test_refcnt_leak:PASS:bpf_map__update_elem dec refcount 0 nsec test_refcnt_leak:PASS:bpf_prog_test_run_opts 0 nsec test_refcnt_leak:FAIL:refcount unexpected refcount: actual 2 != expected 1
#158/1 kptr_refcount_leak/percpu_hash_refcount_leak:FAIL
test_hash_lock_refcount_leak:PASS:refcounted_kptr__open_and_load 0 nsec test_refcnt_leak:PASS:bpf_map__update_elem init 0 nsec test_refcnt_leak:PASS:bpf_prog_test_run_opts 0 nsec test_refcnt_leak:PASS:refcount 0 nsec test_refcnt_leak:PASS:bpf_map__update_elem dec refcount 0 nsec test_refcnt_leak:PASS:bpf_prog_test_run_opts 0 nsec test_refcnt_leak:FAIL:refcount unexpected refcount: actual 2 != expected 1
#158/2 kptr_refcount_leak/hash_lock_refcount_leak:FAIL
test_cgroup_storage_lock_refcount_leak:PASS:setup_cgroup_environment 0 nsec test_cgroup_storage_lock_refcount_leak:PASS:get_root_cgroup 0 nsec test_cgroup_storage_lock_refcount_leak:PASS:refcounted_kptr__open_and_load 0 nsec
test_refcnt_leak:PASS:bpf_map__update_elem init 0 nsec test_refcnt_leak:PASS:bpf_prog_test_run_opts 0 nsec test_refcnt_leak:PASS:refcount 0 nsec test_refcnt_leak:PASS:bpf_map__update_elem dec refcount 0 nsec test_refcnt_leak:PASS:bpf_prog_test_run_opts 0 nsec test_refcnt_leak:FAIL:refcount unexpected refcount: actual 2 != expected 1
#158/3 kptr_refcount_leak/cgroup_storage_lock_refcount_leak:FAIL
#158 kptr_refcount_leak:FAIL
Summary: 0/0 PASSED, 0 SKIPPED, 1 FAILED

All three tests failed because the refcount remained 2 instead of decreasing to 1 after the second update_elem() call.

The CI result [1] also demonstrates this issue.

Sorry for the misleading test name earlier.

Links:
[1] https://github.com/kernel-patches/bpf/pull/10203

Thanks,
Leon

[...]