Re: [dpdk-dev] [PATCH v2 4/4] vhost: fix offload flags in Rx path

[Flavio Leitner <fbl@sysclose.org>]

On Thu, Apr 29, 2021 at 10:04:38AM +0200, David Marchand wrote:
> The vhost library current configures Tx offloading (PKT_TX_*) on any
> packet received from a guest virtio device which asks for some offloading.
> 
> This is problematic, as Tx offloading is something that the application
> must ask for: the application needs to configure devices
> to support every used offloads (ip, tcp checksumming, tso..), and the
> various l2/l3/l4 lengths must be set following any processing that
> happened in the application itself.
> 
> On the other hand, the received packets are not marked wrt current
> packet l3/l4 checksumming info.
> 
> Copy virtio rx processing to fix those offload flags but accepting
> VIRTIO_NET_HDR_GSO_ECN and VIRTIO_NET_HDR_GSO_UDP too.
> 
> The vhost example has been updated accordingly: TSO is applied to any
> packet marked LRO.
> 
> Fixes: 859b480d5afd ("vhost: add guest offload setting")
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> Changes since v1:
> - updated vhost example,
> - restored VIRTIO_NET_HDR_GSO_ECN and VIRTIO_NET_HDR_GSO_UDP support,
> - restored log on buggy offload request,
> 
> ---
>  examples/vhost/main.c  |  42 +++++++------
>  lib/vhost/virtio_net.c | 139 +++++++++++++++++------------------------
>  2 files changed, 78 insertions(+), 103 deletions(-)
> 
[...]

> -	if (l4_hdr && hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE) {
> +	/* GSO request, save required information in mbuf */
> +	if (hdr->gso_type != VIRTIO_NET_HDR_GSO_NONE) {
> +		/* Check unsupported modes */
> +		if (hdr->gso_size == 0)
> +			return -EINVAL;
> +
>  		switch (hdr->gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
>  		case VIRTIO_NET_HDR_GSO_TCPV4:
>  		case VIRTIO_NET_HDR_GSO_TCPV6:
> -			tcp_hdr = l4_hdr;
> -			m->ol_flags |= PKT_TX_TCP_SEG;
> -			m->tso_segsz = hdr->gso_size;
> -			m->l4_len = (tcp_hdr->data_off & 0xf0) >> 2;
> -			break;
>  		case VIRTIO_NET_HDR_GSO_UDP:
> -			m->ol_flags |= PKT_TX_UDP_SEG;
> +			m->ol_flags |= PKT_RX_LRO | PKT_RX_L4_CKSUM_NONE;

My understanding of the virtio 1.1 spec is that GSO can be
used independently of CSUM. There is nothing preventing to
send a fully checksummed TSO packet.

Anyways, that's unusual and not the goal of this patch.

Acked-by: Flavio Leitner <fbl@sysclose.org>

fbl


> +			/* Update mss lengths in mbuf */
>  			m->tso_segsz = hdr->gso_size;
> -			m->l4_len = sizeof(struct rte_udp_hdr);
>  			break;
>  		default:
>  			VHOST_LOG_DATA(WARNING,
>  				"unsupported gso type %u.\n", hdr->gso_type);
> -			break;
> +			return -EINVAL;
>  		}
>  	}
> +
> +	return 0;
>  }
>  
>  static __rte_noinline void
> @@ -2084,8 +2054,11 @@ copy_desc_to_mbuf(struct virtio_net *dev, struct vhost_virtqueue *vq,
>  	prev->data_len = mbuf_offset;
>  	m->pkt_len    += mbuf_offset;
>  
> -	if (hdr)
> -		vhost_dequeue_offload(hdr, m);
> +	if (hdr && vhost_dequeue_offload(hdr, m) < 0) {
> +		VHOST_LOG_DATA(ERR, "Packet with invalid offloads.\n");
> +		error = -1;
> +		goto out;
> +	}
>  
>  out:
>  
> -- 
> 2.23.0
> 

-- 
fbl