From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1aWU8W-0001bh-Lt for mharc-grub-devel@gnu.org; Thu, 18 Feb 2016 14:21:12 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60899) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aWU8T-0001bN-CS for grub-devel@gnu.org; Thu, 18 Feb 2016 14:21:10 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aWU8Q-000245-58 for grub-devel@gnu.org; Thu, 18 Feb 2016 14:21:09 -0500 Received: from mx0a-00082601.pphosted.com ([67.231.145.42]:29211) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aWU8P-00023h-TH for grub-devel@gnu.org; Thu, 18 Feb 2016 14:21:06 -0500 Received: from pps.filterd (m0044010.ppops.net [127.0.0.1]) by mx0a-00082601.pphosted.com (8.15.0.59/8.15.0.59) with SMTP id u1IJJYqg007247; Thu, 18 Feb 2016 11:21:02 -0800 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=fb.com; h=subject : to : references : from : message-id : date : mime-version : in-reply-to : content-type : content-transfer-encoding; s=facebook; bh=jt0TQPXukeqggcUrg/1dugDZBkdWit57Q8AExOPNY9Q=; b=ERt6M1ZFcYfO4OlEnrjYlhF5Z2o9dJ0fvFPF2TfULZ7cKR4ePtXWGVQMUFzqLzlmcR1x uKjjKMim9R1nOQ8OzHUWqSWgMshJ+dmo3LVvSMvzYAUhdf0abamhT2IcAh/SkDHpL986 GagN3Zjk4o8CRq2ltsECt5GSnjosWRVUQGg= Received: from mail.thefacebook.com ([199.201.64.23]) by mx0a-00082601.pphosted.com with ESMTP id 215jxwrbb7-1 (version=TLSv1/SSLv3 cipher=AES128-SHA bits=128 verify=NOT); Thu, 18 Feb 2016 11:21:02 -0800 Received: from localhost.localdomain (192.168.54.13) by mail.thefacebook.com (192.168.16.16) with Microsoft SMTP Server (TLS) id 14.3.248.2; Thu, 18 Feb 2016 11:20:57 -0800 Subject: Re: [PATCH v3] tcp: add window scaling and RTTM support To: Andrei Borzenkov , , References: <1454351457-496174-1-git-send-email-jbacik@fb.com> <56BF4E62.1040508@gmail.com> From: Josef Bacik Message-ID: <56C61997.7090006@fb.com> 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 MIME-Version: 1.0 In-Reply-To: <56BF4E62.1040508@gmail.com> Content-Type: text/plain; charset="windows-1251"; format=flowed X-Originating-IP: [192.168.54.13] X-Proofpoint-Spam-Reason: safe X-FB-Internal: Safe X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:, , definitions=2016-02-18_08:, , signatures=0 Content-Transfer-Encoding: quoted-printable X-MIME-Autoconverted: from 8bit to quoted-printable by mx0a-00082601.pphosted.com id u1IJJYqg007247 X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x [generic] X-Received-From: 67.231.145.42 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: Thu, 18 Feb 2016 19:21:10 -0000 On 02/13/2016 10:40 AM, Andrei Borzenkov wrote: > 01.02.2016 21:30, Josef Bacik =EF=E8=F8=E5=F2: > ... > >> @@ -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 =3D 0; >> grub_err_t err; >> >> /* Ignore broadcast. */ >> @@ -771,6 +844,38 @@ grub_net_recv_tcp_packet (struct grub_net_buff *n= b, >> return GRUB_ERR_NONE; >> } >> > > Still no proper boundary check. > >> + /* If the packet is large enough to have the timestamp opt then let= s look for >> + the tsecr value. */ >> + if ((grub_be_to_cpu16 (tcph->flags >> 12) * sizeof (grub_uint32_t))= >=3D >> + ALIGN_UP (sizeof (struct tcphdr) + sizeof (struct tcp_timestamp= _opt), 4)) >> + { >> + struct tcp_opt_hdr *opt; >> + grub_size_t remaining =3D nb->tail - nb->data; >> + >> + opt =3D (struct tcp_opt_hdr *)(tcph + 1); >> + while (remaining > 0) >> + { >> + grub_uint8_t len =3D 1; >> + if (opt->kind =3D=3D 8 || opt->kind =3D=3D 0) >> + break; >> + if (opt->kind > 1) > > Here (we only can ensure opt->kind here) > >> + len =3D opt->length; >> + if (len > remaining) >> + len =3D remaining; >> + remaining -=3D len; >> + opt =3D (struct tcp_opt_hdr *)((grub_uint8_t *)opt + len); >> + } >> + >> + /* Ok we definitely have the timestamp option. */ >> + if (opt->kind =3D=3D 8) >> + { >> + struct tcp_timestamp_opt *timestamp; >> + >> + timestamp =3D (struct tcp_timestamp_opt *)opt; >> + tsecr =3D grub_be_to_cpu32 (timestamp->tsval); > > And of course here (there is no length check at all when we break out o= f > loop). > >> + } >> + } >> + >> FOR_TCP_SOCKETS (sock) >> { >> if (!(grub_be_to_cpu16 (tcph->dst) =3D=3D sock->in_port >> @@ -805,6 +910,9 @@ grub_net_recv_tcp_packet (struct grub_net_buff *nb= , >> sock->their_start_seq =3D grub_be_to_cpu32 (tcph->seqnr); >> sock->their_cur_seq =3D sock->their_start_seq + 1; >> sock->established =3D 1; >> + sock->timestamp_supported =3D 0; >> + if (tsecr) >> + sock->timestamp_supported =3D 1; >> } >> >> if (grub_be_to_cpu16 (tcph->flags) & TCP_RST) >> @@ -906,6 +1014,8 @@ grub_net_recv_tcp_packet (struct grub_net_buff *n= b, >> return err; >> } >> >> + /* We only update the tsecr when we advance the window. */ >> + sock->cur_tsecr =3D tsecr; > > You intended to check that tsecr >=3D cur_tsecr. > >> sock->their_cur_seq +=3D (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 =3D 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 =3D=3D NULL) >> + return NULL; >> + >> + grub_error_push (); >> + ret =3D (grub_uint32_t) grub_strtoul (val, 0, 0); >> + if (grub_errno !=3D GRUB_ERR_NONE) >> + { >> + grub_printf ("Invalid number for window size '%s'.\n", val); >> + grub_errno =3D 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 <=3D 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 =3D grub_strdup (val); >> + tcp_window_size =3D ret; >> + tcp_window_scale =3D 0; >> + >> + /* The window size is only 16 bits long, so we have to scale it dow= n to fit in >> + the header and calculate the scale along the way. */ >> + while (tcp_window_size > 65535) >> + { >> + tcp_window_size >>=3D 1; >> + tcp_window_scale +=3D 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=20 stuff. And apparently we're the only ones doing netbooting so I feel=20 like we get to pick the default ;). Thanks, Josef