[MPTCP] Re: [PATCH RFC 07/11] mptcp: subflow: add mptcp_subflow_init_cookie_req helper

[Paolo Abeni <pabeni at redhat.com>]

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

On Tue, 2020-07-14 at 12:32 -0700, Christoph Paasch wrote:
> On 07/14/20 - 20:25, Florian Westphal wrote:
> > Will be used to initialize the mptcp request socket when a
> > MP_CAPABLE request was handled in syncookie mode, i.e. when a
> > TCP ACK containing a MP_CAPABLE option is a valid syncookie value.
> > 
> > Signed-off-by: Florian Westphal <fw(a)strlen.de>
> > ---
> >  include/linux/tcp.h |  1 +
> >  include/net/mptcp.h | 10 +++++++++
> >  net/mptcp/subflow.c | 53 +++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 64 insertions(+)
> > 
> > diff --git a/include/linux/tcp.h b/include/linux/tcp.h
> > index 3bdec31ce8f4..1f706157b551 100644
> > --- a/include/linux/tcp.h
> > +++ b/include/linux/tcp.h
> > @@ -118,6 +118,7 @@ struct tcp_request_sock {
> >  	struct inet_request_sock 	req;
> >  	const struct tcp_request_sock_ops *af_specific;
> >  	u64				snt_synack; /* first SYNACK sent time */
> > +	u8				syncookie:1;
> 
> As this field will only be used in init_req, would it make sense to rather
> pass want_cookie from tcp_conn_request as an argument to the callback?

Otherwise, why don't a 'bool' instead? 

I think it would be nice nothing in the commmit message that this new
field fill a pre-existing one byte hole.
> 
> 
> Christoph
> 
> >  	bool				tfo_listener;
> >  	bool				is_mptcp;
> >  #if IS_ENABLED(CONFIG_MPTCP)
> > diff --git a/include/net/mptcp.h b/include/net/mptcp.h
> > index 985344512f31..653acfe58d04 100644
> > --- a/include/net/mptcp.h
> > +++ b/include/net/mptcp.h
> > @@ -131,6 +131,9 @@ static inline bool mptcp_skb_can_collapse(const struct sk_buff *to,
> >  }
> >  
> >  void mptcp_seq_show(struct seq_file *seq);
> > +int mptcp_subflow_init_cookie_req(struct request_sock *req,
> > +				  const struct sock *sk_listener,
> > +				  struct sk_buff *skb);
> >  #else
> >  
> >  static inline void mptcp_init(void)
> > @@ -204,6 +207,13 @@ static inline bool mptcp_skb_can_collapse(const struct sk_buff *to,
> >  
> >  static inline void mptcp_space(const struct sock *ssk, int *s, int *fs) { }
> >  static inline void mptcp_seq_show(struct seq_file *seq) { }
> > +
> > +static inline int mptcp_subflow_init_cookie_req(struct request_sock *req,
> > +						const struct sock *sk_listener,
> > +						struct sk_buff *skb)
> > +{
> > +	return 0; /* TCP fallback */
> > +}
> >  #endif /* CONFIG_MPTCP */
> >  
> >  #if IS_ENABLED(CONFIG_MPTCP_IPV6)
> > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > index cc9eaec14037..943fa650f1e9 100644
> > --- a/net/mptcp/subflow.c
> > +++ b/net/mptcp/subflow.c
> > @@ -154,6 +154,20 @@ static void subflow_init_req(struct request_sock *req,
> >  			get_random_bytes(&subflow_req->local_key, sizeof(subflow_req->local_key));
> >  		} while (subflow_req->local_key == 0);
> >  
> > +		if (unlikely(tcp_rsk(req)->syncookie)) {
> > +			mptcp_crypto_key_sha(subflow_req->local_key,
> > +					     &subflow_req->token,
> > +					     &subflow_req->idsn);
> > +			if (mptcp_token_get_sock(subflow_req->token)) {

Don't we need to sock_put() the returned sock here?
Perhpas we can use a new, different helper that just check for
existance, without touching the sock refcount? Looks like the sock
reference is not used below.

Thanks!

Paolo