netdev - Re: [PATCH bpf-next v3 2/3] selftests/bpf: Add mptcp pm_nl

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <90e916e8-ec4e-447b-8ee6-eb247f3a72ad@linux.dev>
Date: Fri, 5 Jul 2024 16:10:27 -0700
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Matthieu Baerts <matttbe@...nel.org>
Cc: Andrii Nakryiko <andrii@...nel.org>, Eduard Zingerman
 <eddyz87@...il.com>, Mykola Lysenko <mykolal@...com>,
 Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>,
 Song Liu <song@...nel.org>, Yonghong Song <yonghong.song@...ux.dev>,
 John Fastabend <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>,
 Stanislav Fomichev <sdf@...gle.com>, Hao Luo <haoluo@...gle.com>,
 Jiri Olsa <jolsa@...nel.org>, "David S. Miller" <davem@...emloft.net>,
 Jakub Kicinski <kuba@...nel.org>, Jesper Dangaard Brouer <hawk@...nel.org>,
 linux-kernel@...r.kernel.org, netdev@...r.kernel.org, bpf@...r.kernel.org,
 linux-kselftest@...r.kernel.org, Geliang Tang <tanggeliang@...inos.cn>,
 mptcp@...ts.linux.dev, Mat Martineau <martineau@...nel.org>,
 Geliang Tang <geliang@...nel.org>, Shuah Khan <shuah@...nel.org>
Subject: Re: [PATCH bpf-next v3 2/3] selftests/bpf: Add mptcp pm_nl_ctl link

On 7/4/24 3:48 AM, Matthieu Baerts wrote:
>> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
>> index e0b3887b3d2d..204269d0b5b8 100644
>> --- a/tools/testing/selftests/bpf/Makefile
>> +++ b/tools/testing/selftests/bpf/Makefile
>> @@ -144,7 +144,7 @@ TEST_GEN_PROGS_EXTENDED = test_skb_cgroup_id_user \
>>   	flow_dissector_load test_flow_dissector test_tcp_check_syncookie_user \
>>   	test_lirc_mode2_user xdping test_cpp runqslower bench bpf_testmod.ko \
>>   	xskxceiver xdp_redirect_multi xdp_synproxy veristat xdp_hw_metadata \
>> -	xdp_features bpf_test_no_cfi.ko
>> +	xdp_features bpf_test_no_cfi.ko mptcp_pm_nl_ctl
> On the BPF CI, we have such errors:
> 
>     mptcp_pm_nl_ctl.c:20:10: fatal error: 'linux/mptcp.h' file not found
>       20 | #include "linux/mptcp.h"
>          |          ^~~~~~~~~~~~~~~
> 
> On my side, I don't have any issue, because the compiler uses the
> mptcp.h file from the system: /usr/include/linux/mptcp.h
> 
> I suppose that's not OK on the BPF CI, as it looks like it doesn't have
> this file there, probably because it still uses Ubuntu 20.04 as base,
> which doesn't include this file in the linux-libc-dev package.
> 
> When I look at how this 'mptcp_pm_nl_ctl' tool -- and all the other
> programs from that list -- is compiled (V=1), I see that the following
> "-I" options are given:
> 
>    -I${PWD}/tools/testing/selftests/bpf
>    -I${BUILD}//tools/include
>    -I${BUILD}/include/generated
>    -I${PWD}/tools/lib
>    -I${PWD}/tools/include
>    -I${PWD}/tools/include/uapi
>    -I${BUILD}/
> 
> It will then not look at -I${PWD}/usr/include or the directory generated
> with:
> 
>    make headers_install INSTALL_HDR_PATH=(...)

It sounds like the tools/testing/selftests/net/mptcp/Makefile is looking at this 
include path, so it works?

iiu the bpf/Makefile correctly, it has the bpftool "make" compiled and installed 
at tools/testing/selftests/bpf/tools/sbin/. May be directly compile the 
pm_nl_ctl by "make tools/testing/selftests/net/mptcp/"?

> 
> I guess that's why people have duplicated files in 'tools/include/uapi',
> but I also understood from Jakub that it is not a good idea to continue
> to do so.
> 
> What would be the best solution to avoid a copy? A symlink still looks
> like a workaround.
> 
> In the other selftests, KHDR_INCLUDES is used to be able to include the
> path containing the UAPI headers. So if someone built the headers in a

Meaning KHDR_INCLUDES should be used and -I${PWD}/tools/include/uapi can be 
retired? I haven't looked into the details. I quickly tried but it fails in my 
environment.

> seperated directory -- INSTALL_HDR_PATH=(...) -- KHDR_INCLUDES can be
> overridden to look there, instead of ${KERNEL_SRC}/usr/include. Would it
> be OK to do that? Would it work for the CI without extra changes? Or do
> you still prefer a copy/symlink to 'tools/include/uapi' instead?

> 
> Cheers,
> Matt