Re: [PATCH v2 mptcp-net] mptcp: do not warn on bad input from the network

[Mat Martineau <mathew.j.martineau@linux.intel.com>]

On Thu, 3 Jun 2021, Paolo Abeni wrote:

> warn_bad_map() produces a kernel WARN on bad input coming
> from the network. Use pr_debug() to avoid spamming the system
> log.
>
> Additionally, when the right bound check fails, warn_bad_map() reports
> the wrong ssn value, let's fix it.
>
> Fixes: 648ef4b88673 ("mptcp: Implement MPTCP receive path")
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/107
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> I think we should really abvoid the warn here. Later patches will add
> MIB counters for the relevant event
> ---
> net/mptcp/subflow.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>

Thanks for the patch Paolo. Looks good.

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>



> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 5ef31aa16cc1..259f99b61c35 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -787,10 +787,10 @@ static u64 expand_seq(u64 old_seq, u16 old_data_len, u64 seq)
> 	return seq | ((old_seq + old_data_len + 1) & GENMASK_ULL(63, 32));
> }
>
> -static void warn_bad_map(struct mptcp_subflow_context *subflow, u32 ssn)
> +static void dbg_bad_map(struct mptcp_subflow_context *subflow, u32 ssn)
> {
> -	WARN_ONCE(1, "Bad mapping: ssn=%d map_seq=%d map_data_len=%d",
> -		  ssn, subflow->map_subflow_seq, subflow->map_data_len);
> +	pr_debug("Bad mapping: ssn=%d map_seq=%d map_data_len=%d",
> +		 ssn, subflow->map_subflow_seq, subflow->map_data_len);
> }
>
> static bool skb_is_fully_mapped(struct sock *ssk, struct sk_buff *skb)
> @@ -815,13 +815,13 @@ static bool validate_mapping(struct sock *ssk, struct sk_buff *skb)
> 		/* Mapping covers data later in the subflow stream,
> 		 * currently unsupported.
> 		 */
> -		warn_bad_map(subflow, ssn);
> +		dbg_bad_map(subflow, ssn);
> 		return false;
> 	}
> 	if (unlikely(!before(ssn, subflow->map_subflow_seq +
> 				  subflow->map_data_len))) {
> 		/* Mapping does covers past subflow data, invalid */
> -		warn_bad_map(subflow, ssn + skb->len);
> +		dbg_bad_map(subflow, ssn);
> 		return false;
> 	}
> 	return true;
> -- 
> 2.26.3
>
>
>

--
Mat Martineau
Intel