[MPTCP] Re: [PATCH RFC 09/11] tcp: handle want_cookie clause via reqsk_put

[MPTCP] Re: [PATCH RFC 09/11] tcp: handle want_cookie clause via reqsk_put

[Paolo Abeni]

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

On Tue, 2020-07-14 at 20:25 +0200, Florian Westphal wrote:
> This will allow the syn_recv_sock callback to keep the request
> socket around even when syncookies are used.
> 
> This will be needed to make MPTCP JOIN requests work in cookie mode.
> 
> When a JOIN request is received, we cannot use cookies because
> we need to remember the peers nonce value for HMAC validation.
> 
> Next patch will handle the cookie+join case by allowing the
> rsk to stay around provided:
>  1. We can find a valid mptcp socket for the 32bit token provided
>     by the join and
>  2. the found mptcp socket doesn't exceed the maximum number of
>     subflows.
> 
> To handle 2) the request socket will not only be accounted with the
> listener but also with the mptcp (parent) socket.
> 
> Signed-off-by: Florian Westphal <fw(a)strlen.de>
> ---
>  net/ipv4/tcp_input.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 5ae612902806..f45ca689bfa7 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -6690,6 +6690,7 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
>  	/* Note: tcp_v6_init_req() might override ir_iif for link locals */
>  	inet_rsk(req)->ir_iif = inet_request_bound_dev_if(sk, skb);
>  
> +	refcount_set(&req->rsk_refcnt, 1);

I could not find easily where sk_refcnt is increment/how is handled
currently/before this patch. Can you please give some pointers?

>  	af_ops->init_req(req, sk, skb);
>  
>  	if (security_inet_conn_request(sk, skb, req))
> @@ -6760,10 +6761,6 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
>  		af_ops->send_synack(sk, dst, &fl, req, &foc,
>  				    !want_cookie ? TCP_SYNACK_NORMAL :
>  						   TCP_SYNACK_COOKIE);
> -		if (want_cookie) {
> -			reqsk_free(req);
> -			return 0;
> -		}
>  	}
>  	reqsk_put(req);

This means all syn cookied req (even plain TCP ones, even on non MPTCP
build) will get an additional atomic decrement, correct?

I fear the overhead could be measurable - even if the testbed is not
trivial at all!

What if we avoid touching this code path, and instead store a - very
limited, possibly only 1 - number of hmacs into the msk?

syn-cookied mp_join will store the hmac there. If the store is full,
the mp_join subflow fails -> reset.

WDYT?

Thanks!

Paolo

[MPTCP] Re: [PATCH RFC 09/11] tcp: handle want_cookie clause via reqsk_put

[Florian Westphal]

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

Paolo Abeni <pabeni(a)redhat.com> wrote:
> On Tue, 2020-07-14 at 20:25 +0200, Florian Westphal wrote:
> > This will allow the syn_recv_sock callback to keep the request
> > socket around even when syncookies are used.
> > 
> > This will be needed to make MPTCP JOIN requests work in cookie mode.
> > 
> > When a JOIN request is received, we cannot use cookies because
> > we need to remember the peers nonce value for HMAC validation.
> > 
> > Next patch will handle the cookie+join case by allowing the
> > rsk to stay around provided:
> >  1. We can find a valid mptcp socket for the 32bit token provided
> >     by the join and
> >  2. the found mptcp socket doesn't exceed the maximum number of
> >     subflows.
> > 
> > To handle 2) the request socket will not only be accounted with the
> > listener but also with the mptcp (parent) socket.
> > 
> > Signed-off-by: Florian Westphal <fw(a)strlen.de>
> > ---
> >  net/ipv4/tcp_input.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> > 
> > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > index 5ae612902806..f45ca689bfa7 100644
> > --- a/net/ipv4/tcp_input.c
> > +++ b/net/ipv4/tcp_input.c
> > @@ -6690,6 +6690,7 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
> >  	/* Note: tcp_v6_init_req() might override ir_iif for link locals */
> >  	inet_rsk(req)->ir_iif = inet_request_bound_dev_if(sk, skb);
> >  
> > +	refcount_set(&req->rsk_refcnt, 1);
> 
> I could not find easily where sk_refcnt is increment/how is handled
> currently/before this patch. Can you please give some pointers?

Its 0 and set to 3 on insert via reqsk_queue_hash_req().

> >  	af_ops->init_req(req, sk, skb);
> >  
> >  	if (security_inet_conn_request(sk, skb, req))
> > @@ -6760,10 +6761,6 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
> >  		af_ops->send_synack(sk, dst, &fl, req, &foc,
> >  				    !want_cookie ? TCP_SYNACK_NORMAL :
> >  						   TCP_SYNACK_COOKIE);
> > -		if (want_cookie) {
> > -			reqsk_free(req);
> > -			return 0;
> > -		}
> >  	}
> >  	reqsk_put(req);
> 
> This means all syn cookied req (even plain TCP ones, even on non MPTCP
> build) will get an additional atomic decrement, correct?

Yes.

> I fear the overhead could be measurable - even if the testbed is not
> trivial at all!

The alternative is to allow ->init_req to say 'I do want the req to be
inserted anyway', e.g. via a bit in req.

> What if we avoid touching this code path, and instead store a - very
> limited, possibly only 1 - number of hmacs into the msk?

