grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Josef Bacik
Subject: Re: [PATCH v3] tcp: add window scaling and RTTM support
Date: Thu, 18 Feb 2016 14:20:55 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0

On 02/13/2016 10:40 AM, Andrei Borzenkov wrote:
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.

Sorry I just now noticed you replied. I'll fix up the rest of the stuff. And apparently we're the only ones doing netbooting so I feel like we get to pick the default ;). Thanks,

Josef




reply via email to

[Prev in Thread] Current Thread [Next in Thread]