adapt 6.6 #1520
adapt 6.6 #1520
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Summary of ChangesHello @lec-bit, I'm Gemini Code Assist1! 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 refactors the Kmesh eBPF and kernel module components to enhance compatibility with newer Linux kernels, particularly version 6.x. The core change involves integrating BPF kfuncs for key operations like header parsing and socket option handling, which allows eBPF programs to directly call kernel functions. This adaptation simplifies the codebase by removing older, kernel-specific conditional logic, such as that for Highlights
Using Gemini Code AssistThe 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
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 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. In kernel's heart, new kfuncs bloom, Old paths now fade, dispelling gloom. With BTF's light, a clearer way, Code adapts, for a brighter day. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adapts the codebase for Linux kernel 6.6, primarily by introducing kfunc support and removing legacy code for openEuler 23.03. The changes are extensive, touching BPF code, kernel modules, and build scripts. While the direction is correct, I've found several critical issues, including merge conflicts in a generated file, incorrect kfunc signatures and implementations that could lead to memory safety issues or kernel instability, and some less severe maintainability problems. Please address the critical issues before merging.
| #if ENHANCED_KERNEL | ||
| #if KERNEL_KFUNC | ||
| extern int bpf_parse_header_msg_func(void *src, int src__sz) __ksym; | ||
| extern int bpf_km_header_strnstr_func(void *ctx, int ctx__sz, const char *key, int key__sz, const char *subptr) __ksym; |
There was a problem hiding this comment.
The signature for bpf_km_header_strnstr_func is missing the subptr__sz parameter. This causes the kfunc implementation to use a hardcoded and likely incorrect size, which can lead to memory safety issues. The signature should include int subptr__sz.
extern int bpf_km_header_strnstr_func(void *ctx, int ctx__sz, const char *key, int key__sz, const char *subptr, int subptr__sz) __ksym;| int bpf_km_header_strnstr(void *ctx, const char *key, int key__sz, const char *subptr, int subptr__sz) | ||
| { | ||
| struct bpf_mem_ptr msg_tmp = {.ptr = ctx, .size = sizeof(struct bpf_sock_addr)}; | ||
| return bpf_km_header_strnstr_func(&msg_tmp, sizeof(struct bpf_mem_ptr), key, key__sz, subptr); |
There was a problem hiding this comment.
The call to bpf_km_header_strnstr_func is missing the subptr__sz argument, which is required for correctness and memory safety. This is related to the incorrect extern declaration on line 28.
return bpf_km_header_strnstr_func(&msg_tmp, sizeof(struct bpf_mem_ptr), key, key__sz, subptr, subptr__sz);| <<<<<<< HEAD | ||
| ======= | ||
| KmConfigmap *ebpf.MapSpec `ebpf:"km_configmap"` | ||
| >>>>>>> 3bfda9b3 (adapt 6.6) |
There was a problem hiding this comment.
This generated file contains unresolved merge conflict markers (e.g., here and at lines 139, 157). This will cause the build to fail. Please resolve the conflicts, likely by regenerating the file.
| <<<<<<< HEAD | ||
| ======= | ||
| KmConfigmap *ebpf.Map `ebpf:"km_configmap"` | ||
| >>>>>>> 3bfda9b3 (adapt 6.6) |
There was a problem hiding this comment.
This file contains unresolved merge conflict markers, which will cause the build to fail. Please resolve the conflict.
| <<<<<<< HEAD | ||
| ======= | ||
| m.KmConfigmap, | ||
| >>>>>>> 3bfda9b3 (adapt 6.6) |
There was a problem hiding this comment.
This file contains unresolved merge conflict markers, which will cause the build to fail. Please resolve the conflict.
| __bpf_kfunc int bpf_km_header_strnstr_func(void *ctx, int ctx__sz, const char *key, int key__sz, const char *subptr) | ||
| { | ||
| struct bpf_sock_addr_kern *sa_kern = ctx; | ||
| int subptr__sz = 5; | ||
| return bpf_km_header_strnstr_impl(ctx, key, key__sz, subptr, subptr__sz); | ||
| } |
There was a problem hiding this comment.
This kfunc implementation is incorrect and doesn't match the intended usage. It's missing the subptr__sz parameter, hardcodes a value for it, and incorrectly handles the ctx pointer. The ctx argument is a pointer to bpf_mem_ptr and needs to be dereferenced to get the actual context. The sa_kern variable is also unused.
__bpf_kfunc int bpf_km_header_strnstr_func(void *bpf_mem, int bpf_mem__sz, const char *key, int key__sz, const char *subptr, int subptr__sz)
{
struct bpf_mem_ptr *tmp = bpf_mem;
struct bpf_sock_addr_kern *ctx = tmp->ptr;
return bpf_km_header_strnstr_impl(ctx, key, key__sz, subptr, subptr__sz);
}| __bpf_kfunc int bpf_getsockopt_func(void *bpf_mem, int bpf_socket__sz, int optname, char *opt, int opt__sz) | ||
| { | ||
| struct bpf_mem_ptr *tmp = bpf_mem; | ||
| struct bpf_sock_addr_kern *ctx = tmp->ptr; | ||
| struct sock *sk = ctx->sk; | ||
|
|
||
| struct inet_connection_sock *icsk = inet_csk(sk); | ||
| struct tcp_sock *tp = tcp_sk(sk); | ||
| struct net *net = sock_net(sk); | ||
| int val, len; | ||
|
|
||
| sockptr_t optval = KERNEL_SOCKPTR(opt); | ||
| sockptr_t optlen = KERNEL_SOCKPTR(&opt__sz); | ||
|
|
||
| if (copy_from_sockptr(&len, optlen, sizeof(int))) | ||
| return -EFAULT; | ||
|
|
||
| if (len < 0) | ||
| return -EINVAL; | ||
|
|
||
| len = min_t(unsigned int, len, TCP_ULP_NAME_MAX); | ||
| if (!icsk->icsk_ulp_ops) { | ||
| len = 0; | ||
| if (copy_to_sockptr(optlen, &len, sizeof(int))) | ||
| return -EFAULT; | ||
| return -EINVAL; | ||
| } | ||
| if (copy_to_sockptr(optlen, &len, sizeof(int))) | ||
| return -EFAULT; | ||
| if (copy_to_sockptr(optval, icsk->icsk_ulp_ops->name, len)) | ||
| return -EFAULT; | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
This kfunc has several issues:
- The variables
tp,net, andvalare declared but not used, which will cause compiler warnings. - The handling of
opt__szis incorrect and unsafe.opt__szis an integer passed by value.KERNEL_SOCKPTR(&opt__sz)creates a sockptr to a kernel stack variable, andcopy_from_sockptris not meant for this. This can lead to undefined behavior or kernel crashes.
To correctly handle the output length, the kfunc signature should be changed to accept a pointer int *opt__sz, and the call site in bpf/include/common.h should be updated to pass a pointer to the length variable.
| int bpf_km_setsockopt(struct bpf_sock_addr *ctx, int level, int optname, const char *optval, int optval__sz) | ||
| { | ||
| const char kmesh_module_name[] = "kmesh_defer"; | ||
| if (level != IPPROTO_TCP || optval__sz != sizeof(kmesh_module_name)) | ||
| return -1; | ||
|
|
||
| struct bpf_mem_ptr msg_tmp = {.ptr = ctx, .size = sizeof(struct bpf_sock_addr)}; | ||
| return bpf_setsockopt_func( | ||
| &msg_tmp, sizeof(struct bpf_mem_ptr), optname, (void *)kmesh_module_name, sizeof(kmesh_module_name)); | ||
| } |
There was a problem hiding this comment.
The function bpf_km_setsockopt has a generic name, but its implementation is highly specific to setting the TCP_ULP option to "kmesh_defer". This is misleading and harms maintainability. Consider renaming the function to reflect its specific purpose, for example, bpf_km_set_tcp_ulp_kmesh_defer.
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the kernel compatibility layer to support kernel function (kfunc) based BPF operations for kernel 6.x+. It removes deprecated OpenEuler 23.03-specific code and introduces a new approach for handling kernel functions based on BTF/kfunc availability.
- Removes OE_23_03 macro and related version-specific code paths
- Introduces KERNEL_KFUNC macro to conditionally use kfunc-based implementations vs function pointer dispatch
- Adds new kmesh_func.c module that registers BPF kernel functions when kfunc is available
- Updates kernel configuration detection to check for BTF support (CONFIG_DEBUG_INFO_BTF_MODULES and CONFIG_DEBUG_INFO_BTF)
Reviewed Changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| kernel/ko_src/kmesh/kmesh_func.c | New file implementing kernel functions as BPF kfuncs or legacy function pointers based on KERNEL_KFUNC macro |
| kernel/ko_src/kmesh/kmesh_func.h | New header declaring init/exit functions for the kmesh function module |
| kernel/ko_src/kmesh/defer_connect.c | Refactored to use conditional macros for defer_connect operations based on KERNEL_KFUNC support |
| kmesh_macros_env_kernel.sh | Added BTF config detection and KERNEL_KFUNC configuration logic |
| config/kmesh_marcos_def.h | Removed OE_23_03 macro, added KERNEL_KFUNC macro definition |
| bpf/include/common.h | Added kfunc declarations and wrapper functions for BPF operations |
| oncn-mda/include/mesh_accelerate.h | Removed OE_23_03 conditional compilation, standardized port handling |
| bpf/kmesh/bpf2go/kernelnative/enhanced/kmeshsockopscompat_bpfel.go | Contains unresolved merge conflict markers |
| Multiple generated .go files | Regenerated BPF program bindings with updated map/program structures |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <<<<<<< HEAD | ||
| ======= | ||
| KmConfigmap *ebpf.MapSpec `ebpf:"km_configmap"` | ||
| >>>>>>> 3bfda9b3 (adapt 6.6) |
There was a problem hiding this comment.
Unresolved merge conflict markers present in the generated code. These conflict markers must be resolved before the code can compile.
| <<<<<<< HEAD | |
| ======= | |
| KmConfigmap *ebpf.MapSpec `ebpf:"km_configmap"` | |
| >>>>>>> 3bfda9b3 (adapt 6.6) | |
| KmConfigmap *ebpf.MapSpec `ebpf:"km_configmap"` |
| <<<<<<< HEAD | ||
| ======= | ||
| KmConfigmap *ebpf.Map `ebpf:"km_configmap"` | ||
| >>>>>>> 3bfda9b3 (adapt 6.6) |
There was a problem hiding this comment.
Unresolved merge conflict markers present in the generated code. These conflict markers must be resolved before the code can compile.
| <<<<<<< HEAD | ||
| ======= | ||
| m.KmConfigmap, | ||
| >>>>>>> 3bfda9b3 (adapt 6.6) |
There was a problem hiding this comment.
Unresolved merge conflict markers present in the generated code. These conflict markers must be resolved before the code can compile.
| __bpf_kfunc int bpf_km_header_strnstr_func(void *ctx, int ctx__sz, const char *key, int key__sz, const char *subptr) | ||
| { | ||
| struct bpf_sock_addr_kern *sa_kern = ctx; | ||
| int subptr__sz = 5; |
There was a problem hiding this comment.
Variable 'sa_kern' is declared but never used. The hardcoded value '5' for 'subptr__sz' should be explained with a comment or defined as a named constant to clarify its purpose.
| __bpf_kfunc int bpf_km_header_strnstr_func(void *ctx, int ctx__sz, const char *key, int key__sz, const char *subptr) | |
| { | |
| struct bpf_sock_addr_kern *sa_kern = ctx; | |
| int subptr__sz = 5; | |
| // Length of the substring to search for in header parsing | |
| #define SUBPTR_SZ 5 | |
| __bpf_kfunc int bpf_km_header_strnstr_func(void *ctx, int ctx__sz, const char *key, int key__sz, const char *subptr) | |
| { | |
| int subptr__sz = SUBPTR_SZ; |
kernel/ko_src/kmesh/kmesh_func.c
Outdated
| struct bpf_mem_ptr *tmp = bpf_mem; | ||
| struct bpf_sock_addr_kern *ctx = tmp->ptr; | ||
| struct sock *sk = ctx->sk; | ||
| int ret = 0; |
There was a problem hiding this comment.
Variable 'ret' is initialized but never used before being overwritten by the return statement. Remove this unused initialization.
| int ret = 0; |
kernel/ko_src/kmesh/kmesh_func.c
Outdated
| struct tcp_sock *tp = tcp_sk(sk); | ||
| struct net *net = sock_net(sk); | ||
| int val, len; |
There was a problem hiding this comment.
Variables 'tp', 'net', and 'val' are declared but never used. Remove these unused variable declarations.
| struct tcp_sock *tp = tcp_sk(sk); | |
| struct net *net = sock_net(sk); | |
| int val, len; | |
| int len; |
| if [ "$(check_config "CONFIG_DEBUG_INFO_BTF_MODULES")" == "y" ] && \ | ||
| [ "$(check_config "CONFIG_DEBUG_INFO_BTF")" == "y" ] && \ | ||
| [ "$VERSION" -ge 6 ]; then | ||
| set_config ENHANCED_KERNEL 1 |
There was a problem hiding this comment.
This logic unconditionally sets ENHANCED_KERNEL to 1 when KERNEL_KFUNC is enabled, potentially overriding the earlier ENHANCED_KERNEL detection at line 62-66 which checks for 'FN(parse_header_msg)'. This could cause issues if BTF is available but the parse_header_msg function is not. Consider checking the existing ENHANCED_KERNEL value or adjusting the logic.
| set_config ENHANCED_KERNEL 1 | |
| # Only set ENHANCED_KERNEL to 1 if not already set to 1 | |
| if ! grep -qE "ENHANCED_KERNEL[ \t]*1" config/kmesh_marcos_def.h; then | |
| set_config ENHANCED_KERNEL 1 | |
| fi |
kmesh_macros_env.sh
Outdated
| if [ "$VERSION" -ge 6 ]; then | ||
| set_config ENHANCED_KERNEL 1 | ||
| set_config KERNEL_KFUNC 1 | ||
| else | ||
| set_config KERNEL_KFUNC 0 | ||
| fi |
There was a problem hiding this comment.
This duplicates the kernel version check from kmesh_macros_env_kernel.sh but lacks the BTF configuration checks. The logic should be consistent between both scripts, or the KERNEL_KFUNC configuration should only be set in one location to avoid conflicts.
lec-bit
force-pushed
the
new-kernel-6.6-1103
branch
5 times, most recently
from
743d680 to
eba42b6
Compare
lec-bit
force-pushed
the
new-kernel-6.6-1103
branch
from
eba42b6 to
9f824eb
Compare
lec-bit
force-pushed
the
new-kernel-6.6-1103
branch
from
9f824eb to
3574d0e
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
lec-bit
force-pushed
the
new-kernel-6.6-1103
branch
from
841c802 to
afbc03c
Compare
| }; | ||
|
|
||
| #if ENHANCED_KERNEL | ||
| #if KERNEL_KFUNC |
| int bpf_km_setsockopt(struct bpf_sock_addr *ctx, int level, int optname, const char *optval, int optval__sz) | ||
| { | ||
| const char kmesh_module_name[] = "kmesh_defer"; | ||
| if (level != IPPROTO_TCP || optval__sz != sizeof(kmesh_module_name)) |
There was a problem hiding this comment.
No need check for this function
|
|
||
| int bpf_km_getsockopt(struct bpf_sock_addr *ctx, int level, int optname, char *optval, int optval__sz) | ||
| { | ||
| if (level != IPPROTO_TCP) { |
There was a problem hiding this comment.
No need check for this function
|
|
||
| #else | ||
| #include <bpf_helper_defs_ext.h> | ||
| #define bpf_km_setsockopt bpf_setsockopt |
There was a problem hiding this comment.
Pls define macros in bpf_helper_defs_ext.h
| #define KMESH_DELAY_ERROR -1000 | ||
|
|
||
| #define BPF_CGROUP_RUN_PROG_INET4_CONNECT_KMESH(sk, uaddr, t_ctx) \ | ||
| #ifdef KERNEL_KFUNC |
There was a problem hiding this comment.
This is a difference introduced by the kernel version. It is inappropriate to use KERNEL_KFUNC to distinguish this difference.
| ubase = iov->iov_base; | ||
| kbuf_size = iov->iov_len; | ||
| } else if (iter_is_iovec(&msg->msg_iter)) { | ||
| #ifdef KERNEL_KFUNC |
There was a problem hiding this comment.
Same as the previous comment.
| }) | ||
|
|
||
| #define SET_FDEFER_CONNECT_ON(sk) (inet_sk(sk)->defer_connect = 1) | ||
| #define SET_FDEFER_CONNECT_OFF(sk) (inet_sk(sk)->defer_connect = 0) |
There was a problem hiding this comment.
SET_DEFER_CONNECT_ON
SET_DEFER_CONNECT_OFF
| __bpf_kfunc int bpf_km_header_strnstr_func(void *ctx, int ctx__sz, const char *key, int key__sz, const char *subptr) | ||
| { | ||
| struct bpf_sock_addr_kern *sa_kern = ctx; | ||
| int subptr__sz = 5; |
3 participants
What type of PR is this?
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: