Re: [PATCH net-next v1] net: mptcp, Fast Open Mechanism

[Matthieu Baerts <matthieu.baerts@tessares.net>]

Hi Dmytro,

(without netdev ML and net maintainers)

On 22/10/2021 07:15, Dmytro Shytyi wrote:
> This set of patches will bring "Fast Open" Option support to MPTCP.
> The aim of Fast Open Mechanism is to eliminate one round trip 
> time from a TCP conversation by allowing data to be included as 
> part of the SYN segment that initiates the connection. 
> 
> IETF RFC 8684: Appendix B.  TCP Fast Open and MPTCP.
> 
> [PATCH v1] includes "client" partial support for :
> 1. send request for cookie;
> 2. send syn+data+cookie.
> 
> Signed-off-by: Dmytro Shytyi <dmytro@shytyi.net>

Again, thank you for looking at that!

Here is a very quick review about the code style, just to get that out
from under our feet and not being distracted by that later ;-)
This review is not about the "logic" behind, more about how the code
should look like.

> ---
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index cd6b11c9b54d..1f9ef060e980 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1686,6 +1686,68 @@ static void mptcp_set_nospace(struct sock *sk)
>         set_bit(MPTCP_NOSPACE, &mptcp_sk(sk)->flags);

It looks like the patch has not been formatting properly: we can see all
tabs have been converted to spaces.
The documentation about how to submit patches [1] is quite long but most
of the time these two commands can be enough to send one patch to MPTCP ML:

  $ git format-patch --subject-prefix="PATCH mptcp-next" -v2 -1
  $ git send-email --annotate --to "mptcp@lists.linux.dev" <file|dir>

[1] https://www.kernel.org/doc/html/latest/process/submitting-patches.html

>  }
> 
> +static int mptcp_sendmsg_fastopen_cookie_req(struct sock *sk, struct msghdr *msg,
> +                                            size_t *copied, size_t size,
> +                                            struct ubuf_info *uarg)
> +{
> +       struct mptcp_sock *msk = mptcp_sk(sk);
> +       struct socket *ssk = __mptcp_nmpc_socket(msk);

For variables declaration in the net tree, we have to follow the
"reversed xmas tree" order: ordered by char size, longest lines first.
So you need to flip the two lines above. Same for the two lines below
and in the next function.

Sadly, checkpatch doesn't warn you about that.

> +       struct tcp_sock *tp = tcp_sk(ssk->sk);
> +       struct sockaddr *uaddr = msg->msg_name;
> +       struct tcp_fastopen_context *ctx;
> +       const struct iphdr *iph;
> +       struct sk_buff *skb;
> +       int err;
> +
> +       skb = sk_stream_alloc_skb(ssk->sk, 0, ssk->sk->sk_allocation, true);
> +       iph = ip_hdr(skb);
> +       tcp_fastopen_init_key_once(sock_net(ssk->sk));
> +       ctx = tcp_fastopen_get_ctx(ssk->sk);

It looks like you don't use it. Does the compiler not complain about that?

> +       tp->fastopen_req = kzalloc(sizeof(*tp->fatopen_req),
> +                                  ssk->sk->sk_allocation);

After something got allocated, you need to check if you got what you
wanted. If not, you need to revert some actions, usually done at the end
of a function with goto: you can look around in this file for some examples.
In general, you need to look for all possible errors if the function you
call can return some.

Also, Do not hesitate to create "logical blocks" of code separated by
empty lines, e.g. assigning values in tp->fastopen_req can be done in
one block, the 'return' part in another one, allocating a new skb in
another one, etc. Do not hesitate to look at how it is done around.

> +       tp->fastopen_req->data = msg;
> +       tp->fastopen_req->size = size;
> +       tp->fastopen_req->uarg = uarg;
> +       err = mptcp_stream_connect(sk->sk_socket, uaddr, msg->msg_namelen, msg->msg_flags);
> +       return err;

Maybe clearer not to use 'err' if you don't do anything special with it.

> +}
> +
> +static int mptcp_sendmsg_fastopen_cookie_send(struct sock *sk, struct msghdr *msg,
> +                                             size_t *copied, size_t size,
> +                                             struct ubuf_info *uarg)
> +{
> +       struct tcp_fastopen_cookie *fastopen_cookie = kmalloc(sizeof(*fastopen_cookie),
> +                                                             GFP_KERNEL);
> +       struct mptcp_sock *msk = mptcp_sk(sk);
> +       struct socket *ssk = __mptcp_nmpc_socket(msk);
> +       struct tcp_sock *tp = tcp_sk(ssk->sk);
> +       struct sockaddr *uaddr = msg->msg_name;
> +       struct tcp_fastopen_context *ctx;
> +       const struct iphdr *iph;
> +       struct sk_buff *skb;
> +       int err;
> +
> +       skb = sk_stream_alloc_skb(ssk->sk, 0, ssk->sk->sk_allocation, true);
> +       iph = ip_hdr(skb);
> +       tcp_fastopen_init_key_once(sock_net(ssk->sk));
> +       ctx = tcp_fastopen_get_ctx(ssk->sk);
> +
> +       fastopen_cookie->val[0] = cpu_to_le64(siphash(&iph->saddr,
> +                                             sizeof(iph->saddr) +
> +                                             sizeof(iph->daddr),
> +                                             &ctx->key[0]));
> +       fastopen_cookie->len = TCP_FASTOPEN_COOKIE_SIZE;
> +
> +       tp->fastopen_req = kzalloc(sizeof(*tp->fastopen_req),
> +                                  ssk->sk->sk_allocation);
> +       tp->fastopen_req->data = msg;
> +       tp->fastopen_req->size = size;
> +       tp->fastopen_req->uarg = uarg;
> +       memcpy(&tp->fastopen_req->cookie, fastopen_cookie, sizeof(tp->fastopen_req->cookie));
> +       err = mptcp_stream_connect(sk->sk_socket, uaddr, msg->msg_namelen, msg->msg_flags);
> +       return err;

(Most of the comments above apply to the code here too. Also, please
check if it is not possible to avoid code duplication by creating other
static functions.)

> +}
> +
>  static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>  {
>         struct mptcp_sock *msk = mptcp_sk(sk);
> @@ -1694,9 +1756,22 @@ static int mptcp_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>         int ret = 0;
>         long timeo;
> 
> -       /* we don't support FASTOPEN yet */
> -       if (msg->msg_flags & MSG_FASTOPEN)
> -               return -EOPNOTSUPP;
> +       /* we don't fully support FASTOPEN yet */
> +
> +       if (msg->msg_flags & MSG_FASTOPEN) {
> +               struct socket *ssk = __mptcp_nmpc_socket(msk);
> +               struct tcp_sock *tp = tcp_sk(ssk->sk);
> +
> +               if (tp && tp->fastopen_req && tp->fastopen_req->cookie.len != 0) {
> +                       // send cookie

C++-style comments are usually not allowed in the kernel. I thought
checkpatch was saying something about that but maybe it doesn't :)

> +                       ret = mptcp_sendmsg_fastopen_cookie_send(sk, msg, &copied, len, uarg);
> +               } else {
> +                       struct tcp_fastopen_request *fastopen = tp->fastopen_req;
> +                       //requests a cookie
> +                       ret = mptcp_sendmsg_fastopen_cookie_req(sk, msg, &copied, len, uarg);
> +               }
> +       return ret;

The alignement is not OK here.
Please also add a new line before to clearly identify the 'return' part
after the 'if/else'.

I hope this might help reviewing the new versions ;-)

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net