bpf_iter_dmabuf_next() uses kit->dmabuf == NULL to mean "iteration has not started". dma_buf_iter_next() also returns NULL at the end of the list, so a next() call after exhaustion restarts from the list head. The verifier relies on the iterator contract that once next() returns NULL it keeps returning NULL: process_iter_next_call() marks the iterator DRAINED and treats the result of later next() calls as the constant zero. A restarted dmabuf iteration therefore hands the BPF program a kernel pointer while the verifier believes R0 == 0. NULL now means drained. Initialize the state to DMABUF_ITER_START and translate it to dma_buf_iter_begin() on the first next(). The iterator state is BPF-visible ABI (one pointer wide), so use an invalid pointer sentinel instead of growing it. Fixes: 6eab7ac7c5ee ("bpf: Add open coded dmabuf iterator") Signed-off-by: Kang Chen --- v2: - Use DMABUF_ITER_START; NULL now means drained. - Move the ABI explanation to the commit message. kernel/bpf/dmabuf_iter.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/kernel/bpf/dmabuf_iter.c b/kernel/bpf/dmabuf_iter.c index cd500248abd9..390f6d730a67 100644 --- a/kernel/bpf/dmabuf_iter.c +++ b/kernel/bpf/dmabuf_iter.c @@ -156,6 +156,9 @@ struct bpf_iter_dmabuf_kern { struct dma_buf *dmabuf; } __aligned(8); +/* NULL means drained; START means "not started yet". */ +#define DMABUF_ITER_START ((struct dma_buf *)1L) + __bpf_kfunc_start_defs(); __bpf_kfunc int bpf_iter_dmabuf_new(struct bpf_iter_dmabuf *it) @@ -165,7 +168,7 @@ __bpf_kfunc int bpf_iter_dmabuf_new(struct bpf_iter_dmabuf *it) BUILD_BUG_ON(sizeof(*kit) > sizeof(*it)); BUILD_BUG_ON(__alignof__(*kit) != __alignof__(*it)); - kit->dmabuf = NULL; + kit->dmabuf = DMABUF_ITER_START; return 0; } @@ -173,10 +176,14 @@ __bpf_kfunc struct dma_buf *bpf_iter_dmabuf_next(struct bpf_iter_dmabuf *it) { struct bpf_iter_dmabuf_kern *kit = (void *)it; - if (kit->dmabuf) - kit->dmabuf = dma_buf_iter_next(kit->dmabuf); - else + /* Once NULL is returned, next() has to keep returning NULL. */ + if (!kit->dmabuf) + return NULL; + + if (kit->dmabuf == DMABUF_ITER_START) kit->dmabuf = dma_buf_iter_begin(); + else + kit->dmabuf = dma_buf_iter_next(kit->dmabuf); return kit->dmabuf; } @@ -185,7 +192,7 @@ __bpf_kfunc void bpf_iter_dmabuf_destroy(struct bpf_iter_dmabuf *it) { struct bpf_iter_dmabuf_kern *kit = (void *)it; - if (kit->dmabuf) + if (kit->dmabuf && kit->dmabuf != DMABUF_ITER_START) dma_buf_put(kit->dmabuf); } -- 2.34.1 The BPF iterator docs in bpf_iterators.rst specify that the next function of an open-coded iterator should keep returning NULL after it has been returned once. Add a subtest that checks this for the dmabuf iterator. It drains an iterator, calls bpf_iter_dmabuf_next() once more and checks that the result is NULL. The dma-bufs the test creates keep the list from being empty, so an iterator that starts over would return one of them. The existing open_coded subtest uses bpf_for_each(), which stops at the first NULL and never calls next() on a drained iterator. The selftest was originally posted by T.J. Mercier in the v1 thread: Link: https://lore.kernel.org/bpf/CABdmKX09vLG4U-7FwF-vm_iTy1AjGUPXka08KhWE+qJ0J=XGMA@mail.gmail.com/ Signed-off-by: Kang Chen --- v2: - New in v2; T.J. Mercier's selftest, credited in the commit message. .../selftests/bpf/prog_tests/dmabuf_iter.c | 19 +++++++++++++++++++ .../testing/selftests/bpf/progs/dmabuf_iter.c | 19 +++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/tools/testing/selftests/bpf/prog_tests/dmabuf_iter.c b/tools/testing/selftests/bpf/prog_tests/dmabuf_iter.c index 1a5dc1adae73..c1f81b5dccdc 100644 --- a/tools/testing/selftests/bpf/prog_tests/dmabuf_iter.c +++ b/tools/testing/selftests/bpf/prog_tests/dmabuf_iter.c @@ -265,6 +265,23 @@ static void subtest_dmabuf_iter_check_open_coded(struct dmabuf_iter *skel, int m } while (bpf_map_get_next_key(map_fd, key, key)); } +static void subtest_dmabuf_iter_check_next_after_drain(struct dmabuf_iter *skel) +{ + LIBBPF_OPTS(bpf_test_run_opts, topts); + int err, fd; + + fd = bpf_program__fd(skel->progs.iter_dmabuf_next_after_drain); + + err = bpf_prog_test_run_opts(fd, &topts); + if (!ASSERT_OK(err, "test_run_opts err")) + return; + if (!ASSERT_OK(topts.retval, "test_run_opts retval")) + return; + + /* The test buffers exist, so a restarted iteration would find one */ + ASSERT_EQ(skel->data->next_after_drain, 0, "next_after_drain"); +} + void test_dmabuf_iter(void) { struct dmabuf_iter *skel = NULL; @@ -315,6 +332,8 @@ void test_dmabuf_iter(void) } if (test__start_subtest("open_coded")) subtest_dmabuf_iter_check_open_coded(skel, map_fd); + if (test__start_subtest("next_after_drain")) + subtest_dmabuf_iter_check_next_after_drain(skel); destroy: destroy_test_buffers(); diff --git a/tools/testing/selftests/bpf/progs/dmabuf_iter.c b/tools/testing/selftests/bpf/progs/dmabuf_iter.c index 9cbb7442646e..d5421b75c39c 100644 --- a/tools/testing/selftests/bpf/progs/dmabuf_iter.c +++ b/tools/testing/selftests/bpf/progs/dmabuf_iter.c @@ -99,3 +99,22 @@ int iter_dmabuf_for_each(const void *ctx) return 0; } + +/* Result of a bpf_iter_dmabuf_next() call after the iterator was drained */ +__u64 next_after_drain = -1; + +SEC("syscall") +int iter_dmabuf_next_after_drain(const void *ctx) +{ + struct bpf_iter_dmabuf it; + + bpf_iter_dmabuf_new(&it); + while (bpf_iter_dmabuf_next(&it)) + ; + + /* Store the raw value; the verifier would remove a branch on it */ + next_after_drain = (__u64)bpf_iter_dmabuf_next(&it); + bpf_iter_dmabuf_destroy(&it); + + return 0; +} -- 2.34.1