[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3] tcp: add window scaling and RTTM support
From: |
Andrei Borzenkov |
Subject: |
Re: [PATCH v3] tcp: add window scaling and RTTM support |
Date: |
Sat, 13 Feb 2016 18:40:18 +0300 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 |
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;
>
>