bpf: make dmabuf iterator stay drained after NULL
Subsystems (3)
PATCH 1/1 bpf: make dmabuf iterator stay drained after NULL
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.
Use an invalid pointer value as the drained sentinel so the iterator state keeps its one-pointer ABI, and skip dma_buf_put() for it.
--- kernel/bpf/dmabuf_iter.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)
kernel/bpf/dmabuf_iter.c +22 -1
@@ -156,6 +156,13 @@ struct bpf_iter_dmabuf_kern {struct dma_buf *dmabuf; }__aligned(8) ; +/* +* The iterator state is one pointer wide because struct bpf_iter_dmabuf is +* part of the generated vmlinux.h ABI. Use an invalid pointer value as the +* "drained" sentinel instead of growing the state. +*/ +#define DMABUF_ITER_DONE ((struct dma_buf *)1L) +__bpf_kfunc_start_defs (); __bpf_kfunc int bpf_iter_dmabuf_new(struct bpf_iter_dmabuf *it)
@@ -173,11 +180,25 @@ __bpf_kfunc struct dma_buf *bpf_iter_dmabuf_next(struct bpf_iter_dmabuf *it) {struct bpf_iter_dmabuf_kern *kit = (void *)it; +/* +* The verifier assumes that an iterator which returned NULL keeps +* returning NULL (see process_iter_next_call()). Without this check +* the NULL left by the last dma_buf_iter_next() would be interpreted +* as "not started" and the iteration would restart from the list head. +*/ +if (kit->dmabuf ==DMABUF_ITER_DONE ) +return NULL ; +if (kit->dmabuf) kit->dmabuf =dma_buf_iter_next (kit->dmabuf);else kit->dmabuf =dma_buf_iter_begin (); +if (!kit->dmabuf) { + kit->dmabuf =DMABUF_ITER_DONE ; +return NULL ; + } +return kit->dmabuf; }
@@ -185,7 +206,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_DONE )dma_buf_put (kit->dmabuf); }
Signature · 1 line
2.34.1
[email protected]
AI reviewTL;DR: Nit only: the new comment in bpf_iter_dmabuf_next() describes the pre-patch restart behavior, not the code it sits on; the review suggests rewording it to explain why the sentinel exists.
Quoted 1 line
bpf: make dmabuf iterator stay drained after NULL
TL;DR: Nit only: the new comment in bpf_iter_dmabuf_next() describes the pre-patch restart behavior, not the code it sits on; the review suggests rewording it to explain why the sentinel exists.
Quoted 3 lines
kernel/bpf/dmabuf_iter.c +0
-0
[ ... ]
Quoted 13 lines
kernel/bpf/dmabuf_iter.c +9
-0@@ -173,11 +180,25 @@ __bpf_kfunc struct dma_buf *bpf_iter_dmabuf_next(struct bpf_iter_dmabuf *it) {struct bpf_iter_dmabuf_kern *kit = (void *)it; +/* +* The verifier assumes that an iterator which returned NULL keeps +* returning NULL (see process_iter_next_call()). Without this check +* the NULL left by the last dma_buf_iter_next() would be interpreted +* as "not started" and the iteration would restart from the list head. +*/ +if (kit->dmabuf ==DMABUF_ITER_DONE ) +return NULL ; +
kernel/bpf/dmabuf_iter.c __bpf_kfunc struct dma_buf *bpf_iter_dmabuf_next(struct bpf_iter_dmabuf *it) · line 201
Quoted 10 lines
kernel/bpf/dmabuf_iter.c +5
-0if (kit->dmabuf) kit->dmabuf =dma_buf_iter_next (kit->dmabuf);else kit->dmabuf =dma_buf_iter_begin (); +if (!kit->dmabuf) { + kit->dmabuf =DMABUF_ITER_DONE ; +return NULL ; + } +
This isn't a bug, but is the second sentence of the comment above describing the pre-patch behavior rather than the code it sits on?
With this patch no NULL is ever left in kit->dmabuf, since the block above replaces it with DMABUF_ITER_DONE:
if (!kit->dmabuf) {
kit->dmabuf = DMABUF_ITER_DONE;
return NULL;
}If only the "if (kit->dmabuf == DMABUF_ITER_DONE)" check were removed, the iteration would not restart from the list head. The sentinel would take the "if (kit->dmabuf)" branch and be passed to dma_buf_iter_next((struct dma_buf *)1).
The restart described is the bug being fixed, which is already in the changelog. Could the comment instead say why the sentinel exists, for
example: once begin()/next() returns NULL, park the iterator at DMABUF_ITER_DONE so it is not mistaken for a fresh (NULL) iterator, and keep returning NULL as process_iter_next_call() requires?
---
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
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/42982427559
Adding T.J. to comment.
Regards,
Christian.
Quoted 75 lines · Kang Chen
bpf_iter_dmabuf_next()useskit->dmabuf== NULL to mean "iteration has not started".dma_buf_iter_next()also returns NULL at the end of the list, so anext()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 laternext()calls as the constant zero. A restarted dmabuf iteration therefore hands the BPF program a kernel pointer while the verifier believes R0 == 0.Use an invalid pointer value as the drained sentinel so the iterator state keeps its one-pointer ABI, and skip
dma_buf_put()for it.Fixes 6eab7ac7c5ee ("bpf: Add open coded dmabuf iterator")Signed-off-by Kang Chen <[email protected]>--- kernel/bpf/dmabuf_iter.c | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-)kernel/bpf/dmabuf_iter.c +22
-2@@ -156,6 +156,13 @@ struct bpf_iter_dmabuf_kern {struct dma_buf *dmabuf; }__aligned(8) ; +/* +* The iterator state is one pointer wide because struct bpf_iter_dmabuf is +* part of the generated vmlinux.h ABI. Use an invalid pointer value as the +* "drained" sentinel instead of growing the state. +*/ +#define DMABUF_ITER_DONE ((struct dma_buf *)1L) +__bpf_kfunc_start_defs (); __bpf_kfunc int bpf_iter_dmabuf_new(struct bpf_iter_dmabuf *it)@@ -173,11 +180,25 @@ __bpf_kfunc struct dma_buf *bpf_iter_dmabuf_next(struct bpf_iter_dmabuf *it) {struct bpf_iter_dmabuf_kern *kit = (void *)it; +/* +* The verifier assumes that an iterator which returned NULL keeps +* returning NULL (see process_iter_next_call()). Without this check +* the NULL left by the last dma_buf_iter_next() would be interpreted +* as "not started" and the iteration would restart from the list head. +*/ +if (kit->dmabuf ==DMABUF_ITER_DONE ) +return NULL ; +if (kit->dmabuf) kit->dmabuf =dma_buf_iter_next (kit->dmabuf);else kit->dmabuf =dma_buf_iter_begin (); +if (!kit->dmabuf) { + kit->dmabuf =DMABUF_ITER_DONE ; +return NULL ; + } +return kit->dmabuf; }@@ -185,7 +206,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_DONE )dma_buf_put (kit->dmabuf); } --2.34.1