From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1aUcJE-0008Cu-7p for mharc-grub-devel@gnu.org; Sat, 13 Feb 2016 10:40:32 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49258) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aUcJA-00089f-Vu for grub-devel@gnu.org; Sat, 13 Feb 2016 10:40:30 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aUcJ5-00020X-U6 for grub-devel@gnu.org; Sat, 13 Feb 2016 10:40:28 -0500 Received: from mail-lf0-x241.google.com ([2a00:1450:4010:c07::241]:32847) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aUcJ5-000209-Ge for grub-devel@gnu.org; Sat, 13 Feb 2016 10:40:23 -0500 Received: by mail-lf0-x241.google.com with SMTP id e36so5502540lfi.0 for ; Sat, 13 Feb 2016 07:40:21 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=subject:to:references:from:message-id:date:user-agent:mime-version :in-reply-to:content-type:content-transfer-encoding; bh=1xtaOoCUX8RZH/3V0EQfS+AezbF2ah6VI+gvwqXHlOc=; b=zJDrWdXxxLjH6Dau6QqikRsRIY6N8jxBtEhNcO/Oe7nSDpsdgvdRcWJSXUtxcQCv6t 5cAxYNLM0eKBFUQEwt7Fr1B3iis8LEmc/CabZpbYs/nBJ1fOzm3eE3Zi1AYtAgKtcGfG rmYcMLqXpyMKKyQpzfzRdcJsHI5tlhPVjr+TJF4hEmMGygFDu5l2B1+qZdPXv4EHPFOz En7Zh4YUYr7MceUNp+ZLDhnk4HcTEfL1ICbGA1ZrQTSVNBS2uLZDM65tQUbTH7LO5OXd lFFh0vbypdc5AjugKxODXQZpzWTt01xID399/djy0X7uSFjO4czo/5NmmuFWFtTYRWPA 0mbQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-type :content-transfer-encoding; bh=1xtaOoCUX8RZH/3V0EQfS+AezbF2ah6VI+gvwqXHlOc=; b=mvql+UFoy8Ypr3AWqX2jakjyID9PHB23eKGb+C9nGVbR8tiML3QBj34yMihr3MQKDc igLFZ50+QpYrIMzVg0muHr4//zuDYBDHEm4l09uOgXccExR/Bpa3yw+LrGuaFntA5wPs j35j2WW052QkXFtebFAQZeHldoZniRjH/mkhu3+Oia6w183RoV+qkpo0eJBQ1fYV1g8y Ge4oQOCKQtdRsJLd2EIpucG6B/73EEXnZtYqzMf2hUMg2AxqgCttJvE5bDhkky3mFur0 /NX4owOPg/wEqdjavpA6zdGdZuDsof7BJ8FgFgWAOV+10r/TOW2tqL3McCCsHo3lXDNO ZUkA== X-Gm-Message-State: AG10YORRy8yamaqi/z7ksehiDLycOsMkbSoELoSwDAY/uKQD2xIKvpR0uHPXISTwt7aNyA== X-Received: by 10.25.154.65 with SMTP id c62mr3355887lfe.54.1455378020434; Sat, 13 Feb 2016 07:40:20 -0800 (PST) Received: from [192.168.1.41] (ppp109-252-76-159.pppoe.spdop.ru. [109.252.76.159]) by smtp.gmail.com with ESMTPSA id ot1sm2505527lbb.26.2016.02.13.07.40.18 (version=TLSv1/SSLv3 cipher=OTHER); Sat, 13 Feb 2016 07:40:19 -0800 (PST) Subject: Re: [PATCH v3] tcp: add window scaling and RTTM support To: Josef Bacik , grub-devel@gnu.org, kernel-team@fb.com References: <1454351457-496174-1-git-send-email-jbacik@fb.com> From: Andrei Borzenkov X-Enigmail-Draft-Status: N1110 Message-ID: <56BF4E62.1040508@gmail.com> 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 MIME-Version: 1.0 In-Reply-To: <1454351457-496174-1-git-send-email-jbacik@fb.com> Content-Type: text/plain; charset=windows-1251 Content-Transfer-Encoding: 8bit X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 2a00:1450:4010:c07::241 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: The development of GNU GRUB List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 13 Feb 2016 15:40:30 -0000 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; > >