[MPTCP] [PATCH 1/7] mptcp: reduce number of pr_debug() calls.

[MPTCP] [PATCH 1/7] mptcp: reduce number of pr_debug() calls.

[Peter Krystad]

[-- Attachment #1: Type: text/plain, Size: 2368 bytes --]

squash to: Handle MPTCP TCP options

Signed-off-by: Peter Krystad <peter.krystad(a)linux.intel.com>
---
 net/mptcp/options.c | 36 +++++++++++++++++++-----------------
 1 file changed, 19 insertions(+), 17 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 9b9b9c9390c1..964e73b6b48b 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -13,7 +13,7 @@
 void mptcp_parse_option(const unsigned char *ptr, int opsize,
 			struct tcp_options_received *opt_rx)
 {
-	struct mptcp_options_received *mp_opt;
+	struct mptcp_options_received *mp_opt = &opt_rx->mptcp;
 	u8 subtype = *ptr >> 4;
 	int expected_opsize;
 
@@ -25,25 +25,28 @@ void mptcp_parse_option(const unsigned char *ptr, int opsize,
 	 * 10-17: Receiver key (optional)
 	 */
 	case MPTCPOPT_MP_CAPABLE:
-		if (opsize != TCPOLEN_MPTCP_MPC_SYN &&
-		    opsize != TCPOLEN_MPTCP_MPC_SYNACK)
-			break;
-
-		pr_debug("MP_CAPABLE");
-		opt_rx->mptcp.version = *ptr++ & MPTCPOPT_VERSION_MASK;
-		if (opt_rx->mptcp.version != 0)
+		mp_opt->version = *ptr++ & MPTCPOPT_VERSION_MASK;
+		if (mp_opt->version != 0)
 			break;
 
-		opt_rx->mptcp.mp_capable = 1;
-		pr_debug("flags=%02x", *ptr);
-		opt_rx->mptcp.flags = *ptr++;
-		opt_rx->mptcp.sndr_key = get_unaligned_be64(ptr);
-		pr_debug("sndr_key=%llu", opt_rx->mptcp.sndr_key);
+		mp_opt->mp_capable = 1;
+		mp_opt->flags = *ptr++;
+		mp_opt->sndr_key = get_unaligned_be64(ptr);
 		ptr += 8;
-		if (opsize == TCPOLEN_MPTCP_MPC_SYNACK) {
-			opt_rx->mptcp.rcvr_key = get_unaligned_be64(ptr);
-			pr_debug("rcvr_key=%llu", opt_rx->mptcp.rcvr_key);
+
+		if (opsize == TCPOLEN_MPTCP_MPC_SYN) {
+			pr_debug("MP_CAPABLE flags=%x, sndr=%llu",
+				 mp_opt->flags, mp_opt->sndr_key);
+		} else if ((opsize == TCPOLEN_MPTCP_MPC_SYNACK) ||
+			   (opsize == TCPOLEN_MPTCP_MPC_ACK)) {
+			mp_opt->rcvr_key = get_unaligned_be64(ptr);
 			ptr += 8;
+			pr_debug("MP_CAPABLE flags=%x, sndr=%llu, rcvr=%llu",
+				 mp_opt->flags, mp_opt->sndr_key,
+				 mp_opt->rcvr_key);
+		} else {
+			pr_warn("MP_CAPABLE bad option size");
+			mp_opt->mp_capable = 0;
 		}
 		break;
 
@@ -79,7 +82,6 @@ void mptcp_parse_option(const unsigned char *ptr, int opsize,
 	 */
 	case MPTCPOPT_DSS:
 		pr_debug("DSS");
-		mp_opt = &opt_rx->mptcp;
 		mp_opt->dss = 1;
 		ptr++;
 
-- 
2.17.2

Re: [MPTCP] [PATCH 1/7] mptcp: reduce number of pr_debug() calls.

[Paolo Abeni]

[-- Attachment #1: Type: text/plain, Size: 2471 bytes --]

Hi Peter,

Thank you for the great effort!

I have a few comments, see below...

On Wed, 2019-06-05 at 17:36 -0700, Peter Krystad wrote:
> squash to: Handle MPTCP TCP options
> 
> Signed-off-by: Peter Krystad <peter.krystad(a)linux.intel.com>
> ---
>  net/mptcp/options.c | 36 +++++++++++++++++++-----------------
>  1 file changed, 19 insertions(+), 17 deletions(-)
> 
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 9b9b9c9390c1..964e73b6b48b 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -13,7 +13,7 @@
>  void mptcp_parse_option(const unsigned char *ptr, int opsize,
>  			struct tcp_options_received *opt_rx)
>  {
> -	struct mptcp_options_received *mp_opt;
> +	struct mptcp_options_received *mp_opt = &opt_rx->mptcp;
>  	u8 subtype = *ptr >> 4;
>  	int expected_opsize;
>  
> @@ -25,25 +25,28 @@ void mptcp_parse_option(const unsigned char *ptr, int opsize,
>  	 * 10-17: Receiver key (optional)
>  	 */
>  	case MPTCPOPT_MP_CAPABLE:
> -		if (opsize != TCPOLEN_MPTCP_MPC_SYN &&
> -		    opsize != TCPOLEN_MPTCP_MPC_SYNACK)
> -			break;
> -
> -		pr_debug("MP_CAPABLE");
> -		opt_rx->mptcp.version = *ptr++ & MPTCPOPT_VERSION_MASK;
> -		if (opt_rx->mptcp.version != 0)
> +		mp_opt->version = *ptr++ & MPTCPOPT_VERSION_MASK;
> +		if (mp_opt->version != 0)
>  			break;

I think we need to check the optsize before accessiong the option data,
or we could end-up reading after the packet end, for
malformed/malicious pkts.

>  
> -		opt_rx->mptcp.mp_capable = 1;
> -		pr_debug("flags=%02x", *ptr);
> -		opt_rx->mptcp.flags = *ptr++;
> -		opt_rx->mptcp.sndr_key = get_unaligned_be64(ptr);
> -		pr_debug("sndr_key=%llu", opt_rx->mptcp.sndr_key);
> +		mp_opt->mp_capable = 1;
> +		mp_opt->flags = *ptr++;
> +		mp_opt->sndr_key = get_unaligned_be64(ptr);
>  		ptr += 8;
> -		if (opsize == TCPOLEN_MPTCP_MPC_SYNACK) {
> -			opt_rx->mptcp.rcvr_key = get_unaligned_be64(ptr);
> -			pr_debug("rcvr_key=%llu", opt_rx->mptcp.rcvr_key);
> +
> +		if (opsize == TCPOLEN_MPTCP_MPC_SYN) {
> +			pr_debug("MP_CAPABLE flags=%x, sndr=%llu",
> +				 mp_opt->flags, mp_opt->sndr_key);
> +		} else if ((opsize == TCPOLEN_MPTCP_MPC_SYNACK) ||
> +			   (opsize == TCPOLEN_MPTCP_MPC_ACK)) {

Since TCPOLEN_MPTCP_MPC_ACK == TCPOLEN_MPTCP_MPC_SYNACK, possibly some
compilers may emit a warning here?!? perhpas we can just drop the
second comparison, with a comment.
 
Thanks,

Paolo