[PATCH 1/6] net: split IP_TCP header into separate IP/IP6 and TCP headers

Dmitrii Merkurev dimorinny at google.com
Wed May 10 19:11:45 CEST 2023


Hey Paul,

Huge thanks for pointing this out. Uploaded v2 with a couple of fixes
(including wget test on 1/6).

On Mon, May 8, 2023 at 10:18 PM Paul Liu <paul.liu at linaro.org> wrote:
>
> Hi Dmitrii,
>
> It seems to me that this series of patches breaks the unittest of TCP stack in sandbox.
>
> I'll see if I have some time to fix the unittest too.
>
> The unittest can be built by the following commands.
>  1. "make sandbox64_defconfig"
>  2. turn on the options that need to be tested. For example, CMD_WGET
>  3. "make"
>  4. "sudo ./u-boot -D"
>  5. "ut lib net_test_wget" in the U-boot prompt.
>
> Yours,
> Paul
>
>
> On Tue, 9 May 2023 at 01:15, Dmitrii Merkurev <dimorinny at google.com> wrote:
>>
>> This allows us to reuse TCP logic between IP and IP6 stack.
>>
>> Signed-off-by: Dmitrii Merkurev <dimorinny at google.com>
>> Cc: Ying-Chun Liu (PaulLiu) <paul.liu at linaro.org>
>> Cc: Simon Glass <sjg at chromium.org>
>> Сс: Joe Hershberger <joe.hershberger at ni.com>
>> Сс: Ramon Fried <rfried.dev at gmail.com>
>> ---
>>  include/net/tcp.h | 50 ++++++++++++++-------------------
>>  net/tcp.c         | 70 +++++++++++++++++++++++------------------------
>>  test/cmd/wget.c   | 16 +++++------
>>  3 files changed, 64 insertions(+), 72 deletions(-)
>>
>> diff --git a/include/net/tcp.h b/include/net/tcp.h
>> index c29d4ce24a..5ab1127d8b 100644
>> --- a/include/net/tcp.h
>> +++ b/include/net/tcp.h
>> @@ -5,20 +5,16 @@
>>   * Copyright 2017 Duncan Hare, All rights reserved.
>>   */
>>
>> +#ifndef __TCP_H__
>> +#define __TCP_H__
>> +
>>  #define TCP_ACTIVITY 127               /* Number of packets received   */
>>                                         /* before console progress mark */
>> +
>> +#define GET_TCP_HDR_LEN_IN_BYTES(x) ((x) >> 2)
>> +
>>  /**
>> - * struct ip_tcp_hdr - IP and TCP header
>> - * @ip_hl_v: header length and version
>> - * @ip_tos: type of service
>> - * @ip_len: total length
>> - * @ip_id: identification
>> - * @ip_off: fragment offset field
>> - * @ip_ttl: time to live
>> - * @ip_p: protocol
>> - * @ip_sum: checksum
>> - * @ip_src: Source IP address
>> - * @ip_dst: Destination IP address
>> + * struct tcp_hdr - TCP header
>>   * @tcp_src: TCP source port
>>   * @tcp_dst: TCP destination port
>>   * @tcp_seq: TCP sequence number
>> @@ -28,18 +24,8 @@
>>   * @tcp_win: TCP windows size
>>   * @tcp_xsum: Checksum
>>   * @tcp_ugr: Pointer to urgent data
>> - */
>> -struct ip_tcp_hdr {
>> -       u8              ip_hl_v;
>> -       u8              ip_tos;
>> -       u16             ip_len;
>> -       u16             ip_id;
>> -       u16             ip_off;
>> -       u8              ip_ttl;
>> -       u8              ip_p;
>> -       u16             ip_sum;
>> -       struct in_addr  ip_src;
>> -       struct in_addr  ip_dst;
>> +*/
>> +struct tcp_hdr {
>>         u16             tcp_src;
>>         u16             tcp_dst;
>>         u32             tcp_seq;
>> @@ -51,8 +37,8 @@ struct ip_tcp_hdr {
>>         u16             tcp_ugr;
>>  } __packed;
>>
>> -#define IP_TCP_HDR_SIZE                (sizeof(struct ip_tcp_hdr))
>> -#define TCP_HDR_SIZE           (IP_TCP_HDR_SIZE  - IP_HDR_SIZE)
>> +#define TCP_HDR_SIZE           (sizeof(struct tcp_hdr))
>> +#define IP_TCP_HDR_SIZE         (IP_HDR_SIZE + TCP_HDR_SIZE)
>>
>>  #define TCP_DATA       0x00    /* Data Packet - internal use only      */
>>  #define TCP_FIN                0x01    /* Finish flag                          */
>> @@ -178,7 +164,8 @@ struct tcp_t_opt {
>>
>>  /**
>>   * struct ip_tcp_hdr_o - IP + TCP header + TCP options
>> - * @hdr: IP + TCP header
>> + * @ip_hdr: IP + TCP header
>> + * @tcp_hdr: TCP header
>>   * @mss: TCP MSS Option
>>   * @scale: TCP Windows Scale Option
>>   * @sack_p: TCP Sack-Permitted Option
>> @@ -186,7 +173,8 @@ struct tcp_t_opt {
>>   * @end: end of options
>>   */
>>  struct ip_tcp_hdr_o {
>> -       struct  ip_tcp_hdr hdr;
>> +       struct  ip_hdr     ip_hdr;
>> +       struct  tcp_hdr    tcp_hdr;
>>         struct  tcp_mss    mss;
>>         struct  tcp_scale  scale;
>>         struct  tcp_sack_p sack_p;
>> @@ -198,13 +186,15 @@ struct ip_tcp_hdr_o {
>>
>>  /**
>>   * struct ip_tcp_hdr_s - IP + TCP header + TCP options
>> - * @hdr: IP + TCP header
>> + * @ip_hdr: IP + TCP header
>> + * @tcp_hdr: TCP header
>>   * @t_opt: TCP Timestamp Option
>>   * @sack_v: TCP SACK Option
>>   * @end: end of options
>>   */
>>  struct ip_tcp_hdr_s {
>> -       struct  ip_tcp_hdr      hdr;
>> +       struct  ip_hdr     ip_hdr;
>> +       struct  tcp_hdr    tcp_hdr;
>>         struct  tcp_t_opt       t_opt;
>>         struct  tcp_sack_v      sack_v;
>>         u8      end;
>> @@ -303,3 +293,5 @@ void rxhand_tcp_f(union tcp_build_pkt *b, unsigned int len);
>>
>>  u16 tcp_set_pseudo_header(uchar *pkt, struct in_addr src, struct in_addr dest,
>>                           int tcp_len, int pkt_len);
>> +
>> +#endif // __TCP_H__
>> diff --git a/net/tcp.c b/net/tcp.c
>> index a713e1dd60..10ce799814 100644
>> --- a/net/tcp.c
>> +++ b/net/tcp.c
>> @@ -155,7 +155,7 @@ u16 tcp_set_pseudo_header(uchar *pkt, struct in_addr src, struct in_addr dest,
>>   */
>>  int net_set_ack_options(union tcp_build_pkt *b)
>>  {
>> -       b->sack.hdr.tcp_hlen = SHIFT_TO_TCPHDRLEN_FIELD(LEN_B_TO_DW(TCP_HDR_SIZE));
>> +       b->sack.tcp_hdr.tcp_hlen = SHIFT_TO_TCPHDRLEN_FIELD(LEN_B_TO_DW(TCP_HDR_SIZE));
>>
>>         b->sack.t_opt.kind = TCP_O_TS;
>>         b->sack.t_opt.len = TCP_OPT_LEN_A;
>> @@ -187,12 +187,12 @@ int net_set_ack_options(union tcp_build_pkt *b)
>>                         b->sack.sack_v.hill[3].r = TCP_O_NOP;
>>                 }
>>
>> -               b->sack.hdr.tcp_hlen = SHIFT_TO_TCPHDRLEN_FIELD(ROUND_TCPHDR_LEN(TCP_HDR_SIZE +
>> +               b->sack.tcp_hdr.tcp_hlen = SHIFT_TO_TCPHDRLEN_FIELD(ROUND_TCPHDR_LEN(TCP_HDR_SIZE +
>>                                                                                  TCP_TSOPT_SIZE +
>>                                                                                  tcp_lost.len));
>>         } else {
>>                 b->sack.sack_v.kind = 0;
>> -               b->sack.hdr.tcp_hlen = SHIFT_TO_TCPHDRLEN_FIELD(ROUND_TCPHDR_LEN(TCP_HDR_SIZE +
>> +               b->sack.tcp_hdr.tcp_hlen = SHIFT_TO_TCPHDRLEN_FIELD(ROUND_TCPHDR_LEN(TCP_HDR_SIZE +
>>                                                                                  TCP_TSOPT_SIZE));
>>         }
>>
>> @@ -201,7 +201,7 @@ int net_set_ack_options(union tcp_build_pkt *b)
>>          * TCP header to add to the total packet length
>>          */
>>
>> -       return GET_TCP_HDR_LEN_IN_BYTES(b->sack.hdr.tcp_hlen);
>> +       return GET_TCP_HDR_LEN_IN_BYTES(b->sack.tcp_hdr.tcp_hlen);
>>  }
>>
>>  /**
>> @@ -213,7 +213,7 @@ void net_set_syn_options(union tcp_build_pkt *b)
>>         if (IS_ENABLED(CONFIG_PROT_TCP_SACK))
>>                 tcp_lost.len = 0;
>>
>> -       b->ip.hdr.tcp_hlen = 0xa0;
>> +       b->ip.tcp_hdr.tcp_hlen = 0xa0;
>>
>>         b->ip.mss.kind = TCP_O_MSS;
>>         b->ip.mss.len = TCP_OPT_LEN_4;
>> @@ -249,9 +249,9 @@ int tcp_set_tcp_header(uchar *pkt, int dport, int sport, int payload_len,
>>          * Header: 5 32 bit words. 4 bits TCP header Length,
>>          *         4 bits reserved options
>>          */
>> -       b->ip.hdr.tcp_flags = action;
>> +       b->ip.tcp_hdr.tcp_flags = action;
>>         pkt_hdr_len = IP_TCP_HDR_SIZE;
>> -       b->ip.hdr.tcp_hlen = SHIFT_TO_TCPHDRLEN_FIELD(LEN_B_TO_DW(TCP_HDR_SIZE));
>> +       b->ip.tcp_hdr.tcp_hlen = SHIFT_TO_TCPHDRLEN_FIELD(LEN_B_TO_DW(TCP_HDR_SIZE));
>>
>>         switch (action) {
>>         case TCP_SYN:
>> @@ -274,7 +274,7 @@ int tcp_set_tcp_header(uchar *pkt, int dport, int sport, int payload_len,
>>         case TCP_SYN | TCP_ACK:
>>         case TCP_ACK:
>>                 pkt_hdr_len = IP_HDR_SIZE + net_set_ack_options(b);
>> -               b->ip.hdr.tcp_flags = action;
>> +               b->ip.tcp_hdr.tcp_flags = action;
>>                 debug_cond(DEBUG_DEV_PKT,
>>                            "TCP Hdr:ACK (%pI4, %pI4, s=%u, a=%u, A=%x)\n",
>>                            &net_server_ip, &net_ip, tcp_seq_num, tcp_ack_num,
>> @@ -308,7 +308,7 @@ int tcp_set_tcp_header(uchar *pkt, int dport, int sport, int payload_len,
>>                 fallthrough;
>>         default:
>>                 pkt_hdr_len = IP_HDR_SIZE + net_set_ack_options(b);
>> -               b->ip.hdr.tcp_flags = action | TCP_PUSH | TCP_ACK;
>> +               b->ip.tcp_hdr.tcp_flags = action | TCP_PUSH | TCP_ACK;
>>                 debug_cond(DEBUG_DEV_PKT,
>>                            "TCP Hdr:dft  (%pI4, %pI4, s=%u, a=%u, A=%x)\n",
>>                            &net_server_ip, &net_ip,
>> @@ -320,10 +320,10 @@ int tcp_set_tcp_header(uchar *pkt, int dport, int sport, int payload_len,
>>
>>         tcp_ack_edge = tcp_ack_num;
>>         /* TCP Header */
>> -       b->ip.hdr.tcp_ack = htonl(tcp_ack_edge);
>> -       b->ip.hdr.tcp_src = htons(sport);
>> -       b->ip.hdr.tcp_dst = htons(dport);
>> -       b->ip.hdr.tcp_seq = htonl(tcp_seq_num);
>> +       b->ip.tcp_hdr.tcp_ack = htonl(tcp_ack_edge);
>> +       b->ip.tcp_hdr.tcp_src = htons(sport);
>> +       b->ip.tcp_hdr.tcp_dst = htons(dport);
>> +       b->ip.tcp_hdr.tcp_seq = htonl(tcp_seq_num);
>>
>>         /*
>>          * TCP window size - TCP header variable tcp_win.
>> @@ -340,13 +340,13 @@ int tcp_set_tcp_header(uchar *pkt, int dport, int sport, int payload_len,
>>          * it is, then the u-boot tftp or nfs kernel netboot should be
>>          * considered.
>>          */
>> -       b->ip.hdr.tcp_win = htons(PKTBUFSRX * TCP_MSS >> TCP_SCALE);
>> +       b->ip.tcp_hdr.tcp_win = htons(PKTBUFSRX * TCP_MSS >> TCP_SCALE);
>>
>> -       b->ip.hdr.tcp_xsum = 0;
>> -       b->ip.hdr.tcp_ugr = 0;
>> +       b->ip.tcp_hdr.tcp_xsum = 0;
>> +       b->ip.tcp_hdr.tcp_ugr = 0;
>>
>> -       b->ip.hdr.tcp_xsum = tcp_set_pseudo_header(pkt, net_ip, net_server_ip,
>> -                                                  tcp_len, pkt_len);
>> +       b->ip.tcp_hdr.tcp_xsum = tcp_set_pseudo_header(pkt, net_ip, net_server_ip,
>> +                                                      tcp_len, pkt_len);
>>
>>         net_set_ip_header((uchar *)&b->ip, net_server_ip, net_ip,
>>                           pkt_len, IPPROTO_TCP);
>> @@ -638,7 +638,7 @@ static u8 tcp_state_machine(u8 tcp_flags, u32 tcp_seq_num, int payload_len)
>>  void rxhand_tcp_f(union tcp_build_pkt *b, unsigned int pkt_len)
>>  {
>>         int tcp_len = pkt_len - IP_HDR_SIZE;
>> -       u16 tcp_rx_xsum = b->ip.hdr.ip_sum;
>> +       u16 tcp_rx_xsum = b->ip.ip_hdr.ip_sum;
>>         u8  tcp_action = TCP_DATA;
>>         u32 tcp_seq_num, tcp_ack_num;
>>         int tcp_hdr_len, payload_len;
>> @@ -646,11 +646,11 @@ void rxhand_tcp_f(union tcp_build_pkt *b, unsigned int pkt_len)
>>         /* Verify IP header */
>>         debug_cond(DEBUG_DEV_PKT,
>>                    "TCP RX in RX Sum (to=%pI4, from=%pI4, len=%d)\n",
>> -                  &b->ip.hdr.ip_src, &b->ip.hdr.ip_dst, pkt_len);
>> +                  &b->ip.ip_hdr.ip_src, &b->ip.ip_hdr.ip_dst, pkt_len);
>>
>> -       b->ip.hdr.ip_src = net_server_ip;
>> -       b->ip.hdr.ip_dst = net_ip;
>> -       b->ip.hdr.ip_sum = 0;
>> +       b->ip.ip_hdr.ip_src = net_server_ip;
>> +       b->ip.ip_hdr.ip_dst = net_ip;
>> +       b->ip.ip_hdr.ip_sum = 0;
>>         if (tcp_rx_xsum != compute_ip_checksum(b, IP_HDR_SIZE)) {
>>                 debug_cond(DEBUG_DEV_PKT,
>>                            "TCP RX IP xSum Error (%pI4, =%pI4, len=%d)\n",
>> @@ -659,10 +659,10 @@ void rxhand_tcp_f(union tcp_build_pkt *b, unsigned int pkt_len)
>>         }
>>
>>         /* Build pseudo header and verify TCP header */
>> -       tcp_rx_xsum = b->ip.hdr.tcp_xsum;
>> -       b->ip.hdr.tcp_xsum = 0;
>> -       if (tcp_rx_xsum != tcp_set_pseudo_header((uchar *)b, b->ip.hdr.ip_src,
>> -                                                b->ip.hdr.ip_dst, tcp_len,
>> +       tcp_rx_xsum = b->ip.tcp_hdr.tcp_xsum;
>> +       b->ip.tcp_hdr.tcp_xsum = 0;
>> +       if (tcp_rx_xsum != tcp_set_pseudo_header((uchar *)b, b->ip.ip_hdr.ip_src,
>> +                                                b->ip.ip_hdr.ip_dst, tcp_len,
>>                                                  pkt_len)) {
>>                 debug_cond(DEBUG_DEV_PKT,
>>                            "TCP RX TCP xSum Error (%pI4, %pI4, len=%d)\n",
>> @@ -670,7 +670,7 @@ void rxhand_tcp_f(union tcp_build_pkt *b, unsigned int pkt_len)
>>                 return;
>>         }
>>
>> -       tcp_hdr_len = GET_TCP_HDR_LEN_IN_BYTES(b->ip.hdr.tcp_hlen);
>> +       tcp_hdr_len = GET_TCP_HDR_LEN_IN_BYTES(b->ip.tcp_hdr.tcp_hlen);
>>         payload_len = tcp_len - tcp_hdr_len;
>>
>>         if (tcp_hdr_len > TCP_HDR_SIZE)
>> @@ -680,11 +680,11 @@ void rxhand_tcp_f(union tcp_build_pkt *b, unsigned int pkt_len)
>>          * Incoming sequence and ack numbers are server's view of the numbers.
>>          * The app must swap the numbers when responding.
>>          */
>> -       tcp_seq_num = ntohl(b->ip.hdr.tcp_seq);
>> -       tcp_ack_num = ntohl(b->ip.hdr.tcp_ack);
>> +       tcp_seq_num = ntohl(b->ip.tcp_hdr.tcp_seq);
>> +       tcp_ack_num = ntohl(b->ip.tcp_hdr.tcp_ack);
>>
>>         /* Packets are not ordered. Send to app as received. */
>> -       tcp_action = tcp_state_machine(b->ip.hdr.tcp_flags,
>> +       tcp_action = tcp_state_machine(b->ip.tcp_hdr.tcp_flags,
>>                                        tcp_seq_num, payload_len);
>>
>>         tcp_activity_count++;
>> @@ -698,8 +698,8 @@ void rxhand_tcp_f(union tcp_build_pkt *b, unsigned int pkt_len)
>>                            "TCP Notify (action=%x, Seq=%u,Ack=%u,Pay%d)\n",
>>                            tcp_action, tcp_seq_num, tcp_ack_num, payload_len);
>>
>> -               (*tcp_packet_handler) ((uchar *)b + pkt_len - payload_len, b->ip.hdr.tcp_dst,
>> -                                      b->ip.hdr.ip_src, b->ip.hdr.tcp_src, tcp_seq_num,
>> +               (*tcp_packet_handler) ((uchar *)b + pkt_len - payload_len, b->ip.tcp_hdr.tcp_dst,
>> +                                      b->ip.ip_hdr.ip_src, b->ip.tcp_hdr.tcp_src, tcp_seq_num,
>>                                        tcp_ack_num, tcp_action, payload_len);
>>
>>         } else if (tcp_action != TCP_DATA) {
>> @@ -711,8 +711,8 @@ void rxhand_tcp_f(union tcp_build_pkt *b, unsigned int pkt_len)
>>                  * Warning: Incoming Ack & Seq sequence numbers are transposed
>>                  * here to outgoing Seq & Ack sequence numbers
>>                  */
>> -               net_send_tcp_packet(0, ntohs(b->ip.hdr.tcp_src),
>> -                                   ntohs(b->ip.hdr.tcp_dst),
>> +               net_send_tcp_packet(0, ntohs(b->ip.tcp_hdr.tcp_src),
>> +                                   ntohs(b->ip.tcp_hdr.tcp_dst),
>>                                     (tcp_action & (~TCP_PUSH)),
>>                                     tcp_ack_num, tcp_ack_edge);
>>         }
>> diff --git a/test/cmd/wget.c b/test/cmd/wget.c
>> index ed83fc94a5..fd6ed26e8a 100644
>> --- a/test/cmd/wget.c
>> +++ b/test/cmd/wget.c
>> @@ -52,9 +52,9 @@ static int sb_syn_handler(struct udevice *dev, void *packet,
>>  {
>>         struct eth_sandbox_priv *priv = dev_get_priv(dev);
>>         struct ethernet_hdr *eth = packet;
>> -       struct ip_tcp_hdr *tcp = packet + ETHER_HDR_SIZE;
>> +       struct tcp_hdr *tcp = packet + ETHER_HDR_SIZE + IP_HDR_SIZE;
>>         struct ethernet_hdr *eth_send;
>> -       struct ip_tcp_hdr *tcp_send;
>> +       struct tcp_hdr *tcp_send;
>>
>>         /* Don't allow the buffer to overrun */
>>         if (priv->recv_packets >= PKTBUFSRX)
>> @@ -64,7 +64,7 @@ static int sb_syn_handler(struct udevice *dev, void *packet,
>>         memcpy(eth_send->et_dest, eth->et_src, ARP_HLEN);
>>         memcpy(eth_send->et_src, priv->fake_host_hwaddr, ARP_HLEN);
>>         eth_send->et_protlen = htons(PROT_IP);
>> -       tcp_send = (void *)eth_send + ETHER_HDR_SIZE;
>> +       tcp_send = (void *)eth_send + ETHER_HDR_SIZE + IP_HDR_SIZE;
>>         tcp_send->tcp_src = tcp->tcp_dst;
>>         tcp_send->tcp_dst = tcp->tcp_src;
>>         tcp_send->tcp_seq = htonl(0);
>> @@ -97,9 +97,9 @@ static int sb_ack_handler(struct udevice *dev, void *packet,
>>  {
>>         struct eth_sandbox_priv *priv = dev_get_priv(dev);
>>         struct ethernet_hdr *eth = packet;
>> -       struct ip_tcp_hdr *tcp = packet + ETHER_HDR_SIZE;
>> +       struct tcp_hdr *tcp = packet + ETHER_HDR_SIZE + IP_HDR_SIZE;
>>         struct ethernet_hdr *eth_send;
>> -       struct ip_tcp_hdr *tcp_send;
>> +       struct tcp_hdr *tcp_send;
>>         void *data;
>>         int pkt_len;
>>         int payload_len = 0;
>> @@ -115,7 +115,7 @@ static int sb_ack_handler(struct udevice *dev, void *packet,
>>         memcpy(eth_send->et_dest, eth->et_src, ARP_HLEN);
>>         memcpy(eth_send->et_src, priv->fake_host_hwaddr, ARP_HLEN);
>>         eth_send->et_protlen = htons(PROT_IP);
>> -       tcp_send = (void *)eth_send + ETHER_HDR_SIZE;
>> +       tcp_send = (void *)eth_send + ETHER_HDR_SIZE + IP_HDR_SIZE;
>>         tcp_send->tcp_src = tcp->tcp_dst;
>>         tcp_send->tcp_dst = tcp->tcp_src;
>>         data = (void *)tcp_send + IP_TCP_HDR_SIZE;
>> @@ -163,14 +163,14 @@ static int sb_http_handler(struct udevice *dev, void *packet,
>>  {
>>         struct ethernet_hdr *eth = packet;
>>         struct ip_hdr *ip;
>> -       struct ip_tcp_hdr *tcp;
>> +       struct tcp_hdr *tcp;
>>
>>         if (ntohs(eth->et_protlen) == PROT_ARP) {
>>                 return sb_arp_handler(dev, packet, len);
>>         } else if (ntohs(eth->et_protlen) == PROT_IP) {
>>                 ip = packet + ETHER_HDR_SIZE;
>>                 if (ip->ip_p == IPPROTO_TCP) {
>> -                       tcp = packet + ETHER_HDR_SIZE;
>> +                       tcp = ip + IP_HDR_SIZE;
>>                         if (tcp->tcp_flags == TCP_SYN)
>>                                 return sb_syn_handler(dev, packet, len);
>>                         else if (tcp->tcp_flags & TCP_ACK && !(tcp->tcp_flags & TCP_SYN))
>> --
>> 2.40.1.521.gf1e218fcd8-goog
>>


More information about the U-Boot mailing list