Re: [lng-odp] [PATCH v3][WIP] Validate checksums during packet parsing
Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/include/odp_chksum_internal.h
line 24
@@ -0,0 +1,59 @@
+/* Copyright (c) 2013, Linaro Limited
+ * All rights reserved.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+
+/**
+ * @file
+ *
+ * ODP checksum - implementation internal
+ */
+
+#ifndef ODP_CHKSUM_INTERNAL_H_
+#define ODP_CHKSUM_INTERNAL_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/* Simple implementation of ones complement sum.
+ * Based on RFC1071 and its errata.
+ */
+static uint32_t _odp_chksum_ones_comp16_32(const void *p, uint32_t len,
+ odp_bool_t odd_offset)
Comment:
One of the reasons why `odp_packet_copy_to_mem()` exists is to handle this sort
of alignment problem since the caller can ensure that the destination memory
address is aligned properly for intended use. But whenever you use
`odp_packet_offset()` or similar routines there's no guarantee that the address
you get back will have any particular alignment associated with it. Packets may
start out aligned, but once head push/pull operations are issued all bets are
off.
Applications are responsible for keeping track of this sort of thing
themselves, but for ODP internal use we have to be prepared for packets that
have been manipulated in such a way that individual byte offsets may start on
any byte boundary.
> Dmitry Eremin-Solenikov(lumag) wrote:
> Hmm. Should we create a list of all possible unaligned access locations?
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> OK, I see what you're saying now. But it looks like we need to consider
>> both. The `odd_offset` parameter is saying whether the (continued) checksum
>> needs to insert a leading zero into the calculation, however it's still true
>> that you have to take into account whether `p` is even or odd since that can
>> also vary independently. You can't fetch a `uint16_t` value from an odd byte
>> address unless your CPU supports unaligned data accesses. Even for those
>> machines that do, this typically results in a significant performance
>> penalty.
>>
>> But now we have another problem. If at entry `p` is even and `odd_offset` is
>> true, meaning that the previous segment contained an odd number of bytes,
>> then assuming this buffer is `{c, d, e, ...}` and we've added in `{0, c}` to
>> account for the `odd_offset`, the next 16-bit value we want to sum is `{d,
>> e}`, but `d` is on an odd address and `e` is on an even address so we can't
>> fetch them as a `uint16_t` but need to assemble them from two `uint8_t`
>> values. So it sounds like we need to factor out the logic required to fetch
>> the next 16 bits to sum from the main loop:
>> ```
>> static inline uint16_t data16(void *p)
>> {
>> if ((uintptr_t)p % 2)
>> return (*((uint8_t *)p) << 8) + (*((uint8_t *)p + 1));
>>
>> return *((uint16_t *)p);
>> }
>> ```
>> This can then be added to the main logic:
>> ```
>> static uint32_t _odp_chksum_ones_comp_16_32(const void *p, uint32_t len,
>> odp_bool_t odd_offset)
>> {
>> void *data = p;
>> uint32_t sum = 0;
>>
>> if (len > 0 && odd_offset) {
>>sum = *((uint8_t *)data++);
>>len--;
>> }
>>
>> while (len > 1) {
>> sum += data16(data);
>> data += 2;
>> len -= 2;
>> }
>>
>>...etc.
>> }
>> ```
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> Calculation of checksum starts at `l4_offset`. If for some reason this
>>> segment has odd amount of bytes after `l4_offset`, then checksumming will
>>> use that byte as `{b, 0}` `uint16_t` value. However calculation of checksum
>>> for the next segment (with first bytes `{c, d, e...}`) can not start as
>>> usual (`{c ,d}`, `{e, }). It should start as `{0, c}`, `{d, e}`,
>>> This is an odd offset. An offset in checksum calculation bytestream. Feel
>>> free to propose better name.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
What offset? The calculation is for `len` bytes starting at address `p`.
Whether `p` is the address of an even or odd offset in its containing
packet is irrelevant since what matters is whether `p` itself is even or
odd, which is an independent question.
> Dmitry Eremin-Solenikov(lumag) wrote:
> Calculation starts at address p, but an offset inside packet might be odd
> or even. If it is odd, we need to accomodate. Consider the code that uses
> it.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> The calculation starts from the address passed in as `p`. Whether that
>> address is odd or even doesn't need a separate parameter to determine.
>> The various ODP packet routines that return packet addresses, with the
>> exception of `odp_packet_align()`, make no guarantees as to the
>> alignment of any address they return.
Re: [lng-odp] [PATCH v3][WIP] Validate checksums during packet parsing
Dmitry Eremin-Solenikov(lumag) replied on github web page:
platform/linux-generic/include/odp_chksum_internal.h
line 24
@@ -0,0 +1,59 @@
+/* Copyright (c) 2013, Linaro Limited
+ * All rights reserved.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+
+/**
+ * @file
+ *
+ * ODP checksum - implementation internal
+ */
+
+#ifndef ODP_CHKSUM_INTERNAL_H_
+#define ODP_CHKSUM_INTERNAL_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/* Simple implementation of ones complement sum.
+ * Based on RFC1071 and its errata.
+ */
+static uint32_t _odp_chksum_ones_comp16_32(const void *p, uint32_t len,
+ odp_bool_t odd_offset)
Comment:
Hmm. Should we create a list of all possible unaligned access locations?
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> OK, I see what you're saying now. But it looks like we need to consider both.
> The `odd_offset` parameter is saying whether the (continued) checksum needs
> to insert a leading zero into the calculation, however it's still true that
> you have to take into account whether `p` is even or odd since that can also
> vary independently. You can't fetch a `uint16_t` value from an odd byte
> address unless your CPU supports unaligned data accesses. Even for those
> machines that do, this typically results in a significant performance penalty.
>
> But now we have another problem. If at entry `p` is even and `odd_offset` is
> true, meaning that the previous segment contained an odd number of bytes,
> then assuming this buffer is `{c, d, e, ...}` and we've added in `{0, c}` to
> account for the `odd_offset`, the next 16-bit value we want to sum is `{d,
> e}`, but `d` is on an odd address and `e` is on an even address so we can't
> fetch them as a `uint16_t` but need to assemble them from two `uint8_t`
> values. So it sounds like we need to factor out the logic required to fetch
> the next 16 bits to sum from the main loop:
> ```
> static inline uint16_t data16(void *p)
> {
> if ((uintptr_t)p % 2)
> return (*((uint8_t *)p) << 8) + (*((uint8_t *)p + 1));
>
> return *((uint16_t *)p);
> }
> ```
> This can then be added to the main logic:
> ```
> static uint32_t _odp_chksum_ones_comp_16_32(const void *p, uint32_t len,
> odp_bool_t odd_offset)
> {
> void *data = p;
> uint32_t sum = 0;
>
> if (len > 0 && odd_offset) {
>sum = *((uint8_t *)data++);
>len--;
> }
>
> while (len > 1) {
> sum += data16(data);
> data += 2;
> len -= 2;
> }
>
>...etc.
> }
> ```
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> Calculation of checksum starts at `l4_offset`. If for some reason this
>> segment has odd amount of bytes after `l4_offset`, then checksumming will
>> use that byte as `{b, 0}` `uint16_t` value. However calculation of checksum
>> for the next segment (with first bytes `{c, d, e...}`) can not start as
>> usual (`{c ,d}`, `{e, }). It should start as `{0, c}`, `{d, e}`,
>> This is an odd offset. An offset in checksum calculation bytestream. Feel
>> free to propose better name.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> What offset? The calculation is for `len` bytes starting at address `p`.
>>> Whether `p` is the address of an even or odd offset in its containing
>>> packet is irrelevant since what matters is whether `p` itself is even or
>>> odd, which is an independent question.
Dmitry Eremin-Solenikov(lumag) wrote:
Calculation starts at address p, but an offset inside packet might be odd
or even. If it is odd, we need to accomodate. Consider the code that uses
it.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> The calculation starts from the address passed in as `p`. Whether that
> address is odd or even doesn't need a separate parameter to determine.
> The various ODP packet routines that return packet addresses, with the
> exception of `odp_packet_align()`, make no guarantees as to the alignment
> of any address they return.
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> This is true. I'll fix this issue.
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> Yes. However depending on the endianness of the host, it should be
>>> converted to a different host u16 value. It is a pair `{0, proto}`. On
>>> BE it becomes just `proto` u16, on LE it is equal to `proto << 8`.
Dmitry Eremin-Solenikov(lumag) wrote:
sorry. odd_offset references to an offset inside the packet. If you
start calculating checksum from odd offset, you have to insert
additional 0.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> The protocol field in the pseudo-header is simply the 8-bit IP
> protocol field considered as a 16-bit value. It's also in network
> byte order so these two have the same
Re: [lng-odp] [PATCH v3][WIP] Validate checksums during packet parsing
Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/include/odp_chksum_internal.h
line 24
@@ -0,0 +1,59 @@
+/* Copyright (c) 2013, Linaro Limited
+ * All rights reserved.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+
+/**
+ * @file
+ *
+ * ODP checksum - implementation internal
+ */
+
+#ifndef ODP_CHKSUM_INTERNAL_H_
+#define ODP_CHKSUM_INTERNAL_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/* Simple implementation of ones complement sum.
+ * Based on RFC1071 and its errata.
+ */
+static uint32_t _odp_chksum_ones_comp16_32(const void *p, uint32_t len,
+ odp_bool_t odd_offset)
Comment:
OK, I see what you're saying now. But it looks like we need to consider both.
The `odd_offset` parameter is saying whether the (continued) checksum needs to
insert a leading zero into the calculation, however it's still true that you
have to take into account whether `p` is even or odd since that can also vary
independently. You can't fetch a `uint16_t` value from an odd byte address
unless your CPU supports unaligned data accesses. Even for those machines that
do, this typically results in a significant performance penalty.
But now we have another problem. If at entry `p` is even and `odd_offset` is
true, meaning that the previous segment contained an odd number of bytes, then
assuming this buffer is `{c, d, e, ...}` and we've added in `{0, c}` to account
for the `odd_offset`, the next 16-bit value we want to sum is `{d, e}`, but `d`
is on an odd address and `e` is on an even address so we can't fetch them as a
`uint16_t` but need to assemble them from two `uint8_t` values. So it sounds
like we need to factor out the logic required to fetch the next 16 bits to sum
from the main loop:
```
static inline uint16_t data16(void *p)
{
if ((uintptr_t)p % 2)
return (*((uint8_t *)p) << 8) + (*((uint8_t *)p + 1));
return *((uint16_t *)p);
}
```
This can then be added to the main logic:
```
static uint32_t _odp_chksum_ones_comp_16_32(const void *p, uint32_t len,
odp_bool_t odd_offset)
{
void *data = p;
uint32_t sum = 0;
if (len > 0 && odd_offset) {
sum = *((uint8_t *)data++);
len--;
}
while (len > 1) {
sum += data16(data);
data += 2;
len -= 2;
}
...etc.
}
```
> Dmitry Eremin-Solenikov(lumag) wrote:
> Calculation of checksum starts at `l4_offset`. If for some reason this
> segment has odd amount of bytes after `l4_offset`, then checksumming will use
> that byte as `{b, 0}` `uint16_t` value. However calculation of checksum for
> the next segment (with first bytes `{c, d, e...}`) can not start as usual
> (`{c ,d}`, `{e, }). It should start as `{0, c}`, `{d, e}`, This is
> an odd offset. An offset in checksum calculation bytestream. Feel free to
> propose better name.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> What offset? The calculation is for `len` bytes starting at address `p`.
>> Whether `p` is the address of an even or odd offset in its containing packet
>> is irrelevant since what matters is whether `p` itself is even or odd, which
>> is an independent question.
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> Calculation starts at address p, but an offset inside packet might be odd
>>> or even. If it is odd, we need to accomodate. Consider the code that uses
>>> it.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
The calculation starts from the address passed in as `p`. Whether that
address is odd or even doesn't need a separate parameter to determine. The
various ODP packet routines that return packet addresses, with the
exception of `odp_packet_align()`, make no guarantees as to the alignment
of any address they return.
> Dmitry Eremin-Solenikov(lumag) wrote:
> This is true. I'll fix this issue.
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> Yes. However depending on the endianness of the host, it should be
>> converted to a different host u16 value. It is a pair `{0, proto}`. On
>> BE it becomes just `proto` u16, on LE it is equal to `proto << 8`.
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> sorry. odd_offset references to an offset inside the packet. If you
>>> start calculating checksum from odd offset, you have to insert
>>> additional 0.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
The protocol field in the pseudo-header is simply the 8-bit IP
protocol field considered as a 16-bit value. It's also in network byte
order so these two have the same bit value.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> I don't understand that response. Can you elaborate?
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> Yes, but the adding word is not (proto is the lat byte
Re: [lng-odp] [PATCH v3][WIP] Validate checksums during packet parsing
Dmitry Eremin-Solenikov(lumag) replied on github web page:
platform/linux-generic/include/odp_chksum_internal.h
line 24
@@ -0,0 +1,59 @@
+/* Copyright (c) 2013, Linaro Limited
+ * All rights reserved.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+
+/**
+ * @file
+ *
+ * ODP checksum - implementation internal
+ */
+
+#ifndef ODP_CHKSUM_INTERNAL_H_
+#define ODP_CHKSUM_INTERNAL_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/* Simple implementation of ones complement sum.
+ * Based on RFC1071 and its errata.
+ */
+static uint32_t _odp_chksum_ones_comp16_32(const void *p, uint32_t len,
+ odp_bool_t odd_offset)
Comment:
Calculation of checksum starts at `l4_offset`. If for some reason this segment
has odd amount of bytes after `l4_offset`, then checksumming will use that byte
as `{b, 0}` `uint16_t` value. However calculation of checksum for the next
segment (with first bytes `{c, d, e...}`) can not start as usual (`{c ,d}`,
`{e, }). It should start as `{0, c}`, `{d, e}`, This is an odd offset.
An offset in checksum calculation bytestream. Feel free to propose better name.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> What offset? The calculation is for `len` bytes starting at address `p`.
> Whether `p` is the address of an even or odd offset in its containing packet
> is irrelevant since what matters is whether `p` itself is even or odd, which
> is an independent question.
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> Calculation starts at address p, but an offset inside packet might be odd or
>> even. If it is odd, we need to accomodate. Consider the code that uses it.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> The calculation starts from the address passed in as `p`. Whether that
>>> address is odd or even doesn't need a separate parameter to determine. The
>>> various ODP packet routines that return packet addresses, with the
>>> exception of `odp_packet_align()`, make no guarantees as to the alignment
>>> of any address they return.
Dmitry Eremin-Solenikov(lumag) wrote:
This is true. I'll fix this issue.
> Dmitry Eremin-Solenikov(lumag) wrote:
> Yes. However depending on the endianness of the host, it should be
> converted to a different host u16 value. It is a pair `{0, proto}`. On BE
> it becomes just `proto` u16, on LE it is equal to `proto << 8`.
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> sorry. odd_offset references to an offset inside the packet. If you
>> start calculating checksum from odd offset, you have to insert
>> additional 0.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> The protocol field in the pseudo-header is simply the 8-bit IP protocol
>>> field considered as a 16-bit value. It's also in network byte order so
>>> these two have the same bit value.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
I don't understand that response. Can you elaborate?
> Dmitry Eremin-Solenikov(lumag) wrote:
> Yes, but the adding word is not (proto is the lat byte in pseudo
> header).
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> It is an offset insite packet.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Drop last parameter (`odd_offset`)
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
Again `ipv6->next_hdr` is a `uint8_t` field, so is endian-neutral.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> `ipv4->proto` is a `uint8_t` field here, so why is this needed?
> These are the same independent of endianness.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Drop last parameter (`odd_offset`)
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Drop last parameter (`odd_offset`)
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
This erroneously increments `data` by 2 when you want to
increment it by 1 here so that it points to `p + 1`, not `p +
2`. Correct is `data = (const uint16_t *)((uintptr_t)p + 1);`
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> The `odd_offset` parameter is unnecessary here. What matters
> is whether `p` starts on an even or odd byte boundary since
> that determines whether you can fetch `data` as `uint16_t`
> quantities without making unaligned storage references. Each
> call to `_odp_chksum_ones_comp16_32()` is independent in this
> regard, so it must always be determined anew for each call.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> `if (len > 1 && (uintptr_t)p % 2) ...`
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Drop use of `odd_offset` as
Re: [lng-odp] [PATCH v3][WIP] Validate checksums during packet parsing
Dmitry Eremin-Solenikov(lumag) replied on github web page:
platform/linux-generic/include/odp_chksum_internal.h
line 24
@@ -0,0 +1,59 @@
+/* Copyright (c) 2013, Linaro Limited
+ * All rights reserved.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+
+/**
+ * @file
+ *
+ * ODP checksum - implementation internal
+ */
+
+#ifndef ODP_CHKSUM_INTERNAL_H_
+#define ODP_CHKSUM_INTERNAL_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/* Simple implementation of ones complement sum.
+ * Based on RFC1071 and its errata.
+ */
+static uint32_t _odp_chksum_ones_comp16_32(const void *p, uint32_t len,
+ odp_bool_t odd_offset)
Comment:
sorry. odd_offset references to an offset inside the packet. If you start
calculating checksum from odd offset, you have to insert additional 0.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> The protocol field in the pseudo-header is simply the 8-bit IP protocol field
> considered as a 16-bit value. It's also in network byte order so these two
> have the same bit value.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> I don't understand that response. Can you elaborate?
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> Yes, but the adding word is not (proto is the lat byte in pseudo header).
Dmitry Eremin-Solenikov(lumag) wrote:
It is an offset insite packet.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Drop last parameter (`odd_offset`)
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Again `ipv6->next_hdr` is a `uint8_t` field, so is endian-neutral.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> `ipv4->proto` is a `uint8_t` field here, so why is this needed? These
>>> are the same independent of endianness.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
Drop last parameter (`odd_offset`)
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Drop last parameter (`odd_offset`)
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> This erroneously increments `data` by 2 when you want to increment
>> it by 1 here so that it points to `p + 1`, not `p + 2`. Correct is
>> `data = (const uint16_t *)((uintptr_t)p + 1);`
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> The `odd_offset` parameter is unnecessary here. What matters is
>>> whether `p` starts on an even or odd byte boundary since that
>>> determines whether you can fetch `data` as `uint16_t` quantities
>>> without making unaligned storage references. Each call to
>>> `_odp_chksum_ones_comp16_32()` is independent in this regard, so it
>>> must always be determined anew for each call.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
`if (len > 1 && (uintptr_t)p % 2) ...`
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Drop use of `odd_offset` as noted earlier.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Checkpatch flag:
>> ```
>> WARNING: line over 80 characters
>> #102: FILE: platform/linux-generic/odp_packet.c:2204:
>> +ip_proto = parse_ipv4(prs, , ,
>> frame_len, chksums);
>> total: 0 errors, 1 warnings, 0 checks, 163 lines checked
>> ```
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> 2018
https://github.com/Linaro/odp/pull/389#discussion_r162241315
updated_at 2018-01-18 03:23:35
Re: [lng-odp] [PATCH v3][WIP] Validate checksums during packet parsing
Dmitry Eremin-Solenikov(lumag) replied on github web page:
platform/linux-generic/odp_packet.c
line 85
@@ -2052,6 +2083,18 @@ static inline uint8_t parse_ipv4(packet_parser_t *prs,
const uint8_t **parseptr,
*offset += ihl * 4;
*parseptr += ihl * 4;
+ if (chksums.chksum.udp || chksums.chksum.tcp) {
+ l4_part_sum = _odp_chksum_ones_comp16_32(
+ (const uint16_t *)>src_addr,
+ 2 * _ODP_IPV4ADDR_LEN, false);
+#if ODP_BYTE_ORDER == ODP_BIG_ENDIAN
+ l4_part_sum += ipv4->proto;
+#else
+ l4_part_sum += ((uint16_t)ipv4->proto) << 8;
+#endif
Comment:
Yes. However depending on the endianness of the host, it should be converted to
a different host u16 value. It is a pair `{0, proto}`. On BE it becomes just
`proto` u16, on LE it is equal to `proto << 8`.
> Dmitry Eremin-Solenikov(lumag) wrote:
> sorry. odd_offset references to an offset inside the packet. If you start
> calculating checksum from odd offset, you have to insert additional 0.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> The protocol field in the pseudo-header is simply the 8-bit IP protocol
>> field considered as a 16-bit value. It's also in network byte order so these
>> two have the same bit value.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> I don't understand that response. Can you elaborate?
Dmitry Eremin-Solenikov(lumag) wrote:
Yes, but the adding word is not (proto is the lat byte in pseudo header).
> Dmitry Eremin-Solenikov(lumag) wrote:
> It is an offset insite packet.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Drop last parameter (`odd_offset`)
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Again `ipv6->next_hdr` is a `uint8_t` field, so is endian-neutral.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
`ipv4->proto` is a `uint8_t` field here, so why is this needed? These
are the same independent of endianness.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Drop last parameter (`odd_offset`)
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Drop last parameter (`odd_offset`)
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> This erroneously increments `data` by 2 when you want to increment
>>> it by 1 here so that it points to `p + 1`, not `p + 2`. Correct is
>>> `data = (const uint16_t *)((uintptr_t)p + 1);`
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
The `odd_offset` parameter is unnecessary here. What matters is
whether `p` starts on an even or odd byte boundary since that
determines whether you can fetch `data` as `uint16_t` quantities
without making unaligned storage references. Each call to
`_odp_chksum_ones_comp16_32()` is independent in this regard, so
it must always be determined anew for each call.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> `if (len > 1 && (uintptr_t)p % 2) ...`
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Drop use of `odd_offset` as noted earlier.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Checkpatch flag:
>>> ```
>>> WARNING: line over 80 characters
>>> #102: FILE: platform/linux-generic/odp_packet.c:2204:
>>> + ip_proto = parse_ipv4(prs, , ,
>>> frame_len, chksums);
>>> total: 0 errors, 1 warnings, 0 checks, 163 lines checked
>>> ```
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
2018
https://github.com/Linaro/odp/pull/389#discussion_r162241718
updated_at 2018-01-18 03:28:46
Re: [lng-odp] [PATCH v3][WIP] Validate checksums during packet parsing
Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/odp_packet.c
line 85
@@ -2052,6 +2083,18 @@ static inline uint8_t parse_ipv4(packet_parser_t *prs,
const uint8_t **parseptr,
*offset += ihl * 4;
*parseptr += ihl * 4;
+ if (chksums.chksum.udp || chksums.chksum.tcp) {
+ l4_part_sum = _odp_chksum_ones_comp16_32(
+ (const uint16_t *)>src_addr,
+ 2 * _ODP_IPV4ADDR_LEN, false);
+#if ODP_BYTE_ORDER == ODP_BIG_ENDIAN
+ l4_part_sum += ipv4->proto;
+#else
+ l4_part_sum += ((uint16_t)ipv4->proto) << 8;
+#endif
Comment:
The protocol field in the pseudo-header is simply the 8-bit IP protocol field
considered as a 16-bit value. It's also in network byte order so these two have
the same bit value.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> I don't understand that response. Can you elaborate?
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> Yes, but the adding word is not (proto is the lat byte in pseudo header).
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> It is an offset insite packet.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
Drop last parameter (`odd_offset`)
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Again `ipv6->next_hdr` is a `uint8_t` field, so is endian-neutral.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> `ipv4->proto` is a `uint8_t` field here, so why is this needed? These
>> are the same independent of endianness.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Drop last parameter (`odd_offset`)
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
Drop last parameter (`odd_offset`)
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> This erroneously increments `data` by 2 when you want to increment it
> by 1 here so that it points to `p + 1`, not `p + 2`. Correct is `data
> = (const uint16_t *)((uintptr_t)p + 1);`
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> The `odd_offset` parameter is unnecessary here. What matters is
>> whether `p` starts on an even or odd byte boundary since that
>> determines whether you can fetch `data` as `uint16_t` quantities
>> without making unaligned storage references. Each call to
>> `_odp_chksum_ones_comp16_32()` is independent in this regard, so it
>> must always be determined anew for each call.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> `if (len > 1 && (uintptr_t)p % 2) ...`
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
Drop use of `odd_offset` as noted earlier.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Checkpatch flag:
> ```
> WARNING: line over 80 characters
> #102: FILE: platform/linux-generic/odp_packet.c:2204:
> + ip_proto = parse_ipv4(prs, , ,
> frame_len, chksums);
> total: 0 errors, 1 warnings, 0 checks, 163 lines checked
> ```
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> 2018
https://github.com/Linaro/odp/pull/389#discussion_r162233042
updated_at 2018-01-18 02:15:45
Re: [lng-odp] [PATCH v3][WIP] Validate checksums during packet parsing
Dmitry Eremin-Solenikov(lumag) replied on github web page:
platform/linux-generic/include/odp_chksum_internal.h
line 24
@@ -0,0 +1,59 @@
+/* Copyright (c) 2013, Linaro Limited
+ * All rights reserved.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+
+/**
+ * @file
+ *
+ * ODP checksum - implementation internal
+ */
+
+#ifndef ODP_CHKSUM_INTERNAL_H_
+#define ODP_CHKSUM_INTERNAL_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/* Simple implementation of ones complement sum.
+ * Based on RFC1071 and its errata.
+ */
+static uint32_t _odp_chksum_ones_comp16_32(const void *p, uint32_t len,
+ odp_bool_t odd_offset)
Comment:
Calculation starts at address p, but an offset inside packet might be odd or
even. If it is odd, we need to accomodate. Consider the code that uses it.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> The calculation starts from the address passed in as `p`. Whether that
> address is odd or even doesn't need a separate parameter to determine. The
> various ODP packet routines that return packet addresses, with the exception
> of `odp_packet_align()`, make no guarantees as to the alignment of any
> address they return.
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> This is true. I'll fix this issue.
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> Yes. However depending on the endianness of the host, it should be
>>> converted to a different host u16 value. It is a pair `{0, proto}`. On BE
>>> it becomes just `proto` u16, on LE it is equal to `proto << 8`.
Dmitry Eremin-Solenikov(lumag) wrote:
sorry. odd_offset references to an offset inside the packet. If you start
calculating checksum from odd offset, you have to insert additional 0.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> The protocol field in the pseudo-header is simply the 8-bit IP protocol
> field considered as a 16-bit value. It's also in network byte order so
> these two have the same bit value.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> I don't understand that response. Can you elaborate?
>>> Dmitry Eremin-Solenikov(lumag) wrote:
>>> Yes, but the adding word is not (proto is the lat byte in pseudo
>>> header).
Dmitry Eremin-Solenikov(lumag) wrote:
It is an offset insite packet.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Drop last parameter (`odd_offset`)
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Again `ipv6->next_hdr` is a `uint8_t` field, so is endian-neutral.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> `ipv4->proto` is a `uint8_t` field here, so why is this needed?
>>> These are the same independent of endianness.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
Drop last parameter (`odd_offset`)
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Drop last parameter (`odd_offset`)
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> This erroneously increments `data` by 2 when you want to
>> increment it by 1 here so that it points to `p + 1`, not `p +
>> 2`. Correct is `data = (const uint16_t *)((uintptr_t)p + 1);`
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> The `odd_offset` parameter is unnecessary here. What matters is
>>> whether `p` starts on an even or odd byte boundary since that
>>> determines whether you can fetch `data` as `uint16_t`
>>> quantities without making unaligned storage references. Each
>>> call to `_odp_chksum_ones_comp16_32()` is independent in this
>>> regard, so it must always be determined anew for each call.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
`if (len > 1 && (uintptr_t)p % 2) ...`
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Drop use of `odd_offset` as noted earlier.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Checkpatch flag:
>> ```
>> WARNING: line over 80 characters
>> #102: FILE: platform/linux-generic/odp_packet.c:2204:
>> +ip_proto = parse_ipv4(prs, , ,
>> frame_len, chksums);
>> total: 0 errors, 1 warnings, 0 checks, 163 lines checked
>> ```
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> 2018
https://github.com/Linaro/odp/pull/389#discussion_r162245601
updated_at 2018-01-18 04:09:04
Re: [lng-odp] [PATCH v3][WIP] Validate checksums during packet parsing
Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/include/odp_chksum_internal.h
line 24
@@ -0,0 +1,59 @@
+/* Copyright (c) 2013, Linaro Limited
+ * All rights reserved.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+
+/**
+ * @file
+ *
+ * ODP checksum - implementation internal
+ */
+
+#ifndef ODP_CHKSUM_INTERNAL_H_
+#define ODP_CHKSUM_INTERNAL_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/* Simple implementation of ones complement sum.
+ * Based on RFC1071 and its errata.
+ */
+static uint32_t _odp_chksum_ones_comp16_32(const void *p, uint32_t len,
+ odp_bool_t odd_offset)
Comment:
I don't understand that response. Can you elaborate?
> Dmitry Eremin-Solenikov(lumag) wrote:
> Yes, but the adding word is not (proto is the lat byte in pseudo header).
>> Dmitry Eremin-Solenikov(lumag) wrote:
>> It is an offset insite packet.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Drop last parameter (`odd_offset`)
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
Again `ipv6->next_hdr` is a `uint8_t` field, so is endian-neutral.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> `ipv4->proto` is a `uint8_t` field here, so why is this needed? These are
> the same independent of endianness.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Drop last parameter (`odd_offset`)
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Drop last parameter (`odd_offset`)
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
This erroneously increments `data` by 2 when you want to increment it
by 1 here so that it points to `p + 1`, not `p + 2`. Correct is `data
= (const uint16_t *)((uintptr_t)p + 1);`
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> The `odd_offset` parameter is unnecessary here. What matters is
> whether `p` starts on an even or odd byte boundary since that
> determines whether you can fetch `data` as `uint16_t` quantities
> without making unaligned storage references. Each call to
> `_odp_chksum_ones_comp16_32()` is independent in this regard, so it
> must always be determined anew for each call.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> `if (len > 1 && (uintptr_t)p % 2) ...`
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Drop use of `odd_offset` as noted earlier.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
Checkpatch flag:
```
WARNING: line over 80 characters
#102: FILE: platform/linux-generic/odp_packet.c:2204:
+ ip_proto = parse_ipv4(prs, , ,
frame_len, chksums);
total: 0 errors, 1 warnings, 0 checks, 163 lines checked
```
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> 2018
https://github.com/Linaro/odp/pull/389#discussion_r162231240
updated_at 2018-01-18 02:15:45
Re: [lng-odp] [PATCH v3][WIP] Validate checksums during packet parsing
Dmitry Eremin-Solenikov(lumag) replied on github web page:
platform/linux-generic/include/odp_chksum_internal.h
line 24
@@ -0,0 +1,59 @@
+/* Copyright (c) 2013, Linaro Limited
+ * All rights reserved.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+
+/**
+ * @file
+ *
+ * ODP checksum - implementation internal
+ */
+
+#ifndef ODP_CHKSUM_INTERNAL_H_
+#define ODP_CHKSUM_INTERNAL_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/* Simple implementation of ones complement sum.
+ * Based on RFC1071 and its errata.
+ */
+static uint32_t _odp_chksum_ones_comp16_32(const void *p, uint32_t len,
+ odp_bool_t odd_offset)
Comment:
It is an offset insite packet.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Drop last parameter (`odd_offset`)
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Again `ipv6->next_hdr` is a `uint8_t` field, so is endian-neutral.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> `ipv4->proto` is a `uint8_t` field here, so why is this needed? These are
>>> the same independent of endianness.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
Drop last parameter (`odd_offset`)
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Drop last parameter (`odd_offset`)
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> This erroneously increments `data` by 2 when you want to increment it by
>> 1 here so that it points to `p + 1`, not `p + 2`. Correct is `data =
>> (const uint16_t *)((uintptr_t)p + 1);`
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> The `odd_offset` parameter is unnecessary here. What matters is whether
>>> `p` starts on an even or odd byte boundary since that determines
>>> whether you can fetch `data` as `uint16_t` quantities without making
>>> unaligned storage references. Each call to
>>> `_odp_chksum_ones_comp16_32()` is independent in this regard, so it
>>> must always be determined anew for each call.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
`if (len > 1 && (uintptr_t)p % 2) ...`
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Drop use of `odd_offset` as noted earlier.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Checkpatch flag:
>> ```
>> WARNING: line over 80 characters
>> #102: FILE: platform/linux-generic/odp_packet.c:2204:
>> +ip_proto = parse_ipv4(prs, , ,
>> frame_len, chksums);
>> total: 0 errors, 1 warnings, 0 checks, 163 lines checked
>> ```
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> 2018
https://github.com/Linaro/odp/pull/389#discussion_r161940611
updated_at 2018-01-17 02:10:12
Re: [lng-odp] [PATCH v3][WIP] Validate checksums during packet parsing
Dmitry Eremin-Solenikov(lumag) replied on github web page:
platform/linux-generic/odp_packet.c
line 85
@@ -2052,6 +2083,18 @@ static inline uint8_t parse_ipv4(packet_parser_t *prs,
const uint8_t **parseptr,
*offset += ihl * 4;
*parseptr += ihl * 4;
+ if (chksums.chksum.udp || chksums.chksum.tcp) {
+ l4_part_sum = _odp_chksum_ones_comp16_32(
+ (const uint16_t *)>src_addr,
+ 2 * _ODP_IPV4ADDR_LEN, false);
+#if ODP_BYTE_ORDER == ODP_BIG_ENDIAN
+ l4_part_sum += ipv4->proto;
+#else
+ l4_part_sum += ((uint16_t)ipv4->proto) << 8;
+#endif
Comment:
Yes, but the adding word is not (proto is the lat byte in pseudo header).
> Dmitry Eremin-Solenikov(lumag) wrote:
> It is an offset insite packet.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Drop last parameter (`odd_offset`)
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Again `ipv6->next_hdr` is a `uint8_t` field, so is endian-neutral.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
`ipv4->proto` is a `uint8_t` field here, so why is this needed? These are
the same independent of endianness.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Drop last parameter (`odd_offset`)
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Drop last parameter (`odd_offset`)
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> This erroneously increments `data` by 2 when you want to increment it
>>> by 1 here so that it points to `p + 1`, not `p + 2`. Correct is `data =
>>> (const uint16_t *)((uintptr_t)p + 1);`
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
The `odd_offset` parameter is unnecessary here. What matters is
whether `p` starts on an even or odd byte boundary since that
determines whether you can fetch `data` as `uint16_t` quantities
without making unaligned storage references. Each call to
`_odp_chksum_ones_comp16_32()` is independent in this regard, so it
must always be determined anew for each call.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> `if (len > 1 && (uintptr_t)p % 2) ...`
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Drop use of `odd_offset` as noted earlier.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Checkpatch flag:
>>> ```
>>> WARNING: line over 80 characters
>>> #102: FILE: platform/linux-generic/odp_packet.c:2204:
>>> + ip_proto = parse_ipv4(prs, , ,
>>> frame_len, chksums);
>>> total: 0 errors, 1 warnings, 0 checks, 163 lines checked
>>> ```
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
2018
https://github.com/Linaro/odp/pull/389#discussion_r161940706
updated_at 2018-01-17 02:11:01
Re: [lng-odp] [PATCH v3][WIP] Validate checksums during packet parsing
Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/odp_packet.c
line 160
@@ -2155,14 +2212,35 @@ static inline void parse_tcp(packet_parser_t *prs,
/**
* Parser helper function for UDP
*/
-static inline void parse_udp(packet_parser_t *prs,
-const uint8_t **parseptr, uint32_t *offset)
+static inline void parse_udp(packet_parser_t *prs, const uint8_t **parseptr,
+uint32_t *offset, odp_proto_chksums_t chksums)
{
const _odp_udphdr_t *udp = (const _odp_udphdr_t *)*parseptr;
uint32_t udplen = odp_be_to_cpu_16(udp->length);
- if (odp_unlikely(udplen < sizeof(_odp_udphdr_t)))
+ if (odp_unlikely(udplen < sizeof(_odp_udphdr_t))) {
prs->error_flags.udp_err = 1;
+ return;
+ }
+
+ if (chksums.chksum.udp &&
+ !prs->input_flags.ipfrag) {
+ if (udp->chksum == 0) {
+ prs->input_flags.l4_chksum_done =
+ (prs->input_flags.ipv4 != 1);
+ prs->error_flags.l4_chksum =
+ (prs->input_flags.ipv4 != 1);
+ } else {
+ prs->input_flags.l4_chksum_done = 1;
+ prs->l4_part_sum += udp->length;
+ /* Do not include checksum into partial sum */
+ prs->l4_part_sum += _odp_chksum_ones_comp16_32(
+ (const void *)udp,
+ _ODP_UDPHDR_LEN - 2,
+ false);
Comment:
Drop last parameter (`odd_offset`)
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Again `ipv6->next_hdr` is a `uint8_t` field, so is endian-neutral.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> `ipv4->proto` is a `uint8_t` field here, so why is this needed? These are
>> the same independent of endianness.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Drop last parameter (`odd_offset`)
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
Drop last parameter (`odd_offset`)
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> This erroneously increments `data` by 2 when you want to increment it by
> 1 here so that it points to `p + 1`, not `p + 2`. Correct is `data =
> (const uint16_t *)((uintptr_t)p + 1);`
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> The `odd_offset` parameter is unnecessary here. What matters is whether
>> `p` starts on an even or odd byte boundary since that determines whether
>> you can fetch `data` as `uint16_t` quantities without making unaligned
>> storage references. Each call to `_odp_chksum_ones_comp16_32()` is
>> independent in this regard, so it must always be determined anew for
>> each call.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> `if (len > 1 && (uintptr_t)p % 2) ...`
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
Drop use of `odd_offset` as noted earlier.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Checkpatch flag:
> ```
> WARNING: line over 80 characters
> #102: FILE: platform/linux-generic/odp_packet.c:2204:
> + ip_proto = parse_ipv4(prs, , ,
> frame_len, chksums);
> total: 0 errors, 1 warnings, 0 checks, 163 lines checked
> ```
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> 2018
https://github.com/Linaro/odp/pull/389#discussion_r161937032
updated_at 2018-01-17 01:41:46
Re: [lng-odp] [PATCH v3][WIP] Validate checksums during packet parsing
Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/odp_packet.c
line 85
@@ -2052,6 +2083,18 @@ static inline uint8_t parse_ipv4(packet_parser_t *prs,
const uint8_t **parseptr,
*offset += ihl * 4;
*parseptr += ihl * 4;
+ if (chksums.chksum.udp || chksums.chksum.tcp) {
+ l4_part_sum = _odp_chksum_ones_comp16_32(
+ (const uint16_t *)>src_addr,
+ 2 * _ODP_IPV4ADDR_LEN, false);
+#if ODP_BYTE_ORDER == ODP_BIG_ENDIAN
+ l4_part_sum += ipv4->proto;
+#else
+ l4_part_sum += ((uint16_t)ipv4->proto) << 8;
+#endif
Comment:
`ipv4->proto` is a `uint8_t` field here, so why is this needed? These are the
same independent of endianness.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Drop last parameter (`odd_offset`)
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Drop last parameter (`odd_offset`)
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> This erroneously increments `data` by 2 when you want to increment it by 1
>>> here so that it points to `p + 1`, not `p + 2`. Correct is `data = (const
>>> uint16_t *)((uintptr_t)p + 1);`
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
The `odd_offset` parameter is unnecessary here. What matters is whether
`p` starts on an even or odd byte boundary since that determines whether
you can fetch `data` as `uint16_t` quantities without making unaligned
storage references. Each call to `_odp_chksum_ones_comp16_32()` is
independent in this regard, so it must always be determined anew for each
call.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> `if (len > 1 && (uintptr_t)p % 2) ...`
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Drop use of `odd_offset` as noted earlier.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Checkpatch flag:
>>> ```
>>> WARNING: line over 80 characters
>>> #102: FILE: platform/linux-generic/odp_packet.c:2204:
>>> + ip_proto = parse_ipv4(prs, , ,
>>> frame_len, chksums);
>>> total: 0 errors, 1 warnings, 0 checks, 163 lines checked
>>> ```
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
2018
https://github.com/Linaro/odp/pull/389#discussion_r161936805
updated_at 2018-01-17 01:41:46
Re: [lng-odp] [PATCH v3][WIP] Validate checksums during packet parsing
Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/odp_packet.c
line 121
@@ -2099,6 +2144,18 @@ static inline uint8_t parse_ipv6(packet_parser_t *prs,
const uint8_t **parseptr,
*offset += sizeof(_odp_ipv6hdr_t);
*parseptr += sizeof(_odp_ipv6hdr_t);
+ if (chksums.chksum.udp || chksums.chksum.tcp) {
+ l4_part_sum = _odp_chksum_ones_comp16_32(
+ (const uint16_t *)(uintptr_t)>src_addr,
+ 2 * _ODP_IPV6ADDR_LEN, false);
+#if ODP_BYTE_ORDER == ODP_BIG_ENDIAN
+ l4_part_sum += ipv6->next_hdr;
+#else
+ l4_part_sum += ((uint16_t)ipv6->next_hdr) << 8;
+#endif
Comment:
Again `ipv6->next_hdr` is a `uint8_t` field, so is endian-neutral.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> `ipv4->proto` is a `uint8_t` field here, so why is this needed? These are the
> same independent of endianness.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Drop last parameter (`odd_offset`)
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Drop last parameter (`odd_offset`)
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
This erroneously increments `data` by 2 when you want to increment it by 1
here so that it points to `p + 1`, not `p + 2`. Correct is `data = (const
uint16_t *)((uintptr_t)p + 1);`
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> The `odd_offset` parameter is unnecessary here. What matters is whether
> `p` starts on an even or odd byte boundary since that determines whether
> you can fetch `data` as `uint16_t` quantities without making unaligned
> storage references. Each call to `_odp_chksum_ones_comp16_32()` is
> independent in this regard, so it must always be determined anew for each
> call.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> `if (len > 1 && (uintptr_t)p % 2) ...`
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Drop use of `odd_offset` as noted earlier.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
Checkpatch flag:
```
WARNING: line over 80 characters
#102: FILE: platform/linux-generic/odp_packet.c:2204:
+ ip_proto = parse_ipv4(prs, , ,
frame_len, chksums);
total: 0 errors, 1 warnings, 0 checks, 163 lines checked
```
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> 2018
https://github.com/Linaro/odp/pull/389#discussion_r161936908
updated_at 2018-01-17 01:41:46
Re: [lng-odp] [PATCH v3][WIP] Validate checksums during packet parsing
Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/odp_packet.c
line 80
@@ -2052,6 +2083,18 @@ static inline uint8_t parse_ipv4(packet_parser_t *prs,
const uint8_t **parseptr,
*offset += ihl * 4;
*parseptr += ihl * 4;
+ if (chksums.chksum.udp || chksums.chksum.tcp) {
+ l4_part_sum = _odp_chksum_ones_comp16_32(
+ (const uint16_t *)>src_addr,
+ 2 * _ODP_IPV4ADDR_LEN, false);
Comment:
Drop last parameter (`odd_offset`)
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> This erroneously increments `data` by 2 when you want to increment it by 1
> here so that it points to `p + 1`, not `p + 2`. Correct is `data = (const
> uint16_t *)((uintptr_t)p + 1);`
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> The `odd_offset` parameter is unnecessary here. What matters is whether `p`
>> starts on an even or odd byte boundary since that determines whether you can
>> fetch `data` as `uint16_t` quantities without making unaligned storage
>> references. Each call to `_odp_chksum_ones_comp16_32()` is independent in
>> this regard, so it must always be determined anew for each call.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> `if (len > 1 && (uintptr_t)p % 2) ...`
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
Drop use of `odd_offset` as noted earlier.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Checkpatch flag:
> ```
> WARNING: line over 80 characters
> #102: FILE: platform/linux-generic/odp_packet.c:2204:
> + ip_proto = parse_ipv4(prs, , , frame_len,
> chksums);
> total: 0 errors, 1 warnings, 0 checks, 163 lines checked
> ```
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> 2018
https://github.com/Linaro/odp/pull/389#discussion_r161935977
updated_at 2018-01-17 01:41:46
Re: [lng-odp] [PATCH v3][WIP] Validate checksums during packet parsing
Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/odp_packet.c
line 116
@@ -2099,6 +2144,18 @@ static inline uint8_t parse_ipv6(packet_parser_t *prs,
const uint8_t **parseptr,
*offset += sizeof(_odp_ipv6hdr_t);
*parseptr += sizeof(_odp_ipv6hdr_t);
+ if (chksums.chksum.udp || chksums.chksum.tcp) {
+ l4_part_sum = _odp_chksum_ones_comp16_32(
+ (const uint16_t *)(uintptr_t)>src_addr,
+ 2 * _ODP_IPV6ADDR_LEN, false);
Comment:
Drop last parameter (`odd_offset`)
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Drop last parameter (`odd_offset`)
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> This erroneously increments `data` by 2 when you want to increment it by 1
>> here so that it points to `p + 1`, not `p + 2`. Correct is `data = (const
>> uint16_t *)((uintptr_t)p + 1);`
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> The `odd_offset` parameter is unnecessary here. What matters is whether `p`
>>> starts on an even or odd byte boundary since that determines whether you
>>> can fetch `data` as `uint16_t` quantities without making unaligned storage
>>> references. Each call to `_odp_chksum_ones_comp16_32()` is independent in
>>> this regard, so it must always be determined anew for each call.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
`if (len > 1 && (uintptr_t)p % 2) ...`
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Drop use of `odd_offset` as noted earlier.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Checkpatch flag:
>> ```
>> WARNING: line over 80 characters
>> #102: FILE: platform/linux-generic/odp_packet.c:2204:
>> +ip_proto = parse_ipv4(prs, , ,
>> frame_len, chksums);
>> total: 0 errors, 1 warnings, 0 checks, 163 lines checked
>> ```
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> 2018
https://github.com/Linaro/odp/pull/389#discussion_r161936047
updated_at 2018-01-17 01:41:46
Re: [lng-odp] [PATCH v3][WIP] Validate checksums during packet parsing
Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/odp_packet.c
line 31
@@ -1948,6 +1949,35 @@ int _odp_packet_copy_md_to_packet(odp_packet_t srcpkt,
odp_packet_t dstpkt)
return dst_uarea_size < src_uarea_size;
}
+static uint16_t packet_sum_ones_comp16(odp_packet_hdr_t *pkt_hdr,
+ uint32_t offset,
+ uint32_t len)
+{
+ uint32_t sum = pkt_hdr->p.l4_part_sum;
+ odp_bool_t odd_offset = false;
+
+ if (offset + len > pkt_hdr->frame_len)
+ return 0;
+
+ while (len > 0) {
+ uint32_t seglen = 0; /* GCC */
+ void *mapaddr = packet_map(pkt_hdr, offset, , NULL);
+
+ if (seglen < len)
+ seglen = len;
+
+ len -= seglen;
+ sum += _odp_chksum_ones_comp16_32(mapaddr, seglen, odd_offset);
+ odd_offset ^= (seglen % 2);
Comment:
Drop use of `odd_offset` as noted earlier.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Checkpatch flag:
> ```
> WARNING: line over 80 characters
> #102: FILE: platform/linux-generic/odp_packet.c:2204:
> + ip_proto = parse_ipv4(prs, , , frame_len,
> chksums);
> total: 0 errors, 1 warnings, 0 checks, 163 lines checked
> ```
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> 2018
https://github.com/Linaro/odp/pull/389#discussion_r161932843
updated_at 2018-01-17 01:41:46
Re: [lng-odp] [PATCH v3][WIP] Validate checksums during packet parsing
Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/include/odp_chksum_internal.h
line 35
@@ -0,0 +1,59 @@
+/* Copyright (c) 2013, Linaro Limited
+ * All rights reserved.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+
+/**
+ * @file
+ *
+ * ODP checksum - implementation internal
+ */
+
+#ifndef ODP_CHKSUM_INTERNAL_H_
+#define ODP_CHKSUM_INTERNAL_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/* Simple implementation of ones complement sum.
+ * Based on RFC1071 and its errata.
+ */
+static uint32_t _odp_chksum_ones_comp16_32(const void *p, uint32_t len,
+ odp_bool_t odd_offset)
+
+{
+ uint32_t sum = 0;
+ const uint16_t *data = p;
+
+ if (odd_offset) {
+ uint16_t left_over = 0;
+
+ *(uint8_t *)_over = *(const uint8_t *)data;
+ sum = left_over;
+ data++;
Comment:
This erroneously increments `data` by 2 when you want to increment it by 1 here
so that it points to `p + 1`, not `p + 2`. Correct is `data = (const uint16_t
*)((uintptr_t)p + 1);`
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> The `odd_offset` parameter is unnecessary here. What matters is whether `p`
> starts on an even or odd byte boundary since that determines whether you can
> fetch `data` as `uint16_t` quantities without making unaligned storage
> references. Each call to `_odp_chksum_ones_comp16_32()` is independent in
> this regard, so it must always be determined anew for each call.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> `if (len > 1 && (uintptr_t)p % 2) ...`
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Drop use of `odd_offset` as noted earlier.
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
Checkpatch flag:
```
WARNING: line over 80 characters
#102: FILE: platform/linux-generic/odp_packet.c:2204:
+ ip_proto = parse_ipv4(prs, , , frame_len,
chksums);
total: 0 errors, 1 warnings, 0 checks, 163 lines checked
```
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> 2018
https://github.com/Linaro/odp/pull/389#discussion_r161935049
updated_at 2018-01-17 01:41:46
Re: [lng-odp] [PATCH v3][WIP] Validate checksums during packet parsing
Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/include/odp_chksum_internal.h
line 24
@@ -0,0 +1,59 @@
+/* Copyright (c) 2013, Linaro Limited
+ * All rights reserved.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+
+/**
+ * @file
+ *
+ * ODP checksum - implementation internal
+ */
+
+#ifndef ODP_CHKSUM_INTERNAL_H_
+#define ODP_CHKSUM_INTERNAL_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/* Simple implementation of ones complement sum.
+ * Based on RFC1071 and its errata.
+ */
+static uint32_t _odp_chksum_ones_comp16_32(const void *p, uint32_t len,
+ odp_bool_t odd_offset)
Comment:
The `odd_offset` parameter is unnecessary here. What matters is whether `p`
starts on an even or odd byte boundary since that determines whether you can
fetch `data` as `uint16_t` quantities without making unaligned storage
references. Each call to `_odp_chksum_ones_comp16_32()` is independent in this
regard, so it must always be determined anew for each call.
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> `if (len > 1 && (uintptr_t)p % 2) ...`
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Drop use of `odd_offset` as noted earlier.
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> Checkpatch flag:
>>> ```
>>> WARNING: line over 80 characters
>>> #102: FILE: platform/linux-generic/odp_packet.c:2204:
>>> + ip_proto = parse_ipv4(prs, , , frame_len,
>>> chksums);
>>> total: 0 errors, 1 warnings, 0 checks, 163 lines checked
>>> ```
Bill Fischofer(Bill-Fischofer-Linaro) wrote:
2018
https://github.com/Linaro/odp/pull/389#discussion_r161934743
updated_at 2018-01-17 01:41:46
Re: [lng-odp] [PATCH v3][WIP] Validate checksums during packet parsing
Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/include/odp_chksum_internal.h
line 30
@@ -0,0 +1,59 @@
+/* Copyright (c) 2013, Linaro Limited
+ * All rights reserved.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+
+/**
+ * @file
+ *
+ * ODP checksum - implementation internal
+ */
+
+#ifndef ODP_CHKSUM_INTERNAL_H_
+#define ODP_CHKSUM_INTERNAL_H_
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/* Simple implementation of ones complement sum.
+ * Based on RFC1071 and its errata.
+ */
+static uint32_t _odp_chksum_ones_comp16_32(const void *p, uint32_t len,
+ odp_bool_t odd_offset)
+
+{
+ uint32_t sum = 0;
+ const uint16_t *data = p;
+
+ if (odd_offset) {
Comment:
`if (len > 1 && (uintptr_t)p % 2) ...`
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> Drop use of `odd_offset` as noted earlier.
>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>> Checkpatch flag:
>> ```
>> WARNING: line over 80 characters
>> #102: FILE: platform/linux-generic/odp_packet.c:2204:
>> +ip_proto = parse_ipv4(prs, , , frame_len,
>> chksums);
>> total: 0 errors, 1 warnings, 0 checks, 163 lines checked
>> ```
>>> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
>>> 2018
https://github.com/Linaro/odp/pull/389#discussion_r161933918
updated_at 2018-01-17 01:41:46
Re: [lng-odp] [PATCH v3][WIP] Validate checksums during packet parsing
Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page:
platform/linux-generic/odp_packet.c
line 182
@@ -2191,7 +2201,7 @@ int packet_parse_common_l3_l4(packet_parser_t *prs, const
uint8_t *parseptr,
switch (ethtype) {
case _ODP_ETHTYPE_IPV4:
prs->input_flags.ipv4 = 1;
- ip_proto = parse_ipv4(prs, , , frame_len);
+ ip_proto = parse_ipv4(prs, , , frame_len,
chksums);
Comment:
Checkpatch flag:
```
WARNING: line over 80 characters
#102: FILE: platform/linux-generic/odp_packet.c:2204:
+ ip_proto = parse_ipv4(prs, , , frame_len,
chksums);
total: 0 errors, 1 warnings, 0 checks, 163 lines checked
```
> Bill Fischofer(Bill-Fischofer-Linaro) wrote:
> 2018
https://github.com/Linaro/odp/pull/389#discussion_r161403118
updated_at 2018-01-14 16:39:22
Re: [lng-odp] [PATCH v3][WIP] Validate checksums during packet parsing
Bill Fischofer(Bill-Fischofer-Linaro) replied on github web page: platform/linux-generic/include/odp_chksum_internal.h line 1 @@ -0,0 +1,59 @@ +/* Copyright (c) 2013, Linaro Limited Comment: 2018 https://github.com/Linaro/odp/pull/389#discussion_r161402959 updated_at 2018-01-14 16:39:21
