[PATCH mptcp-next] Squash to "DO-NOT-MERGE: mptcp: improve code coverage for CI"

[PATCH mptcp-next] Squash to "DO-NOT-MERGE: mptcp: improve code coverage for CI"

[Matthieu Baerts (NGI0)]

As discussed on [1], an additional check is done to catch local software
bug.

This patch is supposed to land only in our tree, for both 'export' and
'export-net' branches, because the warning could be triggered by bugged/
malicious peer. We want it in our tree for our CI to detect internal
bugs.

Link: https://lore.kernel.org/all/20240215-mptcp-fix-bogus-pr-warn-v1-1-d14c10312820@kernel.org/ [1]
Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/options.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 27ca42c77b02..5926955625cf 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -982,7 +982,8 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
 		WRITE_ONCE(msk->pm.remote_deny_join_id0, true);
 
 	if (unlikely(!READ_ONCE(msk->pm.server_side)))
-		pr_warn_once("bogus mpc option on established client sk");
+		/* DO-NOT-MERGE: use WARN i/o pr_warn: only for MPTCP export */
+		WARN_ONCE(1, "bogus mpc option on established client sk");
 
 set_fully_established:
 	mptcp_data_lock((struct sock *)msk);

---
base-commit: 65bda47e218b37c3dc919d3ccbd19dfad87a99e5
change-id: 20240216-mptcp-warn-bogus-pr-warn-6dcc88038246

Best regards,
-- 
Matthieu Baerts (NGI0) <matttbe@kernel.org>

Re: [PATCH mptcp-next] Squash to "DO-NOT-MERGE: mptcp: improve code coverage for CI"

[Mat Martineau]

On Fri, 16 Feb 2024, Matthieu Baerts (NGI0) wrote:

> As discussed on [1], an additional check is done to catch local software
> bug.
>
> This patch is supposed to land only in our tree, for both 'export' and
> 'export-net' branches, because the warning could be triggered by bugged/
> malicious peer. We want it in our tree for our CI to detect internal
> bugs.
>
> Link: https://lore.kernel.org/all/20240215-mptcp-fix-bogus-pr-warn-v1-1-d14c10312820@kernel.org/ [1]
> Suggested-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

Looks good to squash. Would be good to keep some of the above info in the 
post-squash commit message for context (especially the lore link).

One other thing, I noticed that this is mentioned in the "DO-NOT-MERGE: 
mptcp: improve code coverage for CI" commit message:

"""
     mptcp: use kmalloc on kasan build

       Helps detection UaF, which apparently kasan misses with kmem_cache
       allocator.

       We also need to always set the SOCK_RCU_FREE flag, to preserved the
       current code leveraging SLAB_TYPESAFE_BY_RCU. This latter change will
       make unreachable some existing errors path, but I don't see other
       options.

"""

But it looks like the code for that disappeared between 
export/20230405T131056 and export/20230405T132954 (also missing on 
export-net). I didn't find mention of this in the mailing list. A topgit 
glitch?


- Mat

> ---
> net/mptcp/options.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 27ca42c77b02..5926955625cf 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -982,7 +982,8 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
> 		WRITE_ONCE(msk->pm.remote_deny_join_id0, true);
>
> 	if (unlikely(!READ_ONCE(msk->pm.server_side)))
> -		pr_warn_once("bogus mpc option on established client sk");
> +		/* DO-NOT-MERGE: use WARN i/o pr_warn: only for MPTCP export */
> +		WARN_ONCE(1, "bogus mpc option on established client sk");
>
> set_fully_established:
> 	mptcp_data_lock((struct sock *)msk);
>
> ---
> base-commit: 65bda47e218b37c3dc919d3ccbd19dfad87a99e5
> change-id: 20240216-mptcp-warn-bogus-pr-warn-6dcc88038246
>
> Best regards,
> -- 
> Matthieu Baerts (NGI0) <matttbe@kernel.org>
>
>
>

