[MPTCP] Re: [RFC PATCH 1/4] Squash-to: "mptcp: Handle MP_CAPABLE options for outgoing connections"

[MPTCP] Re: [RFC PATCH 1/4] Squash-to: "mptcp: Handle MP_CAPABLE options for outgoing connections"

[Paolo Abeni]

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

On Fri, 2019-12-20 at 19:45 +0100, Paolo Abeni wrote:
> After upstream feedback, I just noticed that we actually
> don't clear the is_mptcp field on clone allocation failure.
> 
> Do that and additionally don't allocate at all the subflow if MPC handshake
> failed.
> 
> Note: we need to update accordingly accept on later patch
> 
> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> ---
>  net/mptcp/subflow.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index b9aca17b0b91..7dd8733dc72a 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -262,23 +262,19 @@ static void subflow_ulp_clone(const struct request_sock *req,
>  	struct mptcp_subflow_context *old_ctx = mptcp_subflow_ctx(newsk);
>  	struct mptcp_subflow_context *new_ctx;
>  
> -	/* newsk->sk_socket is NULL at this point */
> -	new_ctx = subflow_create_ctx(newsk, priority);
> -	if (!new_ctx)
> +	if (!subflow_req->mp_capable ||
> +	    (new_ctx = subflow_create_ctx(newsk, priority)) == NULL) {
> +		tcp_sk(newsk)->is_mptcp = 0;
>  		return;
> +	}
 
We have an additional problem here.

When the MPC handshake and/or allocation fail, we need to clear the
icsk ulp fields, or we will get double free on the old_ctx.

Still trying to clean this thing up...

/P

[MPTCP] Re: [RFC PATCH 1/4] Squash-to: "mptcp: Handle MP_CAPABLE options for outgoing connections"

[Florian Westphal]

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

Paolo Abeni <pabeni(a)redhat.com> wrote:
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -262,23 +262,19 @@ static void subflow_ulp_clone(const struct request_sock *req,
>  	struct mptcp_subflow_context *old_ctx = mptcp_subflow_ctx(newsk);
>  	struct mptcp_subflow_context *new_ctx;
>  
> -	/* newsk->sk_socket is NULL at this point */
> -	new_ctx = subflow_create_ctx(newsk, priority);
> -	if (!new_ctx)
> +	if (!subflow_req->mp_capable ||
> +	    (new_ctx = subflow_create_ctx(newsk, priority)) == NULL) {
> +		tcp_sk(newsk)->is_mptcp = 0;
>  		return;
> +	}

I would prefer to avoid assignment inside a conditional.

Otherwise change looks useful to me.

[MPTCP] Re: [RFC PATCH 1/4] Squash-to: "mptcp: Handle MP_CAPABLE options for outgoing connections"

[Mat Martineau]

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


On Fri, 20 Dec 2019, Paolo Abeni wrote:

> After upstream feedback, I just noticed that we actually
> don't clear the is_mptcp field on clone allocation failure.
>
> Do that and additionally don't allocate at all the subflow if MPC handshake
> failed.
>
> Note: we need to update accordingly accept on later patch
>
> Signed-off-by: Paolo Abeni <pabeni(a)redhat.com>
> ---
> net/mptcp/subflow.c | 20 ++++++++------------
> 1 file changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index b9aca17b0b91..7dd8733dc72a 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -262,23 +262,19 @@ static void subflow_ulp_clone(const struct request_sock *req,
> 	struct mptcp_subflow_context *old_ctx = mptcp_subflow_ctx(newsk);
> 	struct mptcp_subflow_context *new_ctx;
>
> -	/* newsk->sk_socket is NULL at this point */
> -	new_ctx = subflow_create_ctx(newsk, priority);
> -	if (!new_ctx)
> +	if (!subflow_req->mp_capable ||
> +	    (new_ctx = subflow_create_ctx(newsk, priority)) == NULL) {
> +		tcp_sk(newsk)->is_mptcp = 0;
> 		return;
> +	}
>
> 	new_ctx->conn = NULL;

subflow_create_ctx() uses kzalloc, so we can also drop this initialization 
of new_ctx->conn.

> 	new_ctx->conn_finished = 1;
> 	new_ctx->icsk_af_ops = old_ctx->icsk_af_ops;
> -
> -	if (subflow_req->mp_capable) {
> -		new_ctx->mp_capable = 1;
> -		new_ctx->fourth_ack = 1;
> -		new_ctx->remote_key = subflow_req->remote_key;
> -		new_ctx->local_key = subflow_req->local_key;
> -	} else {
> -		tcp_sk(newsk)->is_mptcp = 0;
> -	}
> +	new_ctx->mp_capable = 1;
> +	new_ctx->fourth_ack = 1;
> +	new_ctx->remote_key = subflow_req->remote_key;
> +	new_ctx->local_key = subflow_req->local_key;
> }
>
> static struct tcp_ulp_ops subflow_ulp_ops __read_mostly = {
> -- 
> 2.21.0

--
Mat Martineau
Intel