Re: [PATCH v3] tcp: add window scaling and RTTM support

[Andrei Borzenkov <arvidjaar@gmail.com>]

01.02.2016 21:30, Josef Bacik пишет:
...

> @@ -745,6 +817,7 @@ grub_net_recv_tcp_packet (struct grub_net_buff *nb,
>  {
>    struct tcphdr *tcph;
>    grub_net_tcp_socket_t sock;
> +  grub_uint32_t tsecr = 0;
>    grub_err_t err;
>  
>    /* Ignore broadcast.  */
> @@ -771,6 +844,38 @@ grub_net_recv_tcp_packet (struct grub_net_buff *nb,
>        return GRUB_ERR_NONE;
>      }
>  

Still no proper boundary check.

> +  /* If the packet is large enough to have the timestamp opt then lets look for
> +     the tsecr value. */
> +  if ((grub_be_to_cpu16 (tcph->flags >> 12) * sizeof (grub_uint32_t)) >=
> +      ALIGN_UP (sizeof (struct tcphdr) + sizeof (struct tcp_timestamp_opt), 4))
> +    {
> +      struct tcp_opt_hdr *opt;
> +      grub_size_t remaining = nb->tail - nb->data;
> +
> +      opt = (struct tcp_opt_hdr *)(tcph + 1);
> +      while (remaining > 0)
> +	{
> +	  grub_uint8_t len = 1;
> +	  if (opt->kind == 8 || opt->kind == 0)
> +	    break;
> +	  if (opt->kind > 1)

Here (we only can ensure opt->kind here)

> +	    len = opt->length;
> +	  if (len > remaining)
> +	    len = remaining;
> +	  remaining -= len;
> +	  opt = (struct tcp_opt_hdr *)((grub_uint8_t *)opt + len);
> +	}
> +
> +      /* Ok we definitely have the timestamp option. */
> +      if (opt->kind == 8)
> +	{
> +	  struct tcp_timestamp_opt *timestamp;
> +
> +	  timestamp = (struct tcp_timestamp_opt *)opt;
> +	  tsecr = grub_be_to_cpu32 (timestamp->tsval);

And of course here (there is no length check at all when we break out of
loop).

> +	}
> +    }
> +
>    FOR_TCP_SOCKETS (sock)
>    {
>      if (!(grub_be_to_cpu16 (tcph->dst) == sock->in_port
> @@ -805,6 +910,9 @@ grub_net_recv_tcp_packet (struct grub_net_buff *nb,
>  	sock->their_start_seq = grub_be_to_cpu32 (tcph->seqnr);
>  	sock->their_cur_seq = sock->their_start_seq + 1;
>  	sock->established = 1;
> +	sock->timestamp_supported = 0;
> +	if (tsecr)
> +	  sock->timestamp_supported = 1;
>        }
>  
>      if (grub_be_to_cpu16 (tcph->flags) & TCP_RST)
> @@ -906,6 +1014,8 @@ grub_net_recv_tcp_packet (struct grub_net_buff *nb,
>  	      return err;
>  	    }
>  
> +	  /* We only update the tsecr when we advance the window. */
> +	  sock->cur_tsecr = tsecr;

You intended to check that tsecr >= cur_tsecr.

>  	  sock->their_cur_seq += (nb_top->tail - nb_top->data);
>  	  if (grub_be_to_cpu16 (tcph->flags) & TCP_FIN)
>  	    {
> @@ -999,3 +1109,63 @@ grub_net_tcp_unstall (grub_net_tcp_socket_t sock)
>    sock->i_stall = 0;
>    ack (sock);
>  }
> +
> +static const char *
> +window_get_env (struct grub_env_var *var __attribute__ ((unused)),
> +		const char *val __attribute__ ((unused)))
> +{
> +  return grub_net_tcp_window_size;
> +}
> +

Oh, that's really redundant as is, string is stored as variable value
anyway, no need to duplicate it, just return from window_set_env.

> +static char *
> +window_set_env (struct grub_env_var *var __attribute__ ((unused)),
> +		const char *val)
> +{
> +  grub_uint32_t ret;
> +
> +  if (val == NULL)
> +    return NULL;
> +
> +  grub_error_push ();
> +  ret = (grub_uint32_t) grub_strtoul (val, 0, 0);
> +  if (grub_errno != GRUB_ERR_NONE)
> +    {
> +      grub_printf ("Invalid number for window size '%s'.\n", val);
> +      grub_errno = GRUB_ERR_NONE;
> +      grub_error_pop ();

You just lost proper error return from grub_strtoul. Just return NULL
here without any push/pop and error from strtoul will be correctly
propagated and printed when set command completes.

> +      return NULL;
> +    }
> +  grub_error_pop ();
> +
> +  /* A window size greater than 1gib is invalid. */
> +  if (ret > 1024 * 1024 * 1024)
> +    {
> +      grub_printf ("TCP window size must be <= 1gib.\n");

Please use grub_error to set grub_errno and error string;
GRUB_ERR_BAD_ARGUMENT looks suitable here.

> +      return NULL;
> +    }
> +  grub_net_tcp_window_size = grub_strdup (val);
> +  tcp_window_size = ret;
> +  tcp_window_scale = 0;
> +
> +  /* The window size is only 16 bits long, so we have to scale it down to fit in
> +     the header and calculate the scale along the way. */
> +  while (tcp_window_size > 65535)
> +    {
> +      tcp_window_size >>= 1;
> +      tcp_window_scale += 1;
> +    }
> +
> +  return grub_net_tcp_window_size;
> +}
> +
> +/* We set the default window size to 1mib. */
> +#define DEFAULT_TCP_WINDOW_SIZE "1048576"

Well ... I'm still unsure we should do it by default.

> +
> +void
> +grub_net_tcp_init (void)
> +{
> +  grub_register_variable_hook ("net_tcp_window_size", window_get_env,
> +			       window_set_env);
> +  grub_env_export ("net_tcp_window_size");
> +  grub_env_set ("net_tcp_window_size", DEFAULT_TCP_WINDOW_SIZE);
> +}
> diff --git a/include/grub/net.h b/include/grub/net.h
> index 4571b72..fa3d286 100644
> --- a/include/grub/net.h
> +++ b/include/grub/net.h
> @@ -551,6 +551,8 @@ grub_net_add_dns_server (const struct grub_net_network_level_address *s);
>  void
>  grub_net_remove_dns_server (const struct grub_net_network_level_address *s);
>  
> +void
> +grub_net_tcp_init (void);
>  
>  extern char *grub_net_default_server;
>  
>