[v1] 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"

Posted by Matthieu Baerts (NGI0) 1 year, 11 months ago

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"

Posted by Matthieu Baerts 1 year, 11 months ago

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.

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

Posted by Mat Martineau 1 year, 11 months ago

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"

Posted by Matthieu Baerts 1 year, 11 months ago

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.