[dpdk-dev] [PATCH v2] app/testpmd: fix IP checksum calculation

[George Prekas]

On 1/6/2021 12:02 PM, Ferruh Yigit wrote:
> On 12/5/2020 5:42 AM, George Prekas wrote:
>> Strict-aliasing rules are violated by cast to uint16_t* in flowgen.c
>> and the calculated IP checksum is wrong on GCC 9 and GCC 10.
>>
>> Signed-off-by: George Prekas <prekageo at amazon.com>
>> ---
>> v2:
>> * Instead of a compiler barrier, use a compiler flag.
>> ---
>>   app/test-pmd/meson.build | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/app/test-pmd/meson.build b/app/test-pmd/meson.build
>> index 7e9c7bdd6..5d24e807f 100644
>> --- a/app/test-pmd/meson.build
>> +++ b/app/test-pmd/meson.build
>> @@ -4,6 +4,7 @@
>>   # override default name to drop the hyphen
>>   name = 'testpmd'
>>   cflags += '-Wno-deprecated-declarations'
>> +cflags += '-fno-strict-aliasing'
>>   sources = files('5tswap.c',
>>       'cmdline.c',
>>       'cmdline_flow.c',
>>
> 
> Hi George,
> 
> I am trying to understand this, the relevant code is as below:
> ip_hdr->hdr_checksum = ip_sum((unaligned_uint16_t *)ip_hdr, sizeof(*ip_hdr));
> 
> You are suspicious of strict aliasing rule violation, with more details:
> The concern is the "struct rte_ipv4_hdr *ip_hdr;" aliased to "const
> unaligned_uint16_t *hdr", and compiler can optimize out the calculations using
> data pointed by 'hdr' pointer, since the 'hdr' pointer is not used to alter the
> data and compiler may think data is not changed at all.
> 
> 1) But the pointer "hdr" is assigned in the loop, from another pointer whose
> content is changing, why this is not helping to figure out that the data 'hdr'
> pointing is changed.
> 
> 2) I tried to debug this, but I am not able to reproduce the issue, 'ip_sum()'
> called each time and checksum calculated correctly. Using gcc 10.2.1-9. Can you
> able to confirm the case with debug, or from the assembly/object file?
> 
> 
> And if the issue is strict aliasing rule violation as you said, compiler flag is
> an option but not sure how much it reduces the compiler optimization benefit, I
> guess other options also not so good, memcpy brings too much work on runtime and
> union requires bigger change and makes code complex.
> I wonder if making 'ip_sum()' a non inline function can help, can you please
> give a try since you can reproduce it?

Hi Ferruh,

Thanks for looking into it.

I am copy-pasting at the end of this email a minimal reproduction. It calculates a checksum and prints it. The correct value is f8d9. If you compile it with -O0 or -O3 -fno-strict-aliasing, you will get the correct value. If you compile it with gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0 and -O3, you will get f8e8. You can also try it on https://godbolt.org/ and see how different versions behave.

My understanding is that the code violates the C standard (https://stackoverflow.com/a/99010).

--- cut here --- 

#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

struct rte_ipv4_hdr {
	uint8_t  version_ihl;
	uint8_t  type_of_service;
	uint16_t total_length;
	uint16_t packet_id;
	uint16_t fragment_offset;
	uint8_t  time_to_live;
	uint8_t  next_proto_id;
	uint16_t hdr_checksum;
	uint32_t src_addr;
	uint32_t dst_addr;
};

static inline uint16_t ip_sum(const uint16_t *hdr, int hdr_len)
{
	uint32_t sum = 0;

	while (hdr_len > 1)
	{
		sum += *hdr++;
		if (sum & 0x80000000)
			sum = (sum & 0xFFFF) + (sum >> 16);
		hdr_len -= 2;
	}

	while (sum >> 16)
		sum = (sum & 0xFFFF) + (sum >> 16);

	return ~sum;
}

static void pkt_burst_flow_gen(void)
{
	struct rte_ipv4_hdr *ip_hdr = (struct rte_ipv4_hdr *) malloc(4096);
	memset(ip_hdr, 0, sizeof(*ip_hdr));
	ip_hdr->version_ihl	= 1;
	ip_hdr->type_of_service	= 2;
	ip_hdr->fragment_offset	= 3;
	ip_hdr->time_to_live	= 4;
	ip_hdr->next_proto_id	= 5;
	ip_hdr->packet_id	= 6;
	ip_hdr->src_addr	= 7;
	ip_hdr->dst_addr	= 8;
	ip_hdr->total_length	= 9;
	ip_hdr->hdr_checksum	= ip_sum((uint16_t *)ip_hdr, sizeof(*ip_hdr));
	printf("%x\n", ip_hdr->hdr_checksum);
}

int main(void)
{
	pkt_burst_flow_gen();
	return 0;
}