[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 1/3] net: improve UDP/TCP checksum computatio
From: |
Peter Maydell |
Subject: |
Re: [Qemu-devel] [PATCH v2 1/3] net: improve UDP/TCP checksum computation. |
Date: |
Thu, 5 May 2016 15:17:53 +0100 |
On 23 April 2016 at 11:58, Jean-Christophe Dubois <address@hidden> wrote:
> This patch adds:
> * based on Eth, UDP, TCP struct present in eth.h instead of hardcoded
> indexes.
> * based on various macros present in eth.h.
> * allow to account for optional VLAN header.
This is doing several things:
(1) changing style to use structs and macros rather than raw array accesses
(which shouldn't affect functionality)
(2) adding new functionality
I think they would be better in separate patches.
This function is used by multiple network devices, including the
important ones xen_nic and virtio-net -- is it definitely OK to
have it change behaviour for those devices?
> Signed-off-by: Jean-Christophe Dubois <address@hidden>
> ---
> net/checksum.c | 83
> ++++++++++++++++++++++++++++++++++++++++------------------
> 1 file changed, 57 insertions(+), 26 deletions(-)
>
> diff --git a/net/checksum.c b/net/checksum.c
> index d0fa424..fd25209 100644
> --- a/net/checksum.c
> +++ b/net/checksum.c
> @@ -18,9 +18,7 @@
> #include "qemu/osdep.h"
> #include "qemu-common.h"
> #include "net/checksum.h"
> -
> -#define PROTO_TCP 6
> -#define PROTO_UDP 17
> +#include "net/eth.h"
>
> uint32_t net_checksum_add_cont(int len, uint8_t *buf, int seq)
> {
> @@ -57,40 +55,73 @@ uint16_t net_checksum_tcpudp(uint16_t length, uint16_t
> proto,
>
> void net_checksum_calculate(uint8_t *data, int length)
> {
> - int hlen, plen, proto, csum_offset;
> - uint16_t csum;
> + int plen;
> + struct ip_header *ip;
> +
> + /* Ensure we have at least a Eth header */
> + if (length < sizeof(struct eth_header)) {
> + return;
> + }
>
> - /* Ensure data has complete L2 & L3 headers. */
> - if (length < 14 + 20) {
> + /* Now check we have an IP header (with an optonnal VLAN header */
"optional", also missing ")".
> + if (length < eth_get_l2_hdr_length(data) + sizeof(struct ip_header)) {
> return;
> }
>
> - if ((data[14] & 0xf0) != 0x40)
> + ip = PKT_GET_IP_HDR(data);
Are we definitely guaranteed that the input data buffer is aligned?
It seems unlikely, and if it isn't then a lot of this code (both
in macros like PKT_GET_IP_HDR, and open coded stuff below) is going
to go wrong if it tries to just access 16 bit struct fields and they're
not aligned and we're on a host that requires strict alignment.
> +
> + if (IP_HEADER_VERSION(ip) != IP_HEADER_VERSION_4) {
> return; /* not IPv4 */
> - hlen = (data[14] & 0x0f) * 4;
> - plen = (data[16] << 8 | data[17]) - hlen;
> - proto = data[23];
> + }
> +
> + /* Last, check that we have enough data for the IP frame */
> + if (length < eth_get_l2_hdr_length(data) + be16_to_cpu(ip->ip_len)) {
> + return;
> + }
> +
> + plen = be16_to_cpu(ip->ip_len) - IP_HDR_GET_LEN(ip);
> +
> + switch (ip->ip_p) {
> + case IP_PROTO_TCP:
> + {
> + uint16_t csum;
> + tcp_header *tcp = (tcp_header *)(ip + 1);
> +
> + if (plen < sizeof(tcp_header)) {
> + return;
> + }
I think this indent style is indenting to much. switch statements
usually look like:
switch (whatever) {
case FOO:
{
code here;
}
case BAR:
{
more code;
}
default:
whatever;
}
>
> - switch (proto) {
> - case PROTO_TCP:
> - csum_offset = 16;
> + tcp->th_sum = 0;
> +
> + csum = net_checksum_tcpudp(plen, ip->ip_p,
> + (uint8_t *)&ip->ip_src,
> + (uint8_t *)tcp);
> +
> + tcp->th_sum = cpu_to_be16(csum);
> + }
> break;
> - case PROTO_UDP:
> - csum_offset = 6;
> + case IP_PROTO_UDP:
> + {
> + uint16_t csum;
> + udp_header *udp = (udp_header *)(ip + 1);
> +
> + if (plen < sizeof(udp_header)) {
> + return;
> + }
> +
> + udp->uh_sum = 0;
> +
> + csum = net_checksum_tcpudp(plen, ip->ip_p,
> + (uint8_t *)&ip->ip_src,
> + (uint8_t *)udp);
> +
> + udp->uh_sum = cpu_to_be16(csum);
> + }
> break;
> default:
> + /* Can't handle any other protocol */
> return;
> }
> -
> - if (plen < csum_offset + 2 || 14 + hlen + plen > length) {
> - return;
> - }
> -
> - data[14+hlen+csum_offset] = 0;
> - data[14+hlen+csum_offset+1] = 0;
> - csum = net_checksum_tcpudp(plen, proto, data+14+12, data+14+hlen);
> - data[14+hlen+csum_offset] = csum >> 8;
> - data[14+hlen+csum_offset+1] = csum & 0xff;
> }
>
> uint32_t
> --
> 2.7.4
thanks
-- PMM
- Re: [Qemu-devel] [PATCH v2 1/3] net: improve UDP/TCP checksum computation.,
Peter Maydell <=