Kexec bpf v6

[pfliu]

No description provided.

[gemini-code-assist]

Summary of Changes

Hello @pfliu, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the kernel's kexec capabilities by integrating BPF programs for parsing kexec image files, allowing for more flexible and dynamic boot processes. This major feature required extensive additions to the BPF and kexec subsystems, including new configuration options, source files, and build system modifications. In addition to this core functionality, the PR also broadens input device support by incorporating new CRKD controllers, introduces a new input event code for sound profiles, and includes several minor but important fixes across various input drivers and kernel infrastructure. The kernel's release candidate version has also been updated.

Highlights

• Kexec BPF Integration: Introduced Kexec BPF functionality, enabling BPF programs to parse kexec image files. This involves new Kconfig options, build system changes, and new C files (bpf_buffer_parser.c, kexec_bpf_loader.c, kexec_uefi_app.c) to facilitate dynamic image processing during kexec boot.
• New Input Device Support: Added support for several new CRKD controllers (Blueberry Burst Pro, Black Tribal Edition, SG) to the xpad joystick driver, expanding compatibility for gaming peripherals.
• New Input Event Code: Defined a new input event code, ABS_SND_PROFILE, for handling multi-value sound profile switches, along with its documentation and mapping in the HID debug driver.
• DMI Quirk for HONOR Laptops: Added a DMI quirk for 'HONOR FMB-P' laptops to address specific keyboard issues by applying the atkbd_deactivate_fixup.
• Driver Stability Improvements: Implemented fixes in lkkbd, alps, and ti_am335x_tsc drivers, primarily focusing on proper workqueue synchronization during disconnect/error paths and correcting an array bounds check in touchscreen configuration.
• Decompressor Persistence Option: Added CONFIG_KEEP_DECOMPRESSOR to allow decompressor routines to remain in memory after kernel initialization, a necessary dependency for the new Kexec BPF feature.
• Kernel Version Update: The kernel's EXTRAVERSION was incremented from -rc1 to -rc2.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces a significant new feature to kexec, allowing a BPF program embedded in a kexec image to parse the image. This is a powerful mechanism for supporting complex image formats. The implementation is extensive, touching core kexec, BPF infrastructure, and adding new tools.

My review has found several issues, including a build-breaking change, a few bugs that could lead to memory leaks or incorrect behavior, and some issues in the new tooling. These should be addressed to ensure the feature is robust.

• In kernel/Kconfig.kexec, there's a typo in a dependency.
• In kernel/bpf/bpf_buffer_parser.c, the return value of copy_from_kernel_nofault is checked incorrectly.
• In kernel/bpf/helpers.c, a new kfunc is added unconditionally, which will break builds where the feature is disabled.
• In kernel/kexec_bpf_loader.c, there's a memory leak in an error path and a portability issue in a memory allocation calculation.
• The new user-space tool tools/kexec/zboot_image_builder.c has a couple of bugs that will prevent it from compiling or cause it to crash.

Overall, this is a promising feature, but the identified issues need to be fixed.

[gemini-code-assist]

This unconditional BTF_ID_FLAGS for bpf_buffer_parser will cause a linker error when the kernel is built with CONFIG_KEXEC_FILE=n, because the function bpf_buffer_parser is defined in kernel/bpf/bpf_buffer_parser.c which is only compiled when CONFIG_KEXEC_FILE=y. This should be wrapped in an #ifdef to make it conditional on CONFIG_KEXEC_BPF being enabled.

#if IS_ENABLED(CONFIG_KEXEC_BPF)
BTF_ID_FLAGS(func, bpf_buffer_parser, KF_TRUSTED_ARGS | KF_SLEEPABLE)
#endif

[gemini-code-assist]

This line will cause a compilation error. The file descriptor bpf_fd is not declared or opened. You probably meant to close bin_fp, which was opened with fopen(). It should be closed with fclose(bin_fp).

fclose(bin_fp);

[gemini-code-assist]

There appears to be a typo in the dependency. KEEP_COMPRESSOR should be KEEP_DECOMPRESSOR. The new Kconfig option added in lib/Kconfig is KEEP_DECOMPRESSOR.

depends on DEBUG_INFO_BTF && BPF_SYSCALL && KEEP_DECOMPRESSOR

[gemini-code-assist]

The return value of copy_from_kernel_nofault() is the number of bytes that could not be copied (0 on success). Therefore, the check for an error should be if (ret != 0) instead of if (ret < 0), as it does not return negative error codes.

if (ret) {

[gemini-code-assist]

There is a resource leak in this error path. If arm_bpf_prog() fails, the function jumps to err: without releasing the bpf_parser_context allocated earlier. You should call put_bpf_parser_context(bpf) before the goto to prevent the leak.

if (ret) {
				pr_err("Fail to load .bpf section\n");
				put_bpf_parser_context(bpf);
				goto err;
			}

[gemini-code-assist]

base_sections is a pointer into a memory-mapped region (base_start_addr) and was not allocated with malloc/calloc. Calling free(base_sections) will lead to a crash or undefined behavior. This line should be removed.

[gemini-code-assist]

The compilation of bpf_buffer_parser.o is conditioned on CONFIG_KEXEC_FILE. It would be more precise to condition this on CONFIG_KEXEC_BPF, as the functionality in bpf_buffer_parser.c is specific to the kexec BPF feature. While CONFIG_KEXEC_BPF depends on CONFIG_KEXEC_FILE, this change would avoid compiling the file unnecessarily if KEXEC_FILE is enabled but KEXEC_BPF is not.

ifeq ($(CONFIG_KEXEC_BPF),y)
obj-$(CONFIG_BPF_SYSCALL) += bpf_buffer_parser.o
endif

[gemini-code-assist]

The size of a pointer is being calculated using sizeof(unsigned long *). While this may work on many 64-bit systems, it's not portable. The type of chunk_base_addr is char **, so you should use sizeof(char *) or sizeof(void *) for portability.

unsigned long sz = (MAX_UNCOMPRESSED_BUF_SIZE / chunk_size) * sizeof(char *);

[pfliu]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a significant new feature enabling the use of BPF programs to parse kexec images, particularly PE files containing a .bpf section. This allows for more flexible image handling, such as on-the-fly decompression. The changes are extensive, touching the kexec and BPF subsystems, and adding new user-space tooling for image preparation. My review has identified a high-severity issue in the new user-space image builder tool related to an uninitialized variable, which could lead to a crash. I have also provided several medium-severity comments regarding non-idiomatic C code (unnecessary use of the !! operator) that deviates from the Linux kernel coding style, and a minor string formatting issue in a driver file. The overall architecture of this feature appears robust and well-designed.

[gemini-code-assist]

The variable payload_sect_idx is used on line 203 without being initialized if the loop on lines 129-136 does not find a matching section for zheader->payload_offset. This can lead to undefined behavior.

To fix this, you should initialize payload_sect_idx to an invalid value (e.g., (uint32_t)-1), and then add a check after the loop to handle the error case where no section is found. For example:

// after the loop on line 136
if (payload_sect_idx == (uint32_t)-1) {
    fprintf(stderr, "Failed to find section for payload offset 0x%x\n", zheader->payload_offset);
    ret = -1;
    goto err;
}

Also, consider adding a break inside the if block in the loop to make it more efficient.

 uint32_t payload_sect_idx = -1;

[gemini-code-assist]

There is a trailing space in the device name string "CRKD LP Black Tribal Edition (Xbox) ". This should be removed for consistency and to avoid potential issues with string handling in userspace.

 0x0351, 0x2000, "CRKD LP Black Tribal Edition (Xbox)", 0, XTYPE_XBOX360 },

[gemini-code-assist]

The use of double negation !! on a pointer is unnecessary. The Linux kernel coding style prefers a direct check for non-NULL pointers. Using if (ctx->buf) is more idiomatic and readable.

 if (ctx->buf) {

[gemini-code-assist]

The use of double negation !! on an integer return value is unnecessary. The Linux kernel coding style prefers checking against zero explicitly (e.g., if (ret != 0)) or simply if (ret) for error codes where any non-zero value is an error. In this context, if (ret) is sufficient and more idiomatic.

 if (ret) {

[gemini-code-assist]

The use of double negation !! on an integer return value is unnecessary. A direct check if (ret) is sufficient and more idiomatic according to Linux kernel coding style.

 if (ret)

[gemini-code-assist]

The use of double negation !! on a size variable is unnecessary. A direct check if (bpf_sz) is sufficient and more idiomatic.

  if (bpf_sz) {

[gemini-code-assist]

The use of double negation !! on the result of __builtin_memcmp is unnecessary and not idiomatic. A direct comparison with 0 is clearer and preferred: if (__builtin_memcmp(...) != 0).

 if (__builtin_memcmp(&zboot_header->image_type, "zimg",
   sizeof(zboot_header->image_type)) != 0) {