[dpdk-stable] [dpdk-dev] [PATCH] net/virtio: fix mbuf data and pkt length mismatch
Andrew Rybchenko
arybchenko at solarflare.com
Mon Oct 14 17:15:42 CEST 2019
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 at dpdk.org
>
> Reported-by: Stephen Hemminger <stephen at networkplumber.org>
> Signed-off-by: Marvin Liu <yong.liu at 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);
More information about the stable
mailing list