Re: [PATCH mptcp-next] Squash to "DO-NOT-MERGE: mptcp: improve code coverage for CI"

[Matthieu Baerts]

Hi Mat,

On 16/02/2024 21:21, Mat Martineau wrote:
> On Fri, 16 Feb 2024, Matthieu Baerts (NGI0) wrote:
> 
>> As discussed on [1], an additional check is done to catch local software
>> bug.
>>
>> This patch is supposed to land only in our tree, for both 'export' and
>> 'export-net' branches, because the warning could be triggered by bugged/
>> malicious peer. We want it in our tree for our CI to detect internal
>> bugs.
>>
>> Link:
>> https://lore.kernel.org/all/20240215-mptcp-fix-bogus-pr-warn-v1-1-d14c10312820@kernel.org/ [1]
>> Suggested-by: Paolo Abeni <pabeni@redhat.com>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> 
> Looks good to squash.

Thank you for the review!

> Would be good to keep some of the above info in
> the post-squash commit message for context (especially the lore link).

Yes, good point, I will update the '.topmsg' file.

> One other thing, I noticed that this is mentioned in the "DO-NOT-MERGE:
> mptcp: improve code coverage for CI" commit message:
> 
> """
>     mptcp: use kmalloc on kasan build
> 
>       Helps detection UaF, which apparently kasan misses with kmem_cache
>       allocator.
> 
>       We also need to always set the SOCK_RCU_FREE flag, to preserved the
>       current code leveraging SLAB_TYPESAFE_BY_RCU. This latter change will
>       make unreachable some existing errors path, but I don't see other
>       options.
> 
> """
> 
> But it looks like the code for that disappeared between
> export/20230405T131056 and export/20230405T132954 (also missing on
> export-net). I didn't find mention of this in the mailing list. A topgit
> glitch?

Oops, I forgot to update the commit message back then. Here is the
modification I did in TopGit:


$ git log --first-parent --no-merges $(./.tg-get-topic.sh "DO-NOT-MERGE:
mptcp: improve code coverage for CI") -2

(...) # (the first commit was an update of my email address)

commit 0fd1a78a8c4d99054a1b47626d45df7f5d36e2ec
Author: Matthieu Baerts <matttbe@kernel.org>
Date:   Wed Apr 5 15:23:56 2023 +0200

    Revert "DO-NOT-MERGE: mptcp: use kmalloc on kasan build"

    This reverts commit 4edee9adfa20120c18e41c26e2ca0a97ead0d93f.

    This piece of code was causing issues, see:


https://github.com/multipath-tcp/mptcp_net-next/issues/371#issuecomment-1496226126

    It sounds safer to revert it then.

    Suggested-by: Paolo Abeni <pabeni@redhat.com>
    Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>


I can fix that!

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-next] Squash to "DO-NOT-MERGE: mptcp: improve code coverage for CI"

[Matthieu Baerts]

Hi Mat,

On 16/02/2024 13:04, Matthieu Baerts (NGI0) wrote:
> As discussed on [1], an additional check is done to catch local software
> bug.
> 
> This patch is supposed to land only in our tree, for both 'export' and
> 'export-net' branches, because the warning could be triggered by bugged/
> malicious peer. We want it in our tree for our CI to detect internal
> bugs.

Thank you for the review!

Now in our tree, for both -net and -next:

New patches for t/upstream:
- 735e80ba1492: tg:msg: remove 'use kmalloc on kasan build'
- 91351bfa97db: "squashed" in "DO-NOT-MERGE: mptcp: improve code
coverage for CI"
- 359220a914aa: tg:msg: add 'warn in case of bogus mpc option on
established client sk'
- Results: f2fb9bec2195..d5aedc5b25ce (export)

New patches for t/upstream-net:
- a1880660e156: tg:msg: remove 'use kmalloc on kasan build'
- 1c78093d056a: Squash to "DO-NOT-MERGE: mptcp: improve code coverage
for CI"
- 5405ccb66c00: tg:msg: add 'warn in case of bogus mpc option on
established client sk'
- Results: 9dcf86462a59..22c802d10725 (export-net)

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