Would need to duplicate timeout logic and so on which I wanted to
avoid.

But yes, thats another alternative.

[MPTCP] Re: [PATCH RFC 09/11] tcp: handle want_cookie clause via reqsk_put

[Paolo Abeni]

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

On Mon, 2020-07-27 at 11:00 +0200, Florian Westphal wrote:
> Paolo Abeni <pabeni(a)redhat.com> wrote:
> > On Tue, 2020-07-14 at 20:25 +0200, Florian Westphal wrote:
> > > This will allow the syn_recv_sock callback to keep the request
> > > socket around even when syncookies are used.
> > > 
> > > This will be needed to make MPTCP JOIN requests work in cookie mode.
> > > 
> > > When a JOIN request is received, we cannot use cookies because
> > > we need to remember the peers nonce value for HMAC validation.
> > > 
> > > Next patch will handle the cookie+join case by allowing the
> > > rsk to stay around provided:
> > >  1. We can find a valid mptcp socket for the 32bit token provided
> > >     by the join and
> > >  2. the found mptcp socket doesn't exceed the maximum number of
> > >     subflows.
> > > 
> > > To handle 2) the request socket will not only be accounted with the
> > > listener but also with the mptcp (parent) socket.
> > > 
> > > Signed-off-by: Florian Westphal <fw(a)strlen.de>
> > > ---
> > >  net/ipv4/tcp_input.c | 7 ++-----
> > >  1 file changed, 2 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > > index 5ae612902806..f45ca689bfa7 100644
> > > --- a/net/ipv4/tcp_input.c
> > > +++ b/net/ipv4/tcp_input.c
> > > @@ -6690,6 +6690,7 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
> > >  	/* Note: tcp_v6_init_req() might override ir_iif for link locals */
> > >  	inet_rsk(req)->ir_iif = inet_request_bound_dev_if(sk, skb);
> > >  
> > > +	refcount_set(&req->rsk_refcnt, 1);
> > 
> > I could not find easily where sk_refcnt is increment/how is handled
> > currently/before this patch. Can you please give some pointers?
> 
> Its 0 and set to 3 on insert via reqsk_queue_hash_req().
> 
> > >  	af_ops->init_req(req, sk, skb);
> > >  
> > >  	if (security_inet_conn_request(sk, skb, req))
> > > @@ -6760,10 +6761,6 @@ int tcp_conn_request(struct request_sock_ops *rsk_ops,
> > >  		af_ops->send_synack(sk, dst, &fl, req, &foc,
> > >  				    !want_cookie ? TCP_SYNACK_NORMAL :
> > >  						   TCP_SYNACK_COOKIE);
> > > -		if (want_cookie) {
> > > -			reqsk_free(req);
> > > -			return 0;
> > > -		}
> > >  	}
> > >  	reqsk_put(req);
> > 
> > This means all syn cookied req (even plain TCP ones, even on non MPTCP
> > build) will get an additional atomic decrement, correct?
> 
> Yes.
> 
> > I fear the overhead could be measurable - even if the testbed is not
> > trivial at all!
> 
> The alternative is to allow ->init_req to say 'I do want the req to be
> inserted anyway', e.g. via a bit in req.
> 
> > What if we avoid touching this code path, and instead store a - very
> > limited, possibly only 1 - number of hmacs into the msk?
> 
> Would need to duplicate timeout logic and so on which I wanted to
> avoid.

Yep that would be quite complex. I was wondering about something
simpler, like using with LRU policy a static 4 slots circular buffer
inside the msk (the one identified by the token). WDYT?

/P

[MPTCP] Re: [PATCH RFC 09/11] tcp: handle want_cookie clause via reqsk_put

[Florian Westphal]

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

Paolo Abeni <pabeni(a)redhat.com> wrote:
> On Mon, 2020-07-27 at 11:00 +0200, Florian Westphal wrote:
> > Would need to duplicate timeout logic and so on which I wanted to
> > avoid.
> 
> Yep that would be quite complex. I was wondering about something
> simpler, like using with LRU policy a static 4 slots circular buffer
> inside the msk (the one identified by the token). WDYT?

But where do I get the token to find the msk from the cookie ack?
Its not re-sent in the options afaics, unlike mp_capable case.

[MPTCP] Re: [PATCH RFC 09/11] tcp: handle want_cookie clause via reqsk_put

[Paolo Abeni]

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

On Tue, 2020-07-28 at 01:00 +0200, Florian Westphal wrote:
> Paolo Abeni <pabeni(a)redhat.com> wrote:
> > On Mon, 2020-07-27 at 11:00 +0200, Florian Westphal wrote:
> > > Would need to duplicate timeout logic and so on which I wanted to
> > > avoid.
> > 
> > Yep that would be quite complex. I was wondering about something
> > simpler, like using with LRU policy a static 4 slots circular buffer
> > inside the msk (the one identified by the token). WDYT?
> 
> But where do I get the token to find the msk from the cookie ack?
> Its not re-sent in the options afaics, unlike mp_capable case.

uhm... wasting even more memory we could store in each slot hmac and
token (24 bytes per slot)

MPJ ACK with syncookie we could lookup the slot by hmac.

Not sure about the security implications.

Sounds too overkill?

/P