[PATCH v5] helper: ip: correct ipv4 header checksum calculation#132
[PATCH v5] helper: ip: correct ipv4 header checksum calculation#132lumag wants to merge 3 commits intoOpenDataPlane:masterfrom
Conversation
muvarov
changed the title
|
This is a fixup for https://bugs.linaro.org/show_bug.cgi?id=2976 |
helper/include/odp/helper/ip.h
Outdated
| memcpy(buf, ip, sizeof(*ip)); | ||
| res = odp_packet_copy_to_mem(pkt, offset + sizeof(*ip), | ||
| nleft - sizeof(*ip), | ||
| buf + sizeof(*ip)/2); |
There was a problem hiding this comment.
Checkpatch flags this. It's a legit issue that should be fixed.
There was a problem hiding this comment.
@Bill-Fischofer-Linaro missed that. Should be fixed now.
muvarov
changed the title
| nleft - sizeof(*ip), | ||
| buf + sizeof(*ip) / 2); | ||
| if (odp_unlikely(res < 0)) | ||
| return res; |
There was a problem hiding this comment.
type function mismatch. odp_u16sum_t is uint16_t. According to use cases for usage of that function, this function has to be int, not odp_u16sum_t.
helper/include/odp/helper/ip.h
Outdated
| * @return IPv4 checksum in host cpu order, or 0 on failure | ||
| */ | ||
| static inline odp_u16sum_t odph_ipv4_csum_update(odp_packet_t pkt) | ||
| static inline void odph_ipv4_csum_update(odp_packet_t pkt) |
There was a problem hiding this comment.
function can return errors, why it's void? Or it has to assert() on each error, or return valid error which is checked on upper layer.
There was a problem hiding this comment.
Good catch. The @return is also not correct if this becomes a void function.
Bill-Fischofer-Linaro
dismissed
their stale review
@muvarov found some things my review overlooked.
|
@muvarov @Bill-Fischofer-Linaro updated PR to include proposed fixes plus few more changes. |
helper/include/odp/helper/chksum.h
Outdated
|
|
||
| for (sum = 0; len > 1; len -= 2) | ||
| sum += *buf++; | ||
| sum += odp_be_to_cpu_16(*buf++); |
There was a problem hiding this comment.
I'm not sure why this is necessary. Isn't it more efficient to calculate the checksum directly and then return the result in network byte order? The value is going to be the same, it's just how it's being returned.
There was a problem hiding this comment.
Hmm, true. I updated this commit to just change documentation.
| if (odp_unlikely(res < 0)) | ||
| return res; | ||
|
|
||
| res = odph_ipv4_csum(pkt, offset, &ip, &chksum); |
There was a problem hiding this comment.
Since you're deleting setting the chksum to 0 elsewhere before calling odph_ipv4_csum_update(), don't you need to set it to zero here before calling odph_ipv4_csum()? This field is supposed to be initialized to zero to calculate the checksum.
There was a problem hiding this comment.
No. odph_ipv4_csum() initialises checksum to 0 in the header copy. This simplifies code for header verification, etc.
There was a problem hiding this comment.
OK, I missed that on first read.
All examples and usecases assumed network byte order for odph_chksum() return value. Instead of changing this convention, rather document that odph_chksum returns value in network byte order. Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>
Current code for IPv4 header checksum calculation assumes that packet data is aligned on 2-byte boundary, that there are no optional headers, etc. Rewrite checksumming code to properly copy & process headers. Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>
Signed-off-by: Dmitry Eremin-Solenikov <dmitry.ereminsolenikov@linaro.org>
| if (odp_unlikely(res < 0)) | ||
| return res; | ||
|
|
||
| res = odph_ipv4_csum(pkt, offset, &ip, &chksum); |
There was a problem hiding this comment.
OK, I missed that on first read.
|
Merged. |
3 participants
Current code for IPv4 header checksum calculation assumes that packet
data is aligned on 2-byte boundary, that there are no optional headers,
etc. Rewrite checksumming code to properly copy & process headers.
Signed-off-by: Dmitry Eremin-Solenikov dmitry.ereminsolenikov@linaro.org