[v2,1/3] net: improve UDP/TCP checksum computation.

This patch adds:
 * based on Eth, UDP, TCP struct present in eth.h instead of hardcoded indexes.
 * based on various macros present in eth.h.
 * allow to account for optional VLAN header.

Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
---
 net/checksum.c | 83 ++++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 57 insertions(+), 26 deletions(-)

On 23 April 2016 at 11:58, Jean-Christophe Dubois <jcd@tribudubois.net> wrote:
> This patch adds:
>  * based on Eth, UDP, TCP struct present in eth.h instead of hardcoded indexes.
>  * based on various macros present in eth.h.
>  * allow to account for optional VLAN header.

This is doing several things:
 (1) changing style to use structs and macros rather than raw array accesses
    (which shouldn't affect functionality)
 (2) adding new functionality

I think they would be better in separate patches.


This function is used by multiple network devices, including the
important ones xen_nic and virtio-net -- is it definitely OK to
have it change behaviour for those devices?


> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
> ---
>  net/checksum.c | 83 ++++++++++++++++++++++++++++++++++++++++------------------
>  1 file changed, 57 insertions(+), 26 deletions(-)
>
> diff --git a/net/checksum.c b/net/checksum.c
> index d0fa424..fd25209 100644
> --- a/net/checksum.c
> +++ b/net/checksum.c
> @@ -18,9 +18,7 @@
>  #include "qemu/osdep.h"
>  #include "qemu-common.h"
>  #include "net/checksum.h"
> -
> -#define PROTO_TCP  6
> -#define PROTO_UDP 17
> +#include "net/eth.h"
>
>  uint32_t net_checksum_add_cont(int len, uint8_t *buf, int seq)
>  {
> @@ -57,40 +55,73 @@ uint16_t net_checksum_tcpudp(uint16_t length, uint16_t proto,
>
>  void net_checksum_calculate(uint8_t *data, int length)
>  {
> -    int hlen, plen, proto, csum_offset;
> -    uint16_t csum;
> +    int plen;
> +    struct ip_header *ip;
> +
> +    /* Ensure we have at least a Eth header */
> +    if (length < sizeof(struct eth_header)) {
> +        return;
> +    }
>
> -    /* Ensure data has complete L2 & L3 headers. */
> -    if (length < 14 + 20) {
> +    /* Now check we have an IP header (with an optonnal VLAN header */

"optional", also missing ")".

> +    if (length < eth_get_l2_hdr_length(data) + sizeof(struct ip_header)) {
>          return;
>      }
>
> -    if ((data[14] & 0xf0) != 0x40)
> +    ip = PKT_GET_IP_HDR(data);

Are we definitely guaranteed that the input data buffer is aligned?
It seems unlikely, and if it isn't then a lot of this code (both
in macros like PKT_GET_IP_HDR, and open coded stuff below) is going
to go wrong if it tries to just access 16 bit struct fields and they're
not aligned and we're on a host that requires strict alignment.

> +
> +    if (IP_HEADER_VERSION(ip) != IP_HEADER_VERSION_4) {
>         return; /* not IPv4 */
> -    hlen  = (data[14] & 0x0f) * 4;
> -    plen  = (data[16] << 8 | data[17]) - hlen;
> -    proto = data[23];
> +    }
> +
> +    /* Last, check that we have enough data for the IP frame */
> +    if (length < eth_get_l2_hdr_length(data) + be16_to_cpu(ip->ip_len)) {
> +        return;
> +    }
> +
> +    plen  = be16_to_cpu(ip->ip_len) - IP_HDR_GET_LEN(ip);
> +
> +    switch (ip->ip_p) {
> +    case IP_PROTO_TCP:
> +        {
> +            uint16_t csum;
> +            tcp_header *tcp = (tcp_header *)(ip + 1);
> +
> +            if (plen < sizeof(tcp_header)) {
> +                return;
> +            }

I think this indent style is indenting to much. switch statements
usually look like:


    switch (whatever) {
    case FOO:
    {
        code here;
    }
    case BAR:
    {
        more code;
    }
    default:
        whatever;
    }

>
> -    switch (proto) {
> -    case PROTO_TCP:
> -       csum_offset = 16;
> +            tcp->th_sum = 0;
> +
> +            csum = net_checksum_tcpudp(plen, ip->ip_p,
> +                                       (uint8_t *)&ip->ip_src,
> +                                       (uint8_t *)tcp);
> +
> +            tcp->th_sum = cpu_to_be16(csum);
> +        }
>         break;
> -    case PROTO_UDP:
> -       csum_offset = 6;
> +    case IP_PROTO_UDP:
> +        {
> +            uint16_t csum;
> +            udp_header *udp = (udp_header *)(ip + 1);
> +
> +            if (plen < sizeof(udp_header)) {
> +                return;
> +            }
> +
> +            udp->uh_sum = 0;
> +
> +            csum = net_checksum_tcpudp(plen, ip->ip_p,
> +                                       (uint8_t *)&ip->ip_src,
> +                                       (uint8_t *)udp);
> +
> +            udp->uh_sum = cpu_to_be16(csum);
> +        }
>         break;
>      default:
> +        /* Can't handle any other protocol */
>         return;
>      }
> -
> -    if (plen < csum_offset + 2 || 14 + hlen + plen > length) {
> -        return;
> -    }
> -
> -    data[14+hlen+csum_offset]   = 0;
> -    data[14+hlen+csum_offset+1] = 0;
> -    csum = net_checksum_tcpudp(plen, proto, data+14+12, data+14+hlen);
> -    data[14+hlen+csum_offset]   = csum >> 8;
> -    data[14+hlen+csum_offset+1] = csum & 0xff;
>  }
>
>  uint32_t
> --
> 2.7.4

thanks
-- PMM

Le 05/05/2016 16:17, Peter Maydell a écrit :
> On 23 April 2016 at 11:58, Jean-Christophe Dubois <jcd@tribudubois.net> wrote:
>> This patch adds:
>>   * based on Eth, UDP, TCP struct present in eth.h instead of hardcoded indexes.
>>   * based on various macros present in eth.h.
>>   * allow to account for optional VLAN header.
> This is doing several things:
>   (1) changing style to use structs and macros rather than raw array accesses
>      (which shouldn't affect functionality)
>   (2) adding new functionality
>
> I think they would be better in separate patches.

Well, the added functionality comes as a bonus because of the use of the 
struct and macros. It is not so easy to split the 2.
>
>
> This function is used by multiple network devices, including the
> important ones xen_nic and virtio-net -- is it definitely OK to
> have it change behaviour for those devices?

If xen_nic never tries to support VLAN then there is no real change in 
behavior. This patch adds the VLAN support which is a legal network 
frame format which was unsupported so far.

VLAN is supported by the i.MX6 Gb Ethernet device accelerator. 
Therefore, I needed a checksum function that would account for it.

>
>
>> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
>> ---
>>   net/checksum.c | 83 ++++++++++++++++++++++++++++++++++++++++------------------
>>   1 file changed, 57 insertions(+), 26 deletions(-)
>>
>> diff --git a/net/checksum.c b/net/checksum.c
>> index d0fa424..fd25209 100644
>> --- a/net/checksum.c
>> +++ b/net/checksum.c
>> @@ -18,9 +18,7 @@
>>   #include "qemu/osdep.h"
>>   #include "qemu-common.h"
>>   #include "net/checksum.h"
>> -
>> -#define PROTO_TCP  6
>> -#define PROTO_UDP 17
>> +#include "net/eth.h"
>>
>>   uint32_t net_checksum_add_cont(int len, uint8_t *buf, int seq)
>>   {
>> @@ -57,40 +55,73 @@ uint16_t net_checksum_tcpudp(uint16_t length, uint16_t proto,
>>
>>   void net_checksum_calculate(uint8_t *data, int length)
>>   {
>> -    int hlen, plen, proto, csum_offset;
>> -    uint16_t csum;
>> +    int plen;
>> +    struct ip_header *ip;
>> +
>> +    /* Ensure we have at least a Eth header */
>> +    if (length < sizeof(struct eth_header)) {
>> +        return;
>> +    }
>>
>> -    /* Ensure data has complete L2 & L3 headers. */
>> -    if (length < 14 + 20) {
>> +    /* Now check we have an IP header (with an optonnal VLAN header */
> "optional", also missing ")".

OK
>
>> +    if (length < eth_get_l2_hdr_length(data) + sizeof(struct ip_header)) {
>>           return;
>>       }
>>
>> -    if ((data[14] & 0xf0) != 0x40)
>> +    ip = PKT_GET_IP_HDR(data);
> Are we definitely guaranteed that the input data buffer is aligned?
> It seems unlikely, and if it isn't then a lot of this code (both
> in macros like PKT_GET_IP_HDR, and open coded stuff below) is going
> to go wrong if it tries to just access 16 bit struct fields and they're
> not aligned and we're on a host that requires strict alignment.

Beside a potential performance hit on unaligned data (but is it worst 
than accessing all struct members at byte level) is there any Qemu host 
that cannot handle unaligned struct members?

Now most network stacks run by the host or the guest should generally 
used aligned pointers for network buffers. The opposite should be quite 
uncommon. It seems most (emulated) Ethernet chip expect aligned buffers 
to work and therefore the provided pointer should always be correctly 
set to allow aligned access.

Do you have a different experience in Qemu?
>
>> +
>> +    if (IP_HEADER_VERSION(ip) != IP_HEADER_VERSION_4) {
>>          return; /* not IPv4 */
>> -    hlen  = (data[14] & 0x0f) * 4;
>> -    plen  = (data[16] << 8 | data[17]) - hlen;
>> -    proto = data[23];
>> +    }
>> +
>> +    /* Last, check that we have enough data for the IP frame */
>> +    if (length < eth_get_l2_hdr_length(data) + be16_to_cpu(ip->ip_len)) {
>> +        return;
>> +    }
>> +
>> +    plen  = be16_to_cpu(ip->ip_len) - IP_HDR_GET_LEN(ip);
>> +
>> +    switch (ip->ip_p) {
>> +    case IP_PROTO_TCP:
>> +        {
>> +            uint16_t csum;
>> +            tcp_header *tcp = (tcp_header *)(ip + 1);
>> +
>> +            if (plen < sizeof(tcp_header)) {
>> +                return;
>> +            }
> I think this indent style is indenting to much. switch statements
> usually look like:
OK
>
>      switch (whatever) {
>      case FOO:
>      {
>          code here;
>      }
>      case BAR:
>      {
>          more code;
>      }
>      default:
>          whatever;
>      }
>
>> -    switch (proto) {
>> -    case PROTO_TCP:
>> -       csum_offset = 16;
>> +            tcp->th_sum = 0;
>> +
>> +            csum = net_checksum_tcpudp(plen, ip->ip_p,
>> +                                       (uint8_t *)&ip->ip_src,
>> +                                       (uint8_t *)tcp);
>> +
>> +            tcp->th_sum = cpu_to_be16(csum);
>> +        }
>>          break;
>> -    case PROTO_UDP:
>> -       csum_offset = 6;
>> +    case IP_PROTO_UDP:
>> +        {
>> +            uint16_t csum;
>> +            udp_header *udp = (udp_header *)(ip + 1);
>> +
>> +            if (plen < sizeof(udp_header)) {
>> +                return;
>> +            }
>> +
>> +            udp->uh_sum = 0;
>> +
>> +            csum = net_checksum_tcpudp(plen, ip->ip_p,
>> +                                       (uint8_t *)&ip->ip_src,
>> +                                       (uint8_t *)udp);
>> +
>> +            udp->uh_sum = cpu_to_be16(csum);
>> +        }
>>          break;
>>       default:
>> +        /* Can't handle any other protocol */
>>          return;
>>       }
>> -
>> -    if (plen < csum_offset + 2 || 14 + hlen + plen > length) {
>> -        return;
>> -    }
>> -
>> -    data[14+hlen+csum_offset]   = 0;
>> -    data[14+hlen+csum_offset+1] = 0;
>> -    csum = net_checksum_tcpudp(plen, proto, data+14+12, data+14+hlen);
>> -    data[14+hlen+csum_offset]   = csum >> 8;
>> -    data[14+hlen+csum_offset+1] = csum & 0xff;
>>   }
>>
>>   uint32_t
>> --
>> 2.7.4
> thanks
> -- PMM
>

On 6 May 2016 at 21:25, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote:
> Le 05/05/2016 16:17, Peter Maydell a écrit :
>>
>> On 23 April 2016 at 11:58, Jean-Christophe Dubois <jcd@tribudubois.net>
>> wrote:
>>> +    if (length < eth_get_l2_hdr_length(data) + sizeof(struct ip_header))
>>> {
>>>           return;
>>>       }
>>>
>>> -    if ((data[14] & 0xf0) != 0x40)
>>> +    ip = PKT_GET_IP_HDR(data);
>>
>> Are we definitely guaranteed that the input data buffer is aligned?
>> It seems unlikely, and if it isn't then a lot of this code (both
>> in macros like PKT_GET_IP_HDR, and open coded stuff below) is going
>> to go wrong if it tries to just access 16 bit struct fields and they're
>> not aligned and we're on a host that requires strict alignment.
>
>
> Beside a potential performance hit on unaligned data (but is it worst than
> accessing all struct members at byte level) is there any Qemu host that
> cannot handle unaligned struct members?

MIPS, for instance, also older ARM CPUs. I wouldn't be surprised
if PPC also required alignment.

> Now most network stacks run by the host or the guest should generally used
> aligned pointers for network buffers. The opposite should be quite uncommon.
> It seems most (emulated) Ethernet chip expect aligned buffers to work and
> therefore the provided pointer should always be correctly set to allow
> aligned access.

Since the buffer alignment is provided by the guest, you can't
allow QEMU to crash if it's unaligned.

We have good working support for handling loading data from
potentially unaligned buffers (and in particular loading data
of a fixed endianness), so we should probably just use it.

thanks
-- PMM