mptcp: avoid printing warning once on client side

[PATCH mptcp-net] mptcp: avoid printing warning once on client side

Posted by Matthieu Baerts (NGI0) 2 months, 1 week ago

After the 'Fixes' commit mentioned below, the client side might print
the following warning once when a subflow is fully established at the
reception of any valid additional ack:

  MPTCP: bogus mpc option on established client sk

That's a normal situation, and no warning should be printed for that. We
can then skip the check when the label is used.

Fixes: e4a0fa47e816 ("mptcp: corner case locking for rx path fields initialization")
Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Notes:
 - Should we convert this pr_warn_once() to a WARN_ONCE()? Or just in
   our tree? Or just in DEBUG mode?
---
 net/mptcp/options.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 23e317ffc901..27ca42c77b02 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -981,10 +981,10 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
 	if (mp_opt->deny_join_id0)
 		WRITE_ONCE(msk->pm.remote_deny_join_id0, true);
 
-set_fully_established:
 	if (unlikely(!READ_ONCE(msk->pm.server_side)))
 		pr_warn_once("bogus mpc option on established client sk");
 
+set_fully_established:
 	mptcp_data_lock((struct sock *)msk);
 	__mptcp_subflow_fully_established(msk, subflow, mp_opt);
 	mptcp_data_unlock((struct sock *)msk);

---
base-commit: 52e05de42e1094a943053af93ea08c51a44091f7
change-id: 20240215-mptcp-fix-bogus-pr-warn-497a3537984b

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

Re: [PATCH mptcp-net] mptcp: avoid printing warning once on client side

Posted by Matthieu Baerts 2 months, 1 week ago

Hi Mat,

On 15/02/2024 16:06, Matthieu Baerts (NGI0) wrote:
> After the 'Fixes' commit mentioned below, the client side might print
> the following warning once when a subflow is fully established at the
> reception of any valid additional ack:
> 
>   MPTCP: bogus mpc option on established client sk
> 
> That's a normal situation, and no warning should be printed for that. We
> can then skip the check when the label is used.
> 
> Fixes: e4a0fa47e816 ("mptcp: corner case locking for rx path fields initialization")
> Suggested-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

Thank you for the review!

Now in our tree (fixes for -net):

New patches for t/upstream-net and t/upstream:
- 738c16c9c6f1: mptcp: avoid printing warning once on client side
- Results: 8acc1a7e7787..f2ba4d61dbc4 (export-net)
- Results: 8cc4e9d608dd..0b8245446c36 (export)

Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export-net/20240216T104440
https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20240216T104440

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

Re: [PATCH mptcp-net] mptcp: avoid printing warning once on client side

Posted by Mat Martineau 2 months, 1 week ago

On Thu, 15 Feb 2024, Matthieu Baerts (NGI0) wrote:

> After the 'Fixes' commit mentioned below, the client side might print
> the following warning once when a subflow is fully established at the
> reception of any valid additional ack:
>
>  MPTCP: bogus mpc option on established client sk
>
> That's a normal situation, and no warning should be printed for that. We
> can then skip the check when the label is used.
>
> Fixes: e4a0fa47e816 ("mptcp: corner case locking for rx path fields initialization")
> Suggested-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>

Looks good to me:

Reviewed-by: Mat Martineau <martineau@kernel.org>

> ---
> Notes:
> - Should we convert this pr_warn_once() to a WARN_ONCE()? Or just in
>   our tree? Or just in DEBUG mode?

I think it makes sense to keep this patch minimal for -net and stable 
(just moving the label).

Also given the consequences of panic_on_warn, would be better to make any 
changes to WARN_ONCE() in mptcp-next/net-next. I don't see extra 
complexity to modify this warning in our tree or debug mode as being worth 
it, do you think it would be valuable?

- Mat

> ---
> net/mptcp/options.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 23e317ffc901..27ca42c77b02 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -981,10 +981,10 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
> 	if (mp_opt->deny_join_id0)
> 		WRITE_ONCE(msk->pm.remote_deny_join_id0, true);
>
> -set_fully_established:
> 	if (unlikely(!READ_ONCE(msk->pm.server_side)))
> 		pr_warn_once("bogus mpc option on established client sk");
>
> +set_fully_established:
> 	mptcp_data_lock((struct sock *)msk);
> 	__mptcp_subflow_fully_established(msk, subflow, mp_opt);
> 	mptcp_data_unlock((struct sock *)msk);
>
> ---
> base-commit: 52e05de42e1094a943053af93ea08c51a44091f7
> change-id: 20240215-mptcp-fix-bogus-pr-warn-497a3537984b
>
> Best regards,
> -- 
> Matthieu Baerts (NGI0) <matttbe@kernel.org>
>
>
>

