From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 70F72A00BE; Thu, 16 Jun 2022 16:09:09 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2EE2140141; Thu, 16 Jun 2022 16:09:09 +0200 (CEST) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by mails.dpdk.org (Postfix) with ESMTP id 61DA04003C; Thu, 16 Jun 2022 16:09:08 +0200 (CEST) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id 1BDC0C7A0; Thu, 16 Jun 2022 16:09:08 +0200 (CEST) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id 1A971C3EE; Thu, 16 Jun 2022 16:09:08 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on hermod.lysator.liu.se X-Spam-Level: X-Spam-Status: No, score=-3.3 required=5.0 tests=ALL_TRUSTED, AWL, NICE_REPLY_A, T_SCC_BODY_TEXT_LINE autolearn=disabled version=3.4.6 X-Spam-Score: -3.3 Received: from [192.168.1.59] (unknown [62.63.215.114]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (2048 bits)) (No client certificate requested) by mail.lysator.liu.se (Postfix) with ESMTPSA id 42333C79F; Thu, 16 Jun 2022 16:09:07 +0200 (CEST) Message-ID: <5fbc3817-2734-6ad9-c42a-937850f0b797@lysator.liu.se> Date: Thu, 16 Jun 2022 16:09:06 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.1 Subject: Re: [Bug 1035] __rte_raw_cksum() crash with misaligned pointer Content-Language: en-US To: Emil Berg , =?UTF-8?Q?Morten_Br=c3=b8rup?= , "bugzilla@dpdk.org" Cc: "dev@dpdk.org" References: <98CBD80474FA8B44BF855DF32C47DC35D87127@smartserver.smartshare.dk> From: =?UTF-8?Q?Mattias_R=c3=b6nnblom?= In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Virus-Scanned: ClamAV using ClamSMTP X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On 2022-06-16 08:32, Emil Berg wrote: > 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. > ...but you *want* the compiler to vectorize this code. There's much to gain, and it can likely be done also in the non-aligned case. What you don't want is for the compiler to assume the data is 16-bit aligned (and output SIMD load/store instructions which require alignment). I don't see why you just can't take the current implementation, and replace the direct assignment ("*u16_buf") with a temporary variable, and a memcpy(). This also eliminates the need for the may_alias attribute (at least on the u16_buf pointer). > static inline uint32_t > __rte_raw_cksum_newest(const void *buf, size_t len, uint32_t sum) > { > const uint8_t *end = buf + len; > > uint32_t sum_even = 0; > for (const uint8_t *p = buf + 1; p < end; p += 2) { > sum_even += *p; > } > sum += sum_even << 8; > > uint32_t sum_odd = 0; > for (const uint8_t *p = buf; p < end; p += 2) { > sum_odd += *p; > } > sum += sum_odd; > > return sum; > } > > /Emil > > -----Original Message----- > From: Emil Berg > Sent: den 16 juni 2022 07:45 > To: Morten Brørup ; bugzilla@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: Morten Brørup > Sent: den 15 juni 2022 16:41 > To: Emil Berg ; bugzilla@dpdk.org > Cc: dev@dpdk.org > Subject: RE: [Bug 1035] __rte_raw_cksum() crash with misaligned pointer > >> From: bugzilla@dpdk.org [mailto:bugzilla@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.berg@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. >