Re: [PATCH] bpf: make dmabuf iterator stay drained after NULL

[bot+bpf-ci@kernel.org]

[-- Attachment #1: Type: text/plain, Size: 2299 bytes --]

> 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.

> diff --git a/kernel/bpf/dmabuf_iter.c b/kernel/bpf/dmabuf_iter.c
> --- a/kernel/bpf/dmabuf_iter.c
> +++ b/kernel/bpf/dmabuf_iter.c

[ ... ]

> @@ -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;
> +	}
> +

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