Re: [dpdk-stable] [dpdk-dev] [PATCH] net/virtio: fix mbuf data and pkt length mismatch

[Andrew Rybchenko <arybchenko@solarflare.com>]

Hi,

as far as I can see the patch introduces regressions.

CC Kevin and Luca to be careful with stable branches patches.

See details below.

Andrew.

On 9/23/19 5:05 PM, Marvin Liu wrote:
> If reserve virtio header room by function rte_pktmbuf_prepend, both
> segment data length and packet length of mbuf will be increased.
> Data length will be equal to descriptor length, while packet length
> should be decreased as virtio-net header won't be taken into packet.
> Thus will cause mismatch in mbuf structure. Fix this issue by access
> mbuf data directly and increase descriptor length if it is needed.
>
> Fixes: 58169a9c8153 ("net/virtio: support Tx checksum offload")
> Fixes: 892dc798fa9c ("net/virtio: implement Tx path for packed queues")
> Fixes: 4905ed3a523f ("net/virtio: optimize Tx enqueue for packed ring")
> Fixes: e5f456a98d3c ("net/virtio: support in-order Rx and Tx")
> Cc: stable@dpdk.org
>
> Reported-by: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Marvin Liu <yong.liu@intel.com>
>
> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
> index 27ead19fb..822cce06d 100644
> --- a/drivers/net/virtio/virtio_rxtx.c
> +++ b/drivers/net/virtio/virtio_rxtx.c
> @@ -597,9 +597,8 @@ virtqueue_enqueue_xmit_inorder(struct virtnet_tx *txvq,
>   		dxp->cookie = (void *)cookies[i];
>   		dxp->ndescs = 1;
>   
> -		hdr = (struct virtio_net_hdr *)
> -			rte_pktmbuf_prepend(cookies[i], head_size);
> -		cookies[i]->pkt_len -= head_size;
> +		hdr = (struct virtio_net_hdr *)(char *)cookies[i]->buf_addr +
> +			cookies[i]->data_off - head_size;
>   
>   		/* if offload disabled, hdr is not zeroed yet, do it now */
>   		if (!vq->hw->has_tx_offload)
> @@ -608,9 +607,10 @@ virtqueue_enqueue_xmit_inorder(struct virtnet_tx *txvq,
>   			virtqueue_xmit_offload(hdr, cookies[i], true);
>   
>   		start_dp[idx].addr  = VIRTIO_MBUF_DATA_DMA_ADDR(cookies[i], vq);

As I understand the problem is here. It points to start of the packet
(Ethernet header) since data_off is not changed above now, but
should point to virtio_net_hdr before the packet.

I think the patch fixes the bug in a wrong direction. It looks better
to simply remove

     cookies[i]->pkt_len -= head_size;

above and care about real packet length difference in
virtio_update_packet_stats() or when it is called from Tx path.

If it is OK for maintainers I'm ready to send patches to rollback back
this one and fix it as described above.

> -		start_dp[idx].len   = cookies[i]->data_len;
> +		start_dp[idx].len   = cookies[i]->data_len + head_size;
>   		start_dp[idx].flags = 0;
>   
> +
>   		vq_update_avail_ring(vq, idx);
>   
>   		idx++;
> @@ -644,9 +644,8 @@ virtqueue_enqueue_xmit_packed_fast(struct virtnet_tx *txvq,
>   	flags = vq->vq_packed.cached_flags;
>   
>   	/* prepend cannot fail, checked by caller */
> -	hdr = (struct virtio_net_hdr *)
> -		rte_pktmbuf_prepend(cookie, head_size);
> -	cookie->pkt_len -= head_size;
> +	hdr = (struct virtio_net_hdr *)(char *)cookie->buf_addr +
> +		cookie->data_off - head_size;
>   
>   	/* if offload disabled, hdr is not zeroed yet, do it now */
>   	if (!vq->hw->has_tx_offload)
> @@ -655,7 +654,7 @@ virtqueue_enqueue_xmit_packed_fast(struct virtnet_tx *txvq,
>   		virtqueue_xmit_offload(hdr, cookie, true);
>   
>   	dp->addr = VIRTIO_MBUF_DATA_DMA_ADDR(cookie, vq);
> -	dp->len  = cookie->data_len;
> +	dp->len  = cookie->data_len + head_size;
>   	dp->id   = id;
>   
>   	if (++vq->vq_avail_idx >= vq->vq_nentries) {
> @@ -687,6 +686,7 @@ virtqueue_enqueue_xmit_packed(struct virtnet_tx *txvq, struct rte_mbuf *cookie,
>   	uint16_t head_size = vq->hw->vtnet_hdr_size;
>   	struct virtio_net_hdr *hdr;
>   	uint16_t prev;
> +	bool prepend_header = false;
>   
>   	id = in_order ? vq->vq_avail_idx : vq->vq_desc_head_idx;
>   
> @@ -705,12 +705,9 @@ virtqueue_enqueue_xmit_packed(struct virtnet_tx *txvq, struct rte_mbuf *cookie,
>   
>   	if (can_push) {
>   		/* prepend cannot fail, checked by caller */
> -		hdr = (struct virtio_net_hdr *)
> -			rte_pktmbuf_prepend(cookie, head_size);
> -		/* rte_pktmbuf_prepend() counts the hdr size to the pkt length,
> -		 * which is wrong. Below subtract restores correct pkt size.
> -		 */
> -		cookie->pkt_len -= head_size;
> +		hdr = (struct virtio_net_hdr *)(char *)cookie->buf_addr +
> +			cookie->data_off - head_size;
> +		prepend_header = true;
>   
>   		/* if offload disabled, it is not zeroed below, do it now */
>   		if (!vq->hw->has_tx_offload)
> @@ -738,6 +735,11 @@ virtqueue_enqueue_xmit_packed(struct virtnet_tx *txvq, struct rte_mbuf *cookie,
>   
>   		start_dp[idx].addr = VIRTIO_MBUF_DATA_DMA_ADDR(cookie, vq);
>   		start_dp[idx].len  = cookie->data_len;
> +		if (prepend_header) {
> +			start_dp[idx].len += head_size;
> +			prepend_header = false;
> +		}
> +
>   		if (likely(idx != head_idx)) {
>   			flags = cookie->next ? VRING_DESC_F_NEXT : 0;
>   			flags |= vq->vq_packed.cached_flags;
> @@ -779,6 +781,7 @@ virtqueue_enqueue_xmit(struct virtnet_tx *txvq, struct rte_mbuf *cookie,
>   	uint16_t seg_num = cookie->nb_segs;
>   	uint16_t head_idx, idx;
>   	uint16_t head_size = vq->hw->vtnet_hdr_size;
> +	bool prepend_header = false;
>   	struct virtio_net_hdr *hdr;
>   
>   	head_idx = vq->vq_desc_head_idx;
> @@ -794,12 +797,9 @@ virtqueue_enqueue_xmit(struct virtnet_tx *txvq, struct rte_mbuf *cookie,
>   
>   	if (can_push) {
>   		/* prepend cannot fail, checked by caller */
> -		hdr = (struct virtio_net_hdr *)
> -			rte_pktmbuf_prepend(cookie, head_size);
> -		/* rte_pktmbuf_prepend() counts the hdr size to the pkt length,
> -		 * which is wrong. Below subtract restores correct pkt size.
> -		 */
> -		cookie->pkt_len -= head_size;
> +		hdr = (struct virtio_net_hdr *)(char *)cookie->buf_addr +
> +			cookie->data_off - head_size;
> +		prepend_header = true;
>   
>   		/* if offload disabled, it is not zeroed below, do it now */
>   		if (!vq->hw->has_tx_offload)
> @@ -838,6 +838,10 @@ virtqueue_enqueue_xmit(struct virtnet_tx *txvq, struct rte_mbuf *cookie,
>   	do {
>   		start_dp[idx].addr  = VIRTIO_MBUF_DATA_DMA_ADDR(cookie, vq);
>   		start_dp[idx].len   = cookie->data_len;
> +		if (prepend_header) {
> +			start_dp[idx].len += head_size;
> +			prepend_header = false;
> +		}
>   		start_dp[idx].flags = cookie->next ? VRING_DESC_F_NEXT : 0;
>   		idx = start_dp[idx].next;
>   	} while ((cookie = cookie->next) != NULL);