Re: [PATCHv6 06/13] kexec_file: Implement decompress method for parser

[bot+bpf-ci]

> diff --git a/kernel/kexec_bpf_loader.c b/kernel/kexec_bpf_loader.c
> index dc59e1389da9..bd6a47fc53ed 100644
> --- a/kernel/kexec_bpf_loader.c
> +++ b/kernel/kexec_bpf_loader.c

[ ... ]

> +static void decompress_mem_allocator_fini(struct decompress_mem_allocator *a)
> +{
> +	vfree(a->chunk_base_addr);
> +}

Should decompress_mem_allocator_fini() also free the individual chunks stored
in chunk_base_addr[0..next_idx-1]? On the success path, merge_decompressed_data()
frees them, but on error paths (decompressor failure or merge failure), the
chunks appear to be leaked since only the array pointer is freed here.

[ ... ]

> +static int parser_cmd_decompress(char *compressed_data, int image_gz_sz,
> +		char **out_buf, int *out_sz, struct kexec_context *ctx)
> +{
> +	struct decompress_mem_allocator *a = &dcmpr_allocator;
> +	decompress_fn decompressor;
> +	const char *name;
> +	int ret;
> +
> +	decompress_mem_allocator_init(a, CHUNK_SIZE);

Is the return value from decompress_mem_allocator_init() intentionally
ignored? If vmalloc fails inside that function, chunk_base_addr and the
other fields remain uninitialized, and subsequent code will use garbage
values when accessing the allocator.

> +	decompressor = decompress_method(compressed_data, image_gz_sz, &name);
> +	if (!decompressor) {
> +		pr_err("Can not find decompress method\n");
> +		return -1;
> +	}

When decompress_method() returns NULL, should decompress_mem_allocator_fini()
be called before returning? The allocator was initialized above and allocated
both chunk_base_addr and the first chunk, which appear to leak on this path.

> +	pr_debug("Find decompressing method: %s, compressed sz:0x%x\n",
> +			name, image_gz_sz);
> +	ret = decompressor(compressed_data, image_gz_sz, NULL, flush,
> +				NULL, NULL, NULL);
> +	if (!!ret)
> +		goto err;
> +	ret = merge_decompressed_data(a, out_buf, out_sz);
> +
> +err:
> +	decompress_mem_allocator_fini(a);
> +
> +	return ret;
> +}

[ ... ]

> +	switch (cmd->cmd) {
> +	case KEXEC_BPF_CMD_DECOMPRESS:
> +		ret = parser_cmd_decompress(buf, cmd->payload_len, &decompressed_buf,
> +					&decompressed_sz, ctx);
> +		if (!ret) {
> +			switch (cmd->subcmd) {
> +			case KEXEC_BPF_SUBCMD_KERNEL:
> +				vfree(ctx->kernel);
> +				ctx->kernel = decompressed_buf;
> +				ctx->kernel_sz = decompressed_sz;
> +				break;
> +			default:
> +				break;

When decompression succeeds but subcmd is not KEXEC_BPF_SUBCMD_KERNEL (for
example INITRD or CMDLINE), decompressed_buf is allocated by
merge_decompressed_data() but not stored or freed. Is this intentional, or
should decompressed_buf be freed in the default case?

> +			}
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}


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