Re: [PATCH mptcp-net] mptcp: avoid printing warning once on client side

Posted by Matthieu Baerts 2 months, 1 week ago

Hi Mat,

On 16/02/2024 00:25, Mat Martineau wrote:
> On Thu, 15 Feb 2024, Matthieu Baerts (NGI0) wrote:
> 
>> After the 'Fixes' commit mentioned below, the client side might print
>> the following warning once when a subflow is fully established at the
>> reception of any valid additional ack:
>>
>>  MPTCP: bogus mpc option on established client sk
>>
>> That's a normal situation, and no warning should be printed for that. We
>> can then skip the check when the label is used.
>>
>> Fixes: e4a0fa47e816 ("mptcp: corner case locking for rx path fields
>> initialization")
>> Suggested-by: Paolo Abeni <pabeni@redhat.com>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> 
> Looks good to me:
> 
> Reviewed-by: Mat Martineau <martineau@kernel.org>

Thank you for the review!

>> ---
>> Notes:
>> - Should we convert this pr_warn_once() to a WARN_ONCE()? Or just in
>>   our tree? Or just in DEBUG mode?
> 
> I think it makes sense to keep this patch minimal for -net and stable
> (just moving the label).
> 
> Also given the consequences of panic_on_warn, would be better to make
> any changes to WARN_ONCE() in mptcp-next/net-next. I don't see extra
> complexity to modify this warning in our tree or debug mode as being
> worth it, do you think it would be valuable?

Sorry, I'm a bit confused by your reply. For the moment, our CI
complains when a "Call Trace:" is printed, but it doesn't complain when
there is a pr_warn().

If the warning here can be caused by interactions with a buggy host, it
makes sense not to have a WARN() when used in production, but it would
be good for our CI can catch that. If the warning can only be caused by
an internal bug, then easier to use a WARN_ONCE().

So I think here, we could maybe convert it to a DEBUG_NET_WARN_ON_ONCE()
and upstream that. WDYT?

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

Re: [PATCH mptcp-net] mptcp: avoid printing warning once on client side

Posted by Paolo Abeni 2 months, 1 week ago

On Fri, 2024-02-16 at 11:42 +0100, Matthieu Baerts wrote:
> Hi Mat,
> 
> On 16/02/2024 00:25, Mat Martineau wrote:
> > On Thu, 15 Feb 2024, Matthieu Baerts (NGI0) wrote:
> > 
> > > After the 'Fixes' commit mentioned below, the client side might print
> > > the following warning once when a subflow is fully established at the
> > > reception of any valid additional ack:
> > > 
> > >  MPTCP: bogus mpc option on established client sk
> > > 
> > > That's a normal situation, and no warning should be printed for that. We
> > > can then skip the check when the label is used.
> > > 
> > > Fixes: e4a0fa47e816 ("mptcp: corner case locking for rx path fields
> > > initialization")
> > > Suggested-by: Paolo Abeni <pabeni@redhat.com>
> > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> > 
> > Looks good to me:
> > 
> > Reviewed-by: Mat Martineau <martineau@kernel.org>
> 
> Thank you for the review!
> 
> > > ---
> > > Notes:
> > > - Should we convert this pr_warn_once() to a WARN_ONCE()? Or just in
> > >   our tree? Or just in DEBUG mode?
> > 
> > I think it makes sense to keep this patch minimal for -net and stable
> > (just moving the label).
> > 
> > Also given the consequences of panic_on_warn, would be better to make
> > any changes to WARN_ONCE() in mptcp-next/net-next. I don't see extra
> > complexity to modify this warning in our tree or debug mode as being
> > worth it, do you think it would be valuable?
> 
> Sorry, I'm a bit confused by your reply. For the moment, our CI
> complains when a "Call Trace:" is printed, but it doesn't complain when
> there is a pr_warn().
> 
> If the warning here can be caused by interactions with a buggy host, it
> makes sense not to have a WARN() when used in production, but it would
> be good for our CI can catch that. If the warning can only be caused by
> an internal bug, then easier to use a WARN_ONCE().

We can trigger the warning due to either:
* local S/W bug
* bugged/malicious peer.

I think we should avoid a WARN there.

> So I think here, we could maybe convert it to a DEBUG_NET_WARN_ON_ONCE()
> and upstream that. WDYT?

DEBUG_NET is quite lightweight and could be enabled even in production,
that should be an export-branch only change.

Cheers,

Paolo

Re: [PATCH mptcp-net] mptcp: avoid printing warning once on client side

Posted by Matthieu Baerts 2 months, 1 week ago

Hi Paolo,

Thank you for your reply!

On 16/02/2024 12:23, Paolo Abeni wrote:
> On Fri, 2024-02-16 at 11:42 +0100, Matthieu Baerts wrote:
>> On 16/02/2024 00:25, Mat Martineau wrote:
>>> On Thu, 15 Feb 2024, Matthieu Baerts (NGI0) wrote:

(...)

>>>> Notes:
>>>> - Should we convert this pr_warn_once() to a WARN_ONCE()? Or just in
>>>>   our tree? Or just in DEBUG mode?
>>>
>>> I think it makes sense to keep this patch minimal for -net and stable
>>> (just moving the label).
>>>
>>> Also given the consequences of panic_on_warn, would be better to make
>>> any changes to WARN_ONCE() in mptcp-next/net-next. I don't see extra
>>> complexity to modify this warning in our tree or debug mode as being
>>> worth it, do you think it would be valuable?
>>
>> Sorry, I'm a bit confused by your reply. For the moment, our CI
>> complains when a "Call Trace:" is printed, but it doesn't complain when
>> there is a pr_warn().
>>
>> If the warning here can be caused by interactions with a buggy host, it
>> makes sense not to have a WARN() when used in production, but it would
>> be good for our CI can catch that. If the warning can only be caused by
>> an internal bug, then easier to use a WARN_ONCE().
> 
> We can trigger the warning due to either:
> * local S/W bug
> * bugged/malicious peer.
> 
> I think we should avoid a WARN there.
> 
>> So I think here, we could maybe convert it to a DEBUG_NET_WARN_ON_ONCE()
>> and upstream that. WDYT?
> 
> DEBUG_NET is quite lightweight and could be enabled even in production,

Ah OK, I didn't know that!

> that should be an export-branch only change.

Fine by me! I appreciate your suggestion.

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

Re: [PATCH mptcp-net] mptcp: avoid printing warning once on client side

Posted by Mat Martineau 2 months, 1 week ago

On Fri, 16 Feb 2024, Matthieu Baerts wrote:

> Hi Paolo,
>
> Thank you for your reply!
>
> On 16/02/2024 12:23, Paolo Abeni wrote:
>> On Fri, 2024-02-16 at 11:42 +0100, Matthieu Baerts wrote:
>>> On 16/02/2024 00:25, Mat Martineau wrote:
>>>> On Thu, 15 Feb 2024, Matthieu Baerts (NGI0) wrote:
>
> (...)
>
>>>>> Notes:
>>>>> - Should we convert this pr_warn_once() to a WARN_ONCE()? Or just in
>>>>>   our tree? Or just in DEBUG mode?
>>>>
>>>> I think it makes sense to keep this patch minimal for -net and stable
>>>> (just moving the label).
>>>>
>>>> Also given the consequences of panic_on_warn, would be better to make
>>>> any changes to WARN_ONCE() in mptcp-next/net-next. I don't see extra
>>>> complexity to modify this warning in our tree or debug mode as being
>>>> worth it, do you think it would be valuable?
>>>
>>> Sorry, I'm a bit confused by your reply. For the moment, our CI
>>> complains when a "Call Trace:" is printed, but it doesn't complain when
>>> there is a pr_warn().
>>>

Ok, I didn't realize our CI ignored pr_warn() output so didn't really see 
the motivation for switching to the WARN() macro - but did want to make 
sure (at a minimum) that you weren't proposing WARN() for a -net patch.

>>> If the warning here can be caused by interactions with a buggy host, it
>>> makes sense not to have a WARN() when used in production, but it would
>>> be good for our CI can catch that. If the warning can only be caused by
>>> an internal bug, then easier to use a WARN_ONCE().

This makes sense - I agree it's better for CI to catch it.

>>
>> We can trigger the warning due to either:
>> * local S/W bug
>> * bugged/malicious peer.
>>
>> I think we should avoid a WARN there.
>>
>>> So I think here, we could maybe convert it to a DEBUG_NET_WARN_ON_ONCE()
>>> and upstream that. WDYT?
>>
>> DEBUG_NET is quite lightweight and could be enabled even in production,
>
> Ah OK, I didn't know that!
>
>> that should be an export-branch only change.
>

+1

> Fine by me! I appreciate your suggestion.
>

Thanks Paolo and Matthieu


- Mat