diff mbox series

[bpf-next] selftests/bpf: Fix compile error when MPTCP not support

Message ID 20241030100108.2443371-1-chen.dylane@gmail.com (mailing list archive)
State Changes Requested
Delegated to: BPF
Headers show
Series [bpf-next] selftests/bpf: Fix compile error when MPTCP not support | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-1 success Logs for ShellCheck
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Validate matrix.py
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-4 success Logs for aarch64-gcc / build / build for aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-17 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-18 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-6 success Logs for aarch64-gcc / test (test_maps, false, 360) / test_maps on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for bpf-next
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 5 this patch: 5
netdev/build_tools success Errors and warnings before: 0 (+0) this patch: 0 (+0)
netdev/cc_maintainers warning 9 maintainers not CCed: shuah@kernel.org sdf@fomichev.me kpsingh@kernel.org netdev@vger.kernel.org john.fastabend@gmail.com eddyz87@gmail.com song@kernel.org haoluo@google.com mykolal@fb.com
netdev/build_clang success Errors and warnings before: 3 this patch: 3
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 3 this patch: 3
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 80 lines checked
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Tao Chen Oct. 30, 2024, 10:01 a.m. UTC 
Fix compile error when MPTCP feature not support, though eBPF core check
already done which seems invalid in this situation, the error info like:
progs/mptcp_sock.c:49:40: error: no member named 'is_mptcp' in 'struct
tcp_sock'
   49 |         is_mptcp = bpf_core_field_exists(tsk->is_mptcp) ?

The filed created in new definitions with eBPF core feature to solve
this build problem, and test case result still ok in MPTCP kernel.

176/1   mptcp/base:OK
176/2   mptcp/mptcpify:OK
176     mptcp:OK
Summary: 1/2 PASSED, 0 SKIPPED, 0 FAILED

Fixes: 8039d353217c ("selftests/bpf: Add MPTCP test base")
Signed-off-by: Tao Chen <chen.dylane@gmail.com>
---
 .../testing/selftests/bpf/progs/mptcp_sock.c  | 42 ++++++++++++++-----
 1 file changed, 32 insertions(+), 10 deletions(-)

Comments

Matthieu Baerts Oct. 30, 2024, 10:49 a.m. UTC | #1 
Hi Tao Chen,

Thank you for having shared this patch.

On 30/10/2024 11:01, Tao Chen wrote:
> Fix compile error when MPTCP feature not support, though eBPF core check
> already done which seems invalid in this situation, the error info like:
> progs/mptcp_sock.c:49:40: error: no member named 'is_mptcp' in 'struct
> tcp_sock'
>    49 |         is_mptcp = bpf_core_field_exists(tsk->is_mptcp) ?
> 
> The filed created in new definitions with eBPF core feature to solve
> this build problem, and test case result still ok in MPTCP kernel.
> 
> 176/1   mptcp/base:OK
> 176/2   mptcp/mptcpify:OK
> 176     mptcp:OK
> Summary: 1/2 PASSED, 0 SKIPPED, 0 FAILED
> 
> Fixes: 8039d353217c ("selftests/bpf: Add MPTCP test base")

The commit you mentioned here is more than 2 years old, and as far as I
can see, nobody else reported this compilation issue. I guess that's
because people used tools/testing/selftests/bpf/config file as expected
to populate the kernel config, and I suppose you didn't, right?

I don't think other BPF selftests check for missing kernel config if
they are specified in the 'config' file, but even if it is the case, I
think it would be better to skip all the MPTCP tests, and not try to
have them checking something that doesn't exist: no need to validate
these tests if the expected kernel config has not been enabled.

But again, please correct me if I'm wrong, but I don't think there is
anything to change here to fix your compilation issue: simply make sure
to use this tools/testing/selftests/bpf/config file to generate your
kernel config, no?

Cheers,
Matt
Tao Chen Oct. 30, 2024, 11:12 a.m. UTC | #2 
在 2024/10/30 18:49, Matthieu Baerts 写道:
> Hi Tao Chen,
> 
> Thank you for having shared this patch.
> 
> On 30/10/2024 11:01, Tao Chen wrote:
>> Fix compile error when MPTCP feature not support, though eBPF core check
>> already done which seems invalid in this situation, the error info like:
>> progs/mptcp_sock.c:49:40: error: no member named 'is_mptcp' in 'struct
>> tcp_sock'
>>     49 |         is_mptcp = bpf_core_field_exists(tsk->is_mptcp) ?
>>
>> The filed created in new definitions with eBPF core feature to solve
>> this build problem, and test case result still ok in MPTCP kernel.
>>
>> 176/1   mptcp/base:OK
>> 176/2   mptcp/mptcpify:OK
>> 176     mptcp:OK
>> Summary: 1/2 PASSED, 0 SKIPPED, 0 FAILED
>>
>> Fixes: 8039d353217c ("selftests/bpf: Add MPTCP test base")
> 
> The commit you mentioned here is more than 2 years old, and as far as I
> can see, nobody else reported this compilation issue. I guess that's
> because people used tools/testing/selftests/bpf/config file as expected
> to populate the kernel config, and I suppose you didn't, right?
>

Hi Matt, thank you for your reply, as you said, i did not use 
tools/testing/selftests/bpf/config to compile kernel, i will use this 
helpful
feature.

> I don't think other BPF selftests check for missing kernel config if
> they are specified in the 'config' file, but even if it is the case, I
> think it would be better to skip all the MPTCP tests, and not try to
> have them checking something that doesn't exist: no need to validate
> these tests if the expected kernel config has not been enabled.
> 

If i use the kernel not support MPTCP, the compile error still exists, 
and i can not build the bpf test successfully. Maybe skill the test case 
seems better when kernel not support. Now that bpf_core_field_exists 
check already used in the code, i think it is better to use new 
definition mode.

> But again, please correct me if I'm wrong, but I don't think there is
> anything to change here to fix your compilation issue: simply make sure
> to use this tools/testing/selftests/bpf/config file to generate your
> kernel config, no?
> 
> Cheers,
> Matt
Matthieu Baerts Oct. 30, 2024, 4:31 p.m. UTC | #3 
Hi Tao, BPF maintainers,

On 30/10/2024 12:12, Tao Chen wrote:
> 在 2024/10/30 18:49, Matthieu Baerts 写道:
>> Hi Tao Chen,
>>
>> Thank you for having shared this patch.
>>
>> On 30/10/2024 11:01, Tao Chen wrote:
>>> Fix compile error when MPTCP feature not support, though eBPF core check
>>> already done which seems invalid in this situation, the error info like:
>>> progs/mptcp_sock.c:49:40: error: no member named 'is_mptcp' in 'struct
>>> tcp_sock'
>>>     49 |         is_mptcp = bpf_core_field_exists(tsk->is_mptcp) ?
>>>
>>> The filed created in new definitions with eBPF core feature to solve
>>> this build problem, and test case result still ok in MPTCP kernel.
>>>
>>> 176/1   mptcp/base:OK
>>> 176/2   mptcp/mptcpify:OK
>>> 176     mptcp:OK
>>> Summary: 1/2 PASSED, 0 SKIPPED, 0 FAILED
>>>
>>> Fixes: 8039d353217c ("selftests/bpf: Add MPTCP test base")
>>
>> The commit you mentioned here is more than 2 years old, and as far as I
>> can see, nobody else reported this compilation issue. I guess that's
>> because people used tools/testing/selftests/bpf/config file as expected
>> to populate the kernel config, and I suppose you didn't, right?
>>
> 
> Hi Matt, thank you for your reply, as you said, i did not use tools/
> testing/selftests/bpf/config to compile kernel, i will use this helpful
> feature.
> 
>> I don't think other BPF selftests check for missing kernel config if
>> they are specified in the 'config' file, but even if it is the case, I
>> think it would be better to skip all the MPTCP tests, and not try to
>> have them checking something that doesn't exist: no need to validate
>> these tests if the expected kernel config has not been enabled.
>>
> 
> If i use the kernel not support MPTCP, the compile error still exists,
> and i can not build the bpf test successfully. Maybe skill the test case
> seems better when kernel not support. Now that bpf_core_field_exists
> check already used in the code, i think it is better to use new
> definition mode.

I understand it would be better, but it means more code to maintain to
handle that (and remembering that in future test cases). If that's not
necessary, then no need to do the effort.

@BPF maintainers: do we need to support kernels not respecting the
tools/testing/selftests/bpf/config file? Should we detect when a
required kernel config is not set and skip some tests?

>> But again, please correct me if I'm wrong, but I don't think there is
>> anything to change here to fix your compilation issue: simply make sure
>> to use this tools/testing/selftests/bpf/config file to generate your
>> kernel config, no?

Cheers,
Matt
Martin KaFai Lau Oct. 30, 2024, 8:48 p.m. UTC | #4 
On 10/30/24 9:31 AM, Matthieu Baerts wrote:
> Hi Tao, BPF maintainers,
> 
> On 30/10/2024 12:12, Tao Chen wrote:
>> 在 2024/10/30 18:49, Matthieu Baerts 写道:
>>> Hi Tao Chen,
>>>
>>> Thank you for having shared this patch.
>>>
>>> On 30/10/2024 11:01, Tao Chen wrote:
>>>> Fix compile error when MPTCP feature not support, though eBPF core check
>>>> already done which seems invalid in this situation, the error info like:
>>>> progs/mptcp_sock.c:49:40: error: no member named 'is_mptcp' in 'struct
>>>> tcp_sock'
>>>>      49 |         is_mptcp = bpf_core_field_exists(tsk->is_mptcp) ?
>>>>
>>>> The filed created in new definitions with eBPF core feature to solve
>>>> this build problem, and test case result still ok in MPTCP kernel.
>>>>
>>>> 176/1   mptcp/base:OK
>>>> 176/2   mptcp/mptcpify:OK
>>>> 176     mptcp:OK
>>>> Summary: 1/2 PASSED, 0 SKIPPED, 0 FAILED
>>>>
>>>> Fixes: 8039d353217c ("selftests/bpf: Add MPTCP test base")
>>>
>>> The commit you mentioned here is more than 2 years old, and as far as I
>>> can see, nobody else reported this compilation issue. I guess that's
>>> because people used tools/testing/selftests/bpf/config file as expected
>>> to populate the kernel config, and I suppose you didn't, right?
>>>
>>
>> Hi Matt, thank you for your reply, as you said, i did not use tools/
>> testing/selftests/bpf/config to compile kernel, i will use this helpful
>> feature.
>>
>>> I don't think other BPF selftests check for missing kernel config if
>>> they are specified in the 'config' file, but even if it is the case, I
>>> think it would be better to skip all the MPTCP tests, and not try to
>>> have them checking something that doesn't exist: no need to validate
>>> these tests if the expected kernel config has not been enabled.
>>>
>>
>> If i use the kernel not support MPTCP, the compile error still exists,
>> and i can not build the bpf test successfully. Maybe skill the test case
>> seems better when kernel not support. Now that bpf_core_field_exists
>> check already used in the code, i think it is better to use new
>> definition mode.
> 
> I understand it would be better, but it means more code to maintain to
> handle that (and remembering that in future test cases). If that's not
> necessary, then no need to do the effort.
> 
> @BPF maintainers: do we need to support kernels not respecting the
> tools/testing/selftests/bpf/config file? Should we detect when a
> required kernel config is not set and skip some tests?

I guess it depends on the CONFIG_. Otherwise, it takes out the goodies of using 
<vmlinux.h> when writing bpf selftests.

If fixing the config is an option and sounds like it is for Tao, then it is 
always good to run everything in test_progs.

There are some "___local" definitions in the selftests. If mptcp test wants to 
go this path, then Matt's request to at least test__skip() makes sense to me.

pw-bot: cr

> 
>>> But again, please correct me if I'm wrong, but I don't think there is
>>> anything to change here to fix your compilation issue: simply make sure
>>> to use this tools/testing/selftests/bpf/config file to generate your
>>> kernel config, no?
> 
> Cheers,
> Matt
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/progs/mptcp_sock.c b/tools/testing/selftests/bpf/progs/mptcp_sock.c
index f3acb90588c7..2f80d042686a 100644
--- a/tools/testing/selftests/bpf/progs/mptcp_sock.c
+++ b/tools/testing/selftests/bpf/progs/mptcp_sock.c
@@ -25,13 +25,23 @@  struct {
 	__type(value, struct mptcp_storage);
 } socket_storage_map SEC(".maps");
 
+struct tcp_sock___new {
+	bool is_mptcp;
+} __attribute__((preserve_access_index));
+
+struct mptcp_sock___new {
+	__u32 token;
+	struct sock *first;
+	char ca_name[TCP_CA_NAME_MAX];
+} __attribute__((preserve_access_index));
+
 SEC("sockops")
 int _sockops(struct bpf_sock_ops *ctx)
 {
 	struct mptcp_storage *storage;
-	struct mptcp_sock *msk;
+	struct mptcp_sock___new *msk;
 	int op = (int)ctx->op;
-	struct tcp_sock *tsk;
+	struct tcp_sock___new *tsk;
 	struct bpf_sock *sk;
 	bool is_mptcp;
 
@@ -42,11 +52,16 @@  int _sockops(struct bpf_sock_ops *ctx)
 	if (!sk)
 		return 1;
 
-	tsk = bpf_skc_to_tcp_sock(sk);
+	/* recast pointer to capture new type for compiler */
+	tsk = (void *)bpf_skc_to_tcp_sock(sk);
 	if (!tsk)
 		return 1;
 
-	is_mptcp = bpf_core_field_exists(tsk->is_mptcp) ? tsk->is_mptcp : 0;
+	if (bpf_core_field_exists(tsk->is_mptcp))
+		is_mptcp = BPF_CORE_READ(tsk, is_mptcp);
+	else
+		is_mptcp = 0;
+
 	if (!is_mptcp) {
 		storage = bpf_sk_storage_get(&socket_storage_map, sk, 0,
 					     BPF_SK_STORAGE_GET_F_CREATE);
@@ -57,7 +72,7 @@  int _sockops(struct bpf_sock_ops *ctx)
 		__builtin_memset(storage->ca_name, 0, TCP_CA_NAME_MAX);
 		storage->first = NULL;
 	} else {
-		msk = bpf_skc_to_mptcp_sock(sk);
+		msk = (void *)bpf_skc_to_mptcp_sock(sk);
 		if (!msk)
 			return 1;
 
@@ -66,9 +81,9 @@  int _sockops(struct bpf_sock_ops *ctx)
 		if (!storage)
 			return 1;
 
-		storage->token = msk->token;
-		__builtin_memcpy(storage->ca_name, msk->ca_name, TCP_CA_NAME_MAX);
-		storage->first = msk->first;
+		storage->token = BPF_CORE_READ(msk, token);
+		BPF_CORE_READ_STR_INTO(&storage->ca_name, msk, ca_name);
+		storage->first = BPF_CORE_READ(msk, first);
 	}
 	storage->invoked++;
 	storage->is_mptcp = is_mptcp;
@@ -81,8 +96,15 @@  SEC("fentry/mptcp_pm_new_connection")
 int BPF_PROG(trace_mptcp_pm_new_connection, struct mptcp_sock *msk,
 	     const struct sock *ssk, int server_side)
 {
-	if (!server_side)
-		token = msk->token;
+	struct mptcp_sock___new *mskw;
+
+	if (!server_side) {
+		mskw = (void *)msk;
+		if (bpf_core_field_exists(mskw->token))
+			token = BPF_CORE_READ(mskw, token);
+		else
+			token = 0;
+	}
 
 	return 0;
 }