linux-kernel - 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: <08f925cd-e267-4a6b-84b1-792515c4e199@kernel.org>
Date: Thu, 4 Jul 2024 12:48:08 +0200
From: Matthieu Baerts <matttbe@...nel.org>
To: Andrii Nakryiko <andrii@...nel.org>, Eduard Zingerman
 <eddyz87@...il.com>, Mykola Lysenko <mykolal@...com>,
 Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>,
 Martin KaFai Lau <martin.lau@...ux.dev>, 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>
Cc: 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

Hello,

@BPF people: this new tool doesn't compile on the BPF CI [1]. Can I have
a hand to find the best way to fix this please? (see below)

[1] https://github.com/kernel-patches/bpf/pull/7296

On 03/07/2024 20:57, Matthieu Baerts (NGI0) wrote:
> From: Geliang Tang <tanggeliang@...inos.cn>
> 
> This patch adds a symlink to MPTCP's pm_nl_ctl tool into bpf selftests,
> and updates Makefile to compile it.
> 
> This is useful to run MPTCP BPF selftests on systems with an old version
> of IPRoute2. This tool can be used as an alternative to 'ip mptcp'.

(...)

> 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=(...)

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
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
-- 
Sponsored by the NGI0 Core fund.