| Message ID | 1510801633-115970-1-git-send-email-eswierk@skyportsystems.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 11/15/2017 09:07 PM, Ed Swierk via Qemu-devel wrote: [meta-comment] > The checksum algorithm used by IPv4, TCP and UDP allows a zero value > to be represented by either 0x0000 and 0xFFFF. But per RFC 768, a zero > UDP checksum must be transmitted as 0xFFFF, as 0x0000 is a special > value meaning no checksum. > > Substitute 0xFFFF whenever a checksum is computed as zero when > modifying a UDP datagram header. Doing this on IPv4 packets and TCP > segments is unnecessary but legal. Add a wrapper for > net_checksum_finish() that makes the substitution. > > (We can't just change net_checksum_finish(), as that function is also > used by receivers to verify checksums, and in that case the expected > value is always 0x0000.) This part below... > > v3: > > Leave net_tx_pkt_update_ip_checksums() alone since it's only computing > a partial sum of the IP pseudo-header. > > Rename wrapper to net_checksum_finish_nozero() for clarity. > > v2: > > Add a wrapper net_checksum_finish_hdr() rather than duplicating the > logic at every caller. ...through here, belongs... > > Signed-off-by: Ed Swierk <eswierk@skyportsystems.com> > --- ...here, after the --- separator (you can have more than one ---; 'git am' stops parsing at the first). It is data that is useful to reviewers, but will be meaningless a year from now when browsing git history (at which point we won't care how many revisions the patch went through on the list, but only what was checked in).
On Wed, Nov 15, 2017 at 7:55 PM, Eric Blake <eblake@redhat.com> wrote: > > On 11/15/2017 09:07 PM, Ed Swierk via Qemu-devel wrote: > > This part below... > > > > > v3: > > > > Leave net_tx_pkt_update_ip_checksums() alone since it's only computing > > a partial sum of the IP pseudo-header. > > > > Rename wrapper to net_checksum_finish_nozero() for clarity. > > > > v2: > > > > Add a wrapper net_checksum_finish_hdr() rather than duplicating the > > logic at every caller. > > ...through here, belongs... > > > > > Signed-off-by: Ed Swierk <eswierk@skyportsystems.com> > > --- > > ...here, after the --- separator (you can have more than one ---; 'git > am' stops parsing at the first). It is data that is useful to > reviewers, but will be meaningless a year from now when browsing git > history (at which point we won't care how many revisions the patch went > through on the list, but only what was checked in). Thanks for the clue. I'll repost a corrected version. Maybe checkpatch.pl should squawk if it sees ^v[0-9]\+: before the first ---? --Ed
diff --git a/hw/net/e1000.c b/hw/net/e1000.c index 72a92be..804ec08 100644 --- a/hw/net/e1000.c +++ b/hw/net/e1000.c @@ -506,7 +506,7 @@ putsum(uint8_t *data, uint32_t n, uint32_t sloc, uint32_t css, uint32_t cse) n = cse + 1; if (sloc < n-1) { sum = net_checksum_add(n-css, data+css); - stw_be_p(data + sloc, net_checksum_finish(sum)); + stw_be_p(data + sloc, net_checksum_finish_nozero(sum)); } } diff --git a/hw/net/net_rx_pkt.c b/hw/net/net_rx_pkt.c index cef1c2e..98a5030 100644 --- a/hw/net/net_rx_pkt.c +++ b/hw/net/net_rx_pkt.c @@ -518,7 +518,7 @@ _net_rx_pkt_calc_l4_csum(struct NetRxPkt *pkt) cntr += net_checksum_add_iov(pkt->vec, pkt->vec_len, pkt->l4hdr_off, csl, cso); - csum = net_checksum_finish(cntr); + csum = net_checksum_finish_nozero(cntr); trace_net_rx_pkt_l4_csum_calc_csum(pkt->l4hdr_off, csl, cntr, csum); diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c index 20b2549..e29c881 100644 --- a/hw/net/net_tx_pkt.c +++ b/hw/net/net_tx_pkt.c @@ -486,7 +486,7 @@ static void net_tx_pkt_do_sw_csum(struct NetTxPkt *pkt) net_checksum_add_iov(iov, iov_len, pkt->virt_hdr.csum_start, csl, cso); /* Put the checksum obtained into the packet */ - csum = cpu_to_be16(net_checksum_finish(csum_cntr)); + csum = cpu_to_be16(net_checksum_finish_nozero(csum_cntr)); iov_from_buf(iov, iov_len, csum_offset, &csum, sizeof csum); } diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c index 8c4bae5..cdc307d 100644 --- a/hw/net/vmxnet3.c +++ b/hw/net/vmxnet3.c @@ -972,7 +972,8 @@ static void vmxnet3_rx_need_csum_calculate(struct NetRxPkt *pkt, data = (uint8_t *)pkt_data + vhdr->csum_start; len = pkt_len - vhdr->csum_start; /* Put the checksum obtained into the packet */ - stw_be_p(data + vhdr->csum_offset, net_raw_checksum(data, len)); + stw_be_p(data + vhdr->csum_offset, + net_checksum_finish_nozero(net_checksum_add(len, data))); vhdr->flags &= ~VIRTIO_NET_HDR_F_NEEDS_CSUM; vhdr->flags |= VIRTIO_NET_HDR_F_DATA_VALID; diff --git a/include/net/checksum.h b/include/net/checksum.h index 7df472c..05a0d27 100644 --- a/include/net/checksum.h +++ b/include/net/checksum.h @@ -34,6 +34,12 @@ net_checksum_add(int len, uint8_t *buf) } static inline uint16_t +net_checksum_finish_nozero(uint32_t sum) +{ + return net_checksum_finish(sum) ?: 0xFFFF; +} + +static inline uint16_t net_raw_checksum(uint8_t *data, int length) { return net_checksum_finish(net_checksum_add(length, data));
The checksum algorithm used by IPv4, TCP and UDP allows a zero value to be represented by either 0x0000 and 0xFFFF. But per RFC 768, a zero UDP checksum must be transmitted as 0xFFFF, as 0x0000 is a special value meaning no checksum. Substitute 0xFFFF whenever a checksum is computed as zero when modifying a UDP datagram header. Doing this on IPv4 packets and TCP segments is unnecessary but legal. Add a wrapper for net_checksum_finish() that makes the substitution. (We can't just change net_checksum_finish(), as that function is also used by receivers to verify checksums, and in that case the expected value is always 0x0000.) v3: Leave net_tx_pkt_update_ip_checksums() alone since it's only computing a partial sum of the IP pseudo-header. Rename wrapper to net_checksum_finish_nozero() for clarity. v2: Add a wrapper net_checksum_finish_hdr() rather than duplicating the logic at every caller. Signed-off-by: Ed Swierk <eswierk@skyportsystems.com> --- hw/net/e1000.c | 2 +- hw/net/net_rx_pkt.c | 2 +- hw/net/net_tx_pkt.c | 2 +- hw/net/vmxnet3.c | 3 ++- include/net/checksum.h | 6 ++++++ 5 files changed, 11 insertions(+), 4 deletions(-)