From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1ZwxeD-0004ys-Tz for mharc-grub-devel@gnu.org; Thu, 12 Nov 2015 14:35:05 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37620) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZwxeA-0004vm-Bj for grub-devel@gnu.org; Thu, 12 Nov 2015 14:35:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zwxe6-0004W8-Uw for grub-devel@gnu.org; Thu, 12 Nov 2015 14:35:02 -0500 Received: from mail-lf0-x229.google.com ([2a00:1450:4010:c07::229]:34677) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zwxe6-0004Vs-It for grub-devel@gnu.org; Thu, 12 Nov 2015 14:34:58 -0500 Received: by lffu14 with SMTP id u14so40358284lff.1 for ; Thu, 12 Nov 2015 11:34:58 -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=gO+PLsrYyuZ2rp5fD0YbxiB90TCR1Za1P8/zI5bW7mw=; b=wn242Sq/ZxZMU2HZ3l/JsIO6BB8Xk1Nd8CGsPvEJkttcKIB3VguFQ+Ti5l3t0ApRvZ JcdQqu/fosloYEt3PKc/bW93K2NcWOKQfBqrd3hxReEd5e+pbBRnvmEX4t2u+Te1Lt4E eTxCJR3Sd4nEzNY26ScwETCpS+Wg3IMc6V8mcmJfaZ3iQnMWQa4En3Pa90dvmxABZewM u2o1NkGA2BqTtX4FVyfNZNyxFxJvzqcczUn0e8RWq5WHDse0cI/WRMwLx19VSnWg6bcG tfRYPAp1MlPUrpxSRmTtnKZ9/hYqP+n1JO7R9m/qVje+7hAU5qBnjo5cEy9HdzKfXMT0 /QKw== X-Received: by 10.25.141.77 with SMTP id p74mr8202072lfd.17.1447356897833; Thu, 12 Nov 2015 11:34:57 -0800 (PST) Received: from [192.168.1.41] (ppp91-76-25-247.pppoe.mtu-net.ru. [91.76.25.247]) by smtp.gmail.com with ESMTPSA id e63sm2536869lfe.5.2015.11.12.11.34.56 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 12 Nov 2015 11:34:57 -0800 (PST) Subject: Re: [PATCH] tcp: make GRUB honor TCP MSS option To: The development of GNU GRUB References: From: Andrei Borzenkov Message-ID: <5644E9DB.2080604@gmail.com> Date: Thu, 12 Nov 2015 22:34:51 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-detected-operating-system: by eggs.gnu.org: Error: Malformed IPv6 address (bad octet value). X-Received-From: 2a00:1450:4010:c07::229 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, 12 Nov 2015 19:35:03 -0000 27.10.2015 14:49, Ignat Korchagin пишет: > Currently, GRUB neither parses nor announces TCP MSS option. According > to RFC 879 and its successors if the MSS option is not present the > default TCP segment size is 536. Well ... 536 is max common denominator. If you intend to support larger size, you need to implement path MTU discovery. We may use different MSS depending on whether it is IPv4 or IPv6 though. > That is the exact amount received for every TCP segment for an HTTP > download (tested with Debian Linux), that is the peer TCP stack sends > 536 bytes at a time. Announcing larger TCP segment size makes peer TCP > stack sending larger segments which improves HTTP download speed. > Also, GRUB ignores TCP MSS option received from its peer and > calculating fragment length based only on network MTU. This could > crash potential TCP peers with small memory capacity receiving large > TCP segments when they do not expect them. > > diff --git a/grub-core/net/tcp.c b/grub-core/net/tcp.c > index 1d90f1e..65f9637 100644 > --- a/grub-core/net/tcp.c > +++ b/grub-core/net/tcp.c > @@ -66,6 +66,7 @@ > grub_uint32_t their_start_seq; > grub_uint32_t their_cur_seq; > grub_uint16_t my_window; > + grub_uint16_t their_mss; > struct unacked *unack_first; > struct unacked *unack_last; > grub_err_t (*recv_hook) (grub_net_tcp_socket_t sock, struct > grub_net_buff *nb, > @@ -484,6 +485,37 @@ > grub_priority_queue_destroy (sock->pq); > } > > +static grub_uint16_t > +our_mss (grub_net_tcp_socket_t sock) > +{ > + return (grub_uint16_t)(sock->inf->card->mtu - GRUB_NET_MAX_LINK_HEADER_SIZE > + - GRUB_NET_OUR_MAX_IP_HEADER_SIZE - GRUB_NET_TCP_HEADER_SIZE - 40); Where 40 comes from? > +} > + > +static grub_uint16_t > +get_mss (struct tcphdr *tcph) > +{ > + grub_uint8_t *ptr = (grub_uint8_t *)(tcph + 1); > + > + while (ptr < (grub_uint8_t *)tcph + ((grub_be_to_cpu16 > (tcph->flags) >> 12) * 4)) This results in out of bound access later (you access ptr + 1); also we probably can compute tcp header size just once and pass as argument. > + { > + if (0 == *ptr || 1 == *ptr) In code around it reverse order is used. Could you keep it consistent? Could we please have enum for option codes? > + { > + ptr++; > + continue; That's wrong; option code 0 means end of options so we should break out of loop here. > + } > + > + if (2 == *ptr || 4 == *(ptr + 1)) This should be && otherwise *any option with length 4 will be (mis)interpreted as MSS; or even if (*ptr == 2) if (*(ptr + 1) != 4) some error feedback; > + { > + return grub_be_to_cpu16 (*((grub_uint16_t *)(ptr + 2))); > + } > + > + ptr += *(ptr + 1); > + } > + > + return 536; > +} > + > grub_err_t > grub_net_tcp_accept (grub_net_tcp_socket_t sock, > grub_err_t (*recv_hook) (grub_net_tcp_socket_t sock, > @@ -497,13 +529,14 @@ > { > struct grub_net_buff *nb_ack; > struct tcphdr *tcph; > + grub_uint32_t *mss; > grub_err_t err; > > sock->recv_hook = recv_hook; > sock->error_hook = error_hook; > sock->fin_hook = fin_hook; > sock->hook_data = hook_data; > - nb_ack = grub_netbuff_alloc (sizeof (*tcph) > + nb_ack = grub_netbuff_alloc (sizeof (*tcph) + 4 Please no magic numbers; if struct is feasible, define constant with clear name. or at least some constant with meaningful name. > + GRUB_NET_OUR_MAX_IP_HEADER_SIZE > + GRUB_NET_MAX_LINK_HEADER_SIZE); > if (!nb_ack) > @@ -516,17 +549,19 @@ > return err; > } > > - err = grub_netbuff_put (nb_ack, sizeof (*tcph)); > + err = grub_netbuff_put (nb_ack, sizeof (*tcph) + 4); Same > if (err) > { > grub_netbuff_free (nb_ack); > return err; > } > tcph = (void *) nb_ack->data; > + mss = (grub_uint32_t *)(tcph + 1); > tcph->ack = grub_cpu_to_be32 (sock->their_cur_seq); > - tcph->flags = grub_cpu_to_be16_compile_time ((5 << 12) | TCP_SYN | TCP_ACK); > + tcph->flags = grub_cpu_to_be16_compile_time ((6 << 12) | TCP_SYN | TCP_ACK); Would be good to replace number with macro based on sizeof (struct tcphdr) + option size > tcph->window = grub_cpu_to_be16 (sock->my_window); > tcph->urgent = 0; > + *mss = grub_cpu_to_be32(0x02040000 | our_mss(sock)); > sock->established = 1; > tcp_socket_register (sock); > err = tcp_send (nb_ack, sock); > @@ -556,6 +591,7 @@ > static grub_uint16_t in_port = 21550; > struct grub_net_buff *nb; > struct tcphdr *tcph; > + grub_uint32_t *mss; > int i; > grub_uint8_t *nbd; > grub_net_link_level_address_t ll_target_addr; > @@ -593,7 +629,7 @@ > socket->fin_hook = fin_hook; > socket->hook_data = hook_data; > > - nb = grub_netbuff_alloc (sizeof (*tcph) + 128); > + nb = grub_netbuff_alloc (sizeof (*tcph) + 4 + 128); > if (!nb) > return NULL; > err = grub_netbuff_reserve (nb, 128); > @@ -603,7 +639,7 @@ > return NULL; > } > > - err = grub_netbuff_put (nb, sizeof (*tcph)); > + err = grub_netbuff_put (nb, sizeof (*tcph) + 4); > if (err) > { > grub_netbuff_free (nb); > @@ -617,16 +653,18 @@ > } > > tcph = (void *) nb->data; > + mss = (grub_uint32_t *)(tcph + 1); > socket->my_start_seq = grub_get_time_ms (); > socket->my_cur_seq = socket->my_start_seq + 1; > socket->my_window = 8192; > tcph->seqnr = grub_cpu_to_be32 (socket->my_start_seq); > tcph->ack = grub_cpu_to_be32_compile_time (0); > - tcph->flags = grub_cpu_to_be16_compile_time ((5 << 12) | TCP_SYN); > + tcph->flags = grub_cpu_to_be16_compile_time ((6 << 12) | TCP_SYN); > tcph->window = grub_cpu_to_be16 (socket->my_window); > tcph->urgent = 0; > tcph->src = grub_cpu_to_be16 (socket->in_port); > tcph->dst = grub_cpu_to_be16 (socket->out_port); > + *mss = grub_cpu_to_be32(0x02040000 | our_mss(socket)); Use option symbolic name and constant for size. > tcph->checksum = 0; > tcph->checksum = grub_net_ip_transport_checksum (nb, GRUB_NET_IP_TCP, > &socket->inf->address, > @@ -688,6 +726,9 @@ > - sizeof (*tcph)); > else > fraglen = 1280 - GRUB_NET_OUR_IPV6_HEADER_SIZE; > + > + if (fraglen > socket->their_mss) > + fraglen = socket->their_mss; > > while (nb->tail - nb->data > fraglen) > { > @@ -804,6 +845,7 @@ > { > sock->their_start_seq = grub_be_to_cpu32 (tcph->seqnr); > sock->their_cur_seq = sock->their_start_seq + 1; > + sock->their_mss = get_mss(tcph); > sock->established = 1; > } > > @@ -959,6 +1001,7 @@ > sock->their_cur_seq = sock->their_start_seq + 1; > sock->my_cur_seq = sock->my_start_seq = grub_get_time_ms (); > sock->my_window = 8192; > + sock->their_mss = get_mss(tcph); > > sock->pq = grub_priority_queue_new (sizeof (struct grub_net_buff *), > cmp); > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel >