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

[Paolo Abeni <pabeni@redhat.com>]

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