Re: [PATCH] tcp: make GRUB honor TCP MSS option

[Andrei Borzenkov <arvidjaar@gmail.com>]

27.10.2015 14:49, Ignat Korchagin пишет:
> Currently, GRUB neither parses nor announces TCP MSS option. According
> to RFC 879 and its successors if the MSS option is not present the
> default TCP segment size is 536.

Well ... 536 is max common denominator. If you intend to support larger 
size, you need to implement path MTU discovery.

We may use different MSS depending on whether it is IPv4 or IPv6 though.

> That is the exact amount received for every TCP segment for an HTTP
> download (tested with Debian Linux), that is the peer TCP stack sends
> 536 bytes at a time. Announcing larger TCP segment size makes peer TCP
> stack sending larger segments which improves HTTP download speed.
> Also, GRUB ignores TCP MSS option received from its peer and
> calculating fragment length based only on network MTU. This could
> crash potential TCP peers with small memory capacity receiving large
> TCP segments when they do not expect them.
>
> diff --git a/grub-core/net/tcp.c b/grub-core/net/tcp.c
> index 1d90f1e..65f9637 100644
> --- a/grub-core/net/tcp.c
> +++ b/grub-core/net/tcp.c
> @@ -66,6 +66,7 @@
>     grub_uint32_t their_start_seq;
>     grub_uint32_t their_cur_seq;
>     grub_uint16_t my_window;
> +  grub_uint16_t their_mss;
>     struct unacked *unack_first;
>     struct unacked *unack_last;
>     grub_err_t (*recv_hook) (grub_net_tcp_socket_t sock, struct
> grub_net_buff *nb,
> @@ -484,6 +485,37 @@
>     grub_priority_queue_destroy (sock->pq);
>   }
>
> +static grub_uint16_t
> +our_mss (grub_net_tcp_socket_t sock)
> +{
> +  return (grub_uint16_t)(sock->inf->card->mtu - GRUB_NET_MAX_LINK_HEADER_SIZE
> +       - GRUB_NET_OUR_MAX_IP_HEADER_SIZE - GRUB_NET_TCP_HEADER_SIZE - 40);

Where 40 comes from?

> +}
> +
> +static grub_uint16_t
> +get_mss (struct tcphdr *tcph)
> +{
> +  grub_uint8_t *ptr = (grub_uint8_t *)(tcph + 1);
> +
> +  while (ptr < (grub_uint8_t *)tcph + ((grub_be_to_cpu16
> (tcph->flags) >> 12) * 4))

This results in out of bound access later (you access ptr + 1); also we 
probably can compute tcp header size just once and pass as argument.

> +    {
> +      if (0 == *ptr || 1 == *ptr)

In code around it reverse order is used. Could you keep it consistent?

Could we please have enum for option codes?

> +        {
> +          ptr++;
> +          continue;

That's wrong; option code 0 means end of options so we should break out 
of loop here.

> +        }
> +
> +      if (2 == *ptr || 4 == *(ptr + 1))

This should be && otherwise *any option with length 4 will be 
(mis)interpreted as MSS; or even

if (*ptr == 2)
   if (*(ptr + 1) != 4)
     some error feedback;


> +        {
> +          return grub_be_to_cpu16 (*((grub_uint16_t *)(ptr + 2)));
> +        }
> +
> +      ptr += *(ptr + 1);
> +    }
> +
> +  return 536;
> +}
> +
>   grub_err_t
>   grub_net_tcp_accept (grub_net_tcp_socket_t sock,
>        grub_err_t (*recv_hook) (grub_net_tcp_socket_t sock,
> @@ -497,13 +529,14 @@
>   {
>     struct grub_net_buff *nb_ack;
>     struct tcphdr *tcph;
> +  grub_uint32_t *mss;
>     grub_err_t err;
>
>     sock->recv_hook = recv_hook;
>     sock->error_hook = error_hook;
>     sock->fin_hook = fin_hook;
>     sock->hook_data = hook_data;
> -  nb_ack = grub_netbuff_alloc (sizeof (*tcph)
> +  nb_ack = grub_netbuff_alloc (sizeof (*tcph) + 4

Please no magic numbers; if struct is feasible, define constant with 
clear name. or at least some constant with meaningful name.

>          + GRUB_NET_OUR_MAX_IP_HEADER_SIZE
>          + GRUB_NET_MAX_LINK_HEADER_SIZE);
>     if (!nb_ack)
> @@ -516,17 +549,19 @@
>         return err;
>       }
>
> -  err = grub_netbuff_put (nb_ack, sizeof (*tcph));
> +  err = grub_netbuff_put (nb_ack, sizeof (*tcph) + 4);

Same

>     if (err)
>       {
>         grub_netbuff_free (nb_ack);
>         return err;
>       }
>     tcph = (void *) nb_ack->data;
> +  mss = (grub_uint32_t *)(tcph + 1);
>     tcph->ack = grub_cpu_to_be32 (sock->their_cur_seq);
> -  tcph->flags = grub_cpu_to_be16_compile_time ((5 << 12) | TCP_SYN | TCP_ACK);
> +  tcph->flags = grub_cpu_to_be16_compile_time ((6 << 12) | TCP_SYN | TCP_ACK);

Would be good to replace number with macro based on sizeof (struct 
tcphdr) + option size

>     tcph->window = grub_cpu_to_be16 (sock->my_window);
>     tcph->urgent = 0;
> +  *mss = grub_cpu_to_be32(0x02040000 | our_mss(sock));
>     sock->established = 1;
>     tcp_socket_register (sock);
>     err = tcp_send (nb_ack, sock);
> @@ -556,6 +591,7 @@
>     static grub_uint16_t in_port = 21550;
>     struct grub_net_buff *nb;
>     struct tcphdr *tcph;
> +  grub_uint32_t *mss;
>     int i;
>     grub_uint8_t *nbd;
>     grub_net_link_level_address_t ll_target_addr;
> @@ -593,7 +629,7 @@
>     socket->fin_hook = fin_hook;
>     socket->hook_data = hook_data;
>
> -  nb = grub_netbuff_alloc (sizeof (*tcph) + 128);
> +  nb = grub_netbuff_alloc (sizeof (*tcph) + 4 + 128);
>     if (!nb)
>       return NULL;
>     err = grub_netbuff_reserve (nb, 128);
> @@ -603,7 +639,7 @@
>         return NULL;
>       }
>
> -  err = grub_netbuff_put (nb, sizeof (*tcph));
> +  err = grub_netbuff_put (nb, sizeof (*tcph) + 4);
>     if (err)
>       {
>         grub_netbuff_free (nb);
> @@ -617,16 +653,18 @@
>       }
>
>     tcph = (void *) nb->data;
> +  mss = (grub_uint32_t *)(tcph + 1);
>     socket->my_start_seq = grub_get_time_ms ();
>     socket->my_cur_seq = socket->my_start_seq + 1;
>     socket->my_window = 8192;
>     tcph->seqnr = grub_cpu_to_be32 (socket->my_start_seq);
>     tcph->ack = grub_cpu_to_be32_compile_time (0);
> -  tcph->flags = grub_cpu_to_be16_compile_time ((5 << 12) | TCP_SYN);
> +  tcph->flags = grub_cpu_to_be16_compile_time ((6 << 12) | TCP_SYN);
>     tcph->window = grub_cpu_to_be16 (socket->my_window);
>     tcph->urgent = 0;
>     tcph->src = grub_cpu_to_be16 (socket->in_port);
>     tcph->dst = grub_cpu_to_be16 (socket->out_port);
> +  *mss = grub_cpu_to_be32(0x02040000 | our_mss(socket));

Use option symbolic name and constant for size.

>     tcph->checksum = 0;
>     tcph->checksum = grub_net_ip_transport_checksum (nb, GRUB_NET_IP_TCP,
>      &socket->inf->address,
> @@ -688,6 +726,9 @@
>          - sizeof (*tcph));
>     else
>       fraglen = 1280 - GRUB_NET_OUR_IPV6_HEADER_SIZE;
> +
> +  if (fraglen > socket->their_mss)
> +    fraglen = socket->their_mss;
>
>     while (nb->tail - nb->data > fraglen)
>       {
> @@ -804,6 +845,7 @@
>         {
>    sock->their_start_seq = grub_be_to_cpu32 (tcph->seqnr);
>    sock->their_cur_seq = sock->their_start_seq + 1;
> + sock->their_mss = get_mss(tcph);
>    sock->established = 1;
>         }
>
> @@ -959,6 +1001,7 @@
>    sock->their_cur_seq = sock->their_start_seq + 1;
>    sock->my_cur_seq = sock->my_start_seq = grub_get_time_ms ();
>    sock->my_window = 8192;
> + sock->their_mss = get_mss(tcph);
>
>    sock->pq = grub_priority_queue_new (sizeof (struct grub_net_buff *),
>       cmp);
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>