> From: Morten Brørup > Sent: Thursday, 16 June 2022 16.36 > > > From: Mattias Rönnblom [mailto:hof...@lysator.liu.se] > > Sent: Thursday, 16 June 2022 15.58 > > > > On 2022-06-16 08:44, Morten Brørup wrote: > > > +CC Olivier Matz <olivier.m...@6wind.com>, Network Headers > maintainer > > > > > >> -----Original Message----- > > >> From: Morten Brørup <m...@smartsharesystems.com> > > >> Sent: den 15 juni 2022 16:41 > > >> To: Emil Berg <emil.b...@ericsson.com>; bugzi...@dpdk.org > > >> Cc: dev@dpdk.org > > >> Subject: RE: [Bug 1035] __rte_raw_cksum() crash with misaligned > > pointer > > >> > > >>> From: bugzi...@dpdk.org [mailto:bugzi...@dpdk.org] > > >>> Sent: Wednesday, 15 June 2022 09.16 > > >>> > > >>> https://protect2.fireeye.com/v1/url?k=31323334-501d5122-313273af- > > >> 45444 > > >>> 5555731-2e92ae6bf759c0c5&q=1&e=b3fc70af-5d37-4ffb-b34d- > > >> 9a51927f5f6d&u= > > >>> https%3A%2F%2Fbugs.dpdk.org%2Fshow_bug.cgi%3Fid%3D1035 > > >>> > > >>> Bug ID: 1035 > > >>> Summary: __rte_raw_cksum() crash with misaligned > > pointer > > >>> Product: DPDK > > >>> Version: 21.11 > > >>> Hardware: All > > >>> OS: All > > >>> Status: UNCONFIRMED > > >>> Severity: normal > > >>> Priority: Normal > > >>> Component: ethdev > > >>> Assignee: dev@dpdk.org > > >>> Reporter: emil.b...@ericsson.com > > >>> Target Milestone: --- > > >>> > > >>> See rte_raw_cksum() in rte_ip.h, which is part of the public API. > > See > > >>> also the subfunction __rte_raw_cksum(). > > >>> > > >>> _rte_raw_cksum assumes that the buffer over which the checksum is > > >>> calculated is an even address (divisible by two). See for example > > >> this > > >>> stack overflow > > >>> post: > > >>> https://stackoverflow.com/questions/46790550/c-undefined- > behavior- > > >>> strict-aliasing-rule-or-incorrect-alignment > > >>> > > >>> The post explains that there is undefined behavior in C11 when > > >>> "conversion between two pointer types produces a result that is > > >>> incorrectly aligned". When the buf argument starts on an odd > > address > > >>> we thus have undefined behavior, since a pointer is cast from > void* > > >> to > > >>> uint16_t*. > > >>> > > >>> In most cases (at least on x86) that isn't a problem, but with > > higher > > >>> optimization levels it may break due to vector instructions. This > > new > > >>> function seems to be easier to optimize by the compiler, > resulting > > in > > >>> a crash when the buf argument is odd. Please note that the > > undefined > > >>> behavior is present in earlier versions of dpdk as well. > > >>> > > >>> Now you're probably thinking: "Just align your buffers". The > > problem > > >>> is that we have a packet buffer which is aligned. The checksum is > > >>> calculated on a subset of that aligned packet buffer, and that > > >>> sometimes lies on odd addresses. > > >>> > > >>> The question remains if this is an issue with dpdk or not. > > >> > > >> I can imagine other systems doing what you describe too. So it > needs > > to > > >> be addressed. > > >> > > >> Off the top of my head, an easy fix would be updating > > __rte_raw_cksum() > > >> like this: > > >> > > >> static inline uint32_t > > >> __rte_raw_cksum(const void *buf, size_t len, uint32_t sum) { > > >> if (likely((buf & 1) == 0)) { > > >> /* The buffer is 16 bit aligned. */ > > >> Keep the existing, optimized implementation here. > > >> } else { > > >> /* The buffer is not 16 bit aligned. */ > > >> Add a new odd-buf tolerant implementation here. > > >> } > > >> } > > >> > > >> However, I'm not sure that it covers your scenario! > > >> > > >> The checksum is 16 bit wide, so if you calculate the checksum of > > e.g. 4 > > >> bytes of memory starting at offset 1 in a 6 byte packet buffer, > the > > >> memory block can be treated as either 4 or 6 bytes relative to the > > data > > >> covered by the checksum, i.e.: > > >> > > >> A: XX [01 02] [03 04] XX --> cksum = [04 06] > > >> > > >> B: [XX 01] [02 03] [04 XX] --> cksum = [06 04] > > >> > > >> Which one do you need? > > >> > > >> Perhaps an additional function is required to support your use > case, > > >> and the documentation for rte_raw_cksum() and __rte_raw_cksum() > > needs > > >> to reflect that the buffer must be 16 bit aligned. > > >> > > >> Or the rte_raw_cksum() function can be modified to support an odd > > >> buffer pointer as outlined above, with documentation added about > > >> alignment of the running checksum. > > > > > >> -----Original Message----- > > >> From: Emil Berg > > >> Sent: den 16 juni 2022 07:45 > > >> To: Morten Brørup <m...@smartsharesystems.com>; bugzi...@dpdk.org > > >> Cc: dev@dpdk.org > > >> Subject: RE: [Bug 1035] __rte_raw_cksum() crash with misaligned > > pointer > > >> > > >> Hi! > > >> > > >> We want the B option, i.e. the 6 bytes option. Perhaps adding > > alignment > > >> detection to __rte_raw_cksum() is a good idea. > > >> > > >> A minor comment but I think buf & 1 won't work since buf isn't an > > >> integral type, but something along that way. > > >> > > >> I'm starting to think about an efficient way to do this. > > >> > > >> Thank you! > > > > > >> -----Original Message----- > > >> From: Emil Berg [mailto:emil.b...@ericsson.com] > > >> Sent: Thursday, 16 June 2022 08.32 > > >> To: Morten Brørup; bugzi...@dpdk.org > > >> Cc: dev@dpdk.org > > >> Subject: RE: [Bug 1035] __rte_raw_cksum() crash with misaligned > > pointer > > >> > > >> I've been sketching on an efficient solution to this. What about > > >> something along the way below? I've run it with the combinations > of: > > >> even buf, even len > > >> even buf, odd len > > >> odd buf, even len > > >> odd buf, odd len > > >> > > >> and it seems to give the same results as the older version of > > >> __rte_raw_cksum, before 21.03. I ran it without optimizations and > > such > > >> to ensure the compiler didn't insert vector instructions and such > so > > >> the results were comparable. > > > > > > The performance, when using an aligned buffer, needs to be > comparable > > with full compiler optimization, or the patch will not be accepted. > > > > > I think the question is: does rte_raw_cksum() have any alignment > > requirements, from an API contract point of view? The documentation > > says > > nothing about any such. In that case, it seems reasonable to me to > > assume that there are none. > > The packet buffer must be 16 bit aligned. Many structures in DPDK are > designed with this invariant, e.g. the Ethernet header (struct > rte_ether_hdr) [1]. > > I agree that the documentation could mention this invariant in more > places. > > When calculating the checksum of a part of packet buffer, it should > allow that part of the buffer to start at a non-16 bit aligned address, > which is what Emil needs, and this is what I suggest adding support for > in this function. > > Regarding any implementation suggestions, please refer to my examples A > and B above: The running checksum for a partial buffer of 4 bytes > differs, depending on how you calculate it. The checksum calculation > must see the buffer as part of the packet buffer, i.e. aligned with the > (16 bit aligned) packet buffer and the (16 bit aligned) running > checksum, as described by example B. > > [1] > https://elixir.bootlin.com/dpdk/latest/source/lib/net/rte_ether.h#L273
I tried a few things in Godbolt, and will submit a patch shortly. Please hang on...
