Skip to contentSkip to conversation

bpf: make dmabuf iterator stay drained after NULL

PATCH In review Kang Chen 3 participants 3 messages Nov 10 – Nov 11

Subsystems (3)
lore
Share & tools
lore

Kang Chen

author
#0 lore raw

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.

Fixes 6eab7ac7c5ee ("bpf: Add open coded dmabuf iterator")
---
 kernel/bpf/dmabuf_iter.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)
kernel/bpf/dmabuf_iter.c +22 -1
index cd500248abd9..a2b7c38f9ae6 100644
@@ -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
#1 lore raw
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 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 -0
 	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;
+	}
+

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

#2 lore raw
In reply to Kang Chen

Adding T.J. to comment.

Regards,
Christian.

Quoted 75 lines · Kang Chen

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.

Fixes 6eab7ac7c5ee ("bpf: Add open coded dmabuf iterator")
---
 kernel/bpf/dmabuf_iter.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)
kernel/bpf/dmabuf_iter.c +22 -2
index cd500248abd9..a2b7c38f9ae6 100644
@@ -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