[PATCH mptcp-next] Squash to "DO-NOT-MERGE: mptcp: improve code coverage for CI"

[Matthieu Baerts (NGI0)]

And in "DO-NOT-MERGE: mptcp: improve code coverage for CI (net)".

This is similar to commit bc75dcc3cea7 ("net: rds: add option for GCOV
profiling"), but I don't think we need to upstream that. Or not for the
moment as the selftests don't depend on GCOV support.

If we do, we will also need to update Documentation/dev-tools/gcov.rst.

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/Kconfig  | 9 +++++++++
 net/mptcp/Makefile | 5 +++++
 2 files changed, 14 insertions(+)

diff --git a/net/mptcp/Kconfig b/net/mptcp/Kconfig
index 10c97e19a7da5cbb1538b34a8eeca2e89795dfae..f674915dc31e521e6fe9dbcef03e65bdba10dd3d 100644
--- a/net/mptcp/Kconfig
+++ b/net/mptcp/Kconfig
@@ -37,4 +37,13 @@ config MPTCP_KUNIT_TEST
 
 	  If unsure, say N.
 
+config GCOV_PROFILE_MPTCP
+	bool "Enable GCOV profiling on MPTCP"
+	depends on GCOV_KERNEL
+	help
+	  Enable GCOV profiling on MPTCP for checking which functions/lines
+	  are executed.
+
+	  If unsure, say N.
+
 endif
diff --git a/net/mptcp/Makefile b/net/mptcp/Makefile
index bcf1dbf3a432f7423b048e985667f199797042b3..5dbc37d38d7a4b0779c71e7b291ef83a49e46648 100644
--- a/net/mptcp/Makefile
+++ b/net/mptcp/Makefile
@@ -13,3 +13,8 @@ mptcp_token_test-objs := token_test.o
 obj-$(CONFIG_MPTCP_KUNIT_TEST) += mptcp_crypto_test.o mptcp_token_test.o
 
 obj-$(CONFIG_BPF_SYSCALL) += bpf.o
+
+# for GCOV coverage profiling
+ifdef CONFIG_GCOV_PROFILE_MPTCP
+GCOV_PROFILE := y
+endif

---
base-commit: d269ec78a30bc7a57999838c477a83b0e0101a57
change-id: 20240924-mptcp-gcov-49368d19b22f

Best regards,
-- 
Matthieu Baerts (NGI0) <matttbe@kernel.org>

Re: [PATCH mptcp-next] Squash to "DO-NOT-MERGE: mptcp: improve code coverage for CI"

[Matthieu Baerts]

Hello,

On 24/09/2024 18:39, Matthieu Baerts (NGI0) wrote:
> And in "DO-NOT-MERGE: mptcp: improve code coverage for CI (net)".
> 
> This is similar to commit bc75dcc3cea7 ("net: rds: add option for GCOV
> profiling"), but I don't think we need to upstream that. Or not for the
> moment as the selftests don't depend on GCOV support.
> 
> If we do, we will also need to update Documentation/dev-tools/gcov.rst.
FYI, I just applied this patch in our tree

- 66b668896210: "squashed" in "DO-NOT-MERGE: mptcp: improve code
coverage for CI (net)"
- 07234ab477d3: "squashed" in "DO-NOT-MERGE: mptcp: improve code
coverage for CI"

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-next] Squash to "DO-NOT-MERGE: mptcp: improve code coverage for CI"

[MPTCP CI]

Hi Matthieu,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/11018378042

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/1ce376cf68b0
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=892525


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

[PATCH mptcp-next] Squash to "DO-NOT-MERGE: mptcp: improve code coverage for CI"

[Geliang Tang]

When our bpf_selftests are re-built, this error occurs:

 make[1]: *** No rule to make target 'atomics.lskel.h',
			needed by '.../atomics.test.o'.  Stop.
 make[1]: *** Waiting for unfinished jobs....
 make[1]: Leaving directory 'tools/testing/selftests/bpf'
 make: *** [Makefile:259: install] Error 2
 make: Leaving directory 'tools/testing/selftests'

In this case, I had to remove the cache directory .virtme and recompile it
again. This was very time-consuming.

A workaround patch for this is found in [1]. I don't know why this patch
has not been upstreamed, but it's in BPF CI environment. I think it's
necessary to synchronize this modification to MPTCP CI environment as well.

[1]
https://lore.kernel.org/bpf/Q3BN2kW9Kgy6LkrDOwnyY4Pv7_YF8fInLCd2_QA3LimKYM3wD64kRdnwp7blwG2dI_s7UGnfUae-4_dOmuTrxpYCi32G_KTzB3PfmxIerH8=@pm.me/T/

Signed-off-by: Geliang Tang <geliang@kernel.org>
---
 tools/testing/selftests/bpf/Makefile | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 9ec53c858d8d..64388a2fe5a0 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -652,6 +652,12 @@ $(TRUNNER_BPF_SKELS_LINKED): $(TRUNNER_OUTPUT)/%: $$$$(%-deps) $(BPFTOOL) | $(TR
 $(notdir %.skel.h): $(TRUNNER_OUTPUT)/%.skel.h
 	@true
 
+$(notdir %.lskel.h): $(TRUNNER_OUTPUT)/%.lskel.h
+	@true
+
+$(notdir %.subskel.h): $(TRUNNER_OUTPUT)/%.subskel.h
+	@true
+
 endif
 
 # ensure we set up tests.h header generation rule just once
-- 
2.45.2

Re: [PATCH mptcp-next] Squash to "DO-NOT-MERGE: mptcp: improve code coverage for CI"

[MPTCP CI]

Hi Geliang,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/11660745489

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/7c468c5ee032
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=905930


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

Re: [PATCH mptcp-next] Squash to "DO-NOT-MERGE: mptcp: improve code coverage for CI"

[Matthieu Baerts]

Hi Geliang,

On 04/11/2024 09:28, Geliang Tang wrote:
> When our bpf_selftests are re-built, this error occurs:
> 
>  make[1]: *** No rule to make target 'atomics.lskel.h',
> 			needed by '.../atomics.test.o'.  Stop.
>  make[1]: *** Waiting for unfinished jobs....
>  make[1]: Leaving directory 'tools/testing/selftests/bpf'
>  make: *** [Makefile:259: install] Error 2
>  make: Leaving directory 'tools/testing/selftests'

Ah yes, I already noticed that when checking some stuffs, but I didn't
investigate. Thank you for having looked at this!

> In this case, I had to remove the cache directory .virtme and recompile it
> again. This was very time-consuming.

You don't need to remove the whole ".virtme" directory, you can remove
".virtme/build*" instead, so you still have CCache (from ".virtme/ccache*").

> A workaround patch for this is found in [1]. I don't know why this patch
> has not been upstreamed, but it's in BPF CI environment. I think it's
> necessary to synchronize this modification to MPTCP CI environment as well.

It looks like another version has been applied in bpf-next, even if it
is a fix:

  https://lore.kernel.org/bpf/20240916195919.1872371-2-ihor.solodrai@pm.me/

Do you mind checking it please?

If it works, I can backport it to our tree, and ask BPF maintainers if
they can add these two patches in 'bpf' (fix) instead, because this
issue is present in v6.12, but only fixed in next (v6.13).

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-next] Squash to "DO-NOT-MERGE: mptcp: improve code coverage for CI"

[Matthieu Baerts]

On 04/11/2024 12:58, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 04/11/2024 09:28, Geliang Tang wrote:
>> When our bpf_selftests are re-built, this error occurs:
>>
>>  make[1]: *** No rule to make target 'atomics.lskel.h',
>> 			needed by '.../atomics.test.o'.  Stop.
>>  make[1]: *** Waiting for unfinished jobs....
>>  make[1]: Leaving directory 'tools/testing/selftests/bpf'
>>  make: *** [Makefile:259: install] Error 2
>>  make: Leaving directory 'tools/testing/selftests'
> 
> Ah yes, I already noticed that when checking some stuffs, but I didn't
> investigate. Thank you for having looked at this!
> 
>> In this case, I had to remove the cache directory .virtme and recompile it
>> again. This was very time-consuming.
> 
> You don't need to remove the whole ".virtme" directory, you can remove
> ".virtme/build*" instead, so you still have CCache (from ".virtme/ccache*").
> 
>> A workaround patch for this is found in [1]. I don't know why this patch
>> has not been upstreamed, but it's in BPF CI environment. I think it's
>> necessary to synchronize this modification to MPTCP CI environment as well.
> 
> It looks like another version has been applied in bpf-next, even if it
> is a fix:
> 
>   https://lore.kernel.org/bpf/20240916195919.1872371-2-ihor.solodrai@pm.me/
> 
> Do you mind checking it please?

Just in case, their bpf-next/master branch has been rewritten in
between. The last ref I have is:

  fd4a0e67838c ("selftests/bpf: Set vpath in Makefile to search for skels")

> If it works, I can backport it to our tree, and ask BPF maintainers if
> they can add these two patches in 'bpf' (fix) instead, because this
> issue is present in v6.12, but only fixed in next (v6.13).
> 
> Cheers,
> Matt

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-next] Squash to "DO-NOT-MERGE: mptcp: improve code coverage for CI"

