Re: [PATCHv6 01/13] bpf: Introduce kfuncs to parser buffer content

[bot+bpf-ci@kernel.org]

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

> diff --git a/kernel/bpf/bpf_buffer_parser.c b/kernel/bpf/bpf_buffer_parser.c
> new file mode 100644
> index 000000000000..6acb4b5da71b
> --- /dev/null
> +++ b/kernel/bpf/bpf_buffer_parser.c

[ ... ]

> +static void release_bpf_parser_context(struct kref *kref)
> +{
> +	struct bpf_parser_context *ctx = container_of(kref, struct bpf_parser_context, ref);
> +
> +	if (!!ctx->buf) {
> +		vfree(ctx->buf->buf);
> +		kfree(ctx->buf);
> +	}

[ ... ]

> +struct bpf_parser_context *alloc_bpf_parser_context(bpf_parser_handler_t func,
> +		void *data)
> +{
> +	struct bpf_parser_context *ctx;
> +	unsigned int key;
> +
> +	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);

ctx->buf is NULL here from kzalloc.

[ ... ]

> +__bpf_kfunc int bpf_buffer_parser(char *buf, int buf_sz,
> +		struct bpf_parser_context *context)
> +{
> +	struct bpf_parser_buf *parser_buf;
> +	int ret;
> +	char *b;
> +
> +	if (unlikely(context->func == NULL))
> +		return -EINVAL;
> +
> +	b = __vmalloc(buf_sz, GFP_KERNEL_ACCOUNT | __GFP_ZERO);
> +	if (!b)
> +		return -ENOMEM;
> +	ret = copy_from_kernel_nofault(b, buf, buf_sz);
> +	if (!!ret) {
> +		vfree(b);
> +		return ret;
> +	}
> +
> +	parser_buf = kmalloc(sizeof(struct bpf_parser_buf), GFP_KERNEL);
> +	if (!parser_buf) {
> +		vfree(b);
> +		return -ENOMEM;
> +	}
> +	parser_buf->buf = b;
> +	parser_buf->size = buf_sz;
> +	context->buf = parser_buf;

If bpf_buffer_parser() is called multiple times on the same context, does
this leak the previous parser_buf and its backing vmalloc buffer? The
assignment to context->buf overwrites any existing pointer without first
freeing it.

release_bpf_parser_context() will only free the final context->buf value
when the context is destroyed, so any intermediate buffers from prior
calls would be lost.

Should there be a check here to either return an error if context->buf is
already set, or free the old buffer before assigning the new one?

> +	ret = context->func(context);
> +
> +	return ret;
> +}

[ ... ]


---
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/21147860407