[Geliang Tang]

Hi Matt,

On Mon, 2024-11-04 at 13:07 +0100, Matthieu Baerts wrote:
> On 04/11/2024 12:58, Matthieu Baerts wrote:
> > Hi Geliang,
> > 
> > On 04/11/2024 09:28, Geliang Tang wrote:
> > > When our bpf_selftests are re-built, this error occurs:
> > > 
> > >  make[1]: *** No rule to make target 'atomics.lskel.h',
> > > 			needed by '.../atomics.test.o'.  Stop.
> > >  make[1]: *** Waiting for unfinished jobs....
> > >  make[1]: Leaving directory 'tools/testing/selftests/bpf'
> > >  make: *** [Makefile:259: install] Error 2
> > >  make: Leaving directory 'tools/testing/selftests'
> > 
> > Ah yes, I already noticed that when checking some stuffs, but I
> > didn't
> > investigate. Thank you for having looked at this!
> > 
> > > In this case, I had to remove the cache directory .virtme and
> > > recompile it
> > > again. This was very time-consuming.
> > 
> > You don't need to remove the whole ".virtme" directory, you can
> > remove
> > ".virtme/build*" instead, so you still have CCache (from
> > ".virtme/ccache*").
> > 
> > > A workaround patch for this is found in [1]. I don't know why
> > > this patch
> > > has not been upstreamed, but it's in BPF CI environment. I think
> > > it's
> > > necessary to synchronize this modification to MPTCP CI
> > > environment as well.
> > 
> > It looks like another version has been applied in bpf-next, even if
> > it
> > is a fix:
> > 
> >  
> > https://lore.kernel.org/bpf/20240916195919.1872371-2-ihor.solodrai@pm.me/
> > 
> > Do you mind checking it please?
> 
> Just in case, their bpf-next/master branch has been rewritten in
> between. The last ref I have is:
> 
>   fd4a0e67838c ("selftests/bpf: Set vpath in Makefile to search for
> skels")
> 
> > If it works, I can backport it to our tree, and ask BPF maintainers

It works. And "selftests/bpf: Set vpath in Makefile to search for
skels" is in our tree now. So no need to apply this squash-to patch
anymore.

I changed this squash-to patch as "Rejected" in patchwork.

Thanks,
-Geliang

> > if
> > they can add these two patches in 'bpf' (fix) instead, because this
> > issue is present in v6.12, but only fixed in next (v6.13).
> > 
> > Cheers,
> > Matt
> 
> Cheers,
> Matt