[dpdk-dev] [PATCH] virtio: fix packet corruption

[dpdk-dev] [PATCH] virtio: fix packet corruption

[Olivier Matz]

The support of virtio-user changed the way the mbuf dma address is
retrieved, using a physical address in case of virtio-pci and a virtual
address in case of virtio-user.

This change introduced some possible memory corruption in packets,
replacing:
  m->buf_physaddr + RTE_PKTMBUF_HEADROOM
by:
  m->buf_physaddr + m->data_off     (through a macro)

This patch fixes this issue, restoring the original behavior.

By the way, it also rework the macros, adding a "VIRTIO_" prefix and
API comments.

Fixes: f24f8f9fee8a ("net/virtio: allow virtual address to fill vring descriptors")

Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
---
 drivers/net/virtio/virtio_ethdev.c      |  2 +-
 drivers/net/virtio/virtio_rxtx.c        |  5 +++--
 drivers/net/virtio/virtio_rxtx_simple.c | 13 +++++++------
 drivers/net/virtio/virtqueue.h          | 25 +++++++++++++++++--------
 4 files changed, 28 insertions(+), 17 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 850e3ba..fcc996e 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -454,7 +454,7 @@ int virtio_dev_queue_setup(struct rte_eth_dev *dev,
 
 	/* For virtio-user case (that is when dev->pci_dev is NULL), we use
 	 * virtual address. And we need properly set _offset_, please see
-	 * MBUF_DATA_DMA_ADDR in virtqueue.h for more information.
+	 * VIRTIO_MBUF_DATA_DMA_ADDR in virtqueue.h for more information.
 	 */
 	if (dev->pci_dev)
 		vq->offset = offsetof(struct rte_mbuf, buf_physaddr);
diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c
index a27208e..9aba044 100644
--- a/drivers/net/virtio/virtio_rxtx.c
+++ b/drivers/net/virtio/virtio_rxtx.c
@@ -193,7 +193,8 @@ virtqueue_enqueue_recv_refill(struct virtqueue *vq, struct rte_mbuf *cookie)
 
 	start_dp = vq->vq_ring.desc;
 	start_dp[idx].addr =
-		MBUF_DATA_DMA_ADDR(cookie, vq->offset) - hw->vtnet_hdr_size;
+		VIRTIO_MBUF_ADDR(cookie, vq) +
+		RTE_PKTMBUF_HEADROOM - hw->vtnet_hdr_size;
 	start_dp[idx].len =
 		cookie->buf_len - RTE_PKTMBUF_HEADROOM + hw->vtnet_hdr_size;
 	start_dp[idx].flags =  VRING_DESC_F_WRITE;
@@ -265,7 +266,7 @@ virtqueue_enqueue_xmit(struct virtnet_tx *txvq, struct rte_mbuf *cookie,
 	}
 
 	do {
-		start_dp[idx].addr  = MBUF_DATA_DMA_ADDR(cookie, vq->offset);
+		start_dp[idx].addr  = VIRTIO_MBUF_DATA_DMA_ADDR(cookie, vq);
 		start_dp[idx].len   = cookie->data_len;
 		start_dp[idx].flags = cookie->next ? VRING_DESC_F_NEXT : 0;
 		idx = start_dp[idx].next;
diff --git a/drivers/net/virtio/virtio_rxtx_simple.c b/drivers/net/virtio/virtio_rxtx_simple.c
index d8fcc15..6517aa8 100644
--- a/drivers/net/virtio/virtio_rxtx_simple.c
+++ b/drivers/net/virtio/virtio_rxtx_simple.c
@@ -80,8 +80,9 @@ virtqueue_enqueue_recv_refill_simple(struct virtqueue *vq,
 	vq->sw_ring[desc_idx] = cookie;
 
 	start_dp = vq->vq_ring.desc;
-	start_dp[desc_idx].addr = MBUF_DATA_DMA_ADDR(cookie, vq->offset) -
-				  vq->hw->vtnet_hdr_size;
+	start_dp[desc_idx].addr =
+		VIRTIO_MBUF_ADDR(cookie, vq) +
+		RTE_PKTMBUF_HEADROOM - vq->hw->vtnet_hdr_size;
 	start_dp[desc_idx].len = cookie->buf_len -
 		RTE_PKTMBUF_HEADROOM + vq->hw->vtnet_hdr_size;
 
@@ -120,8 +121,8 @@ virtio_rxq_rearm_vec(struct virtnet_rx *rxvq)
 		*(uint64_t *)p = rxvq->mbuf_initializer;
 
 		start_dp[i].addr =
-			MBUF_DATA_DMA_ADDR(sw_ring[i], vq->offset) -
-			vq->hw->vtnet_hdr_size;
+			VIRTIO_MBUF_ADDR(sw_ring[i], vq) +
+			RTE_PKTMBUF_HEADROOM - vq->hw->vtnet_hdr_size;
 		start_dp[i].len = sw_ring[i]->buf_len -
 			RTE_PKTMBUF_HEADROOM + vq->hw->vtnet_hdr_size;
 	}
@@ -371,7 +372,7 @@ virtio_xmit_pkts_simple(void *tx_queue, struct rte_mbuf **tx_pkts,
 			vq->vq_descx[desc_idx + i].cookie = tx_pkts[i];
 		for (i = 0; i < nb_tail; i++) {
 			start_dp[desc_idx].addr =
-				MBUF_DATA_DMA_ADDR(*tx_pkts, vq->offset);
+				VIRTIO_MBUF_DATA_DMA_ADDR(*tx_pkts, vq);
 			start_dp[desc_idx].len = (*tx_pkts)->pkt_len;
 			tx_pkts++;
 			desc_idx++;
@@ -383,7 +384,7 @@ virtio_xmit_pkts_simple(void *tx_queue, struct rte_mbuf **tx_pkts,
 		vq->vq_descx[desc_idx + i].cookie = tx_pkts[i];
 	for (i = 0; i < nb_commit; i++) {
 		start_dp[desc_idx].addr =
-			MBUF_DATA_DMA_ADDR(*tx_pkts, vq->offset);
+			VIRTIO_MBUF_DATA_DMA_ADDR(*tx_pkts, vq);
 		start_dp[desc_idx].len = (*tx_pkts)->pkt_len;
 		tx_pkts++;
 		desc_idx++;
diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
index 455aaaf..c452d04 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -67,12 +67,21 @@ struct rte_mbuf;
 #define VIRTQUEUE_MAX_NAME_SZ 32
 
 #ifdef RTE_VIRTIO_USER
-#define MBUF_DATA_DMA_ADDR(mb, offset) \
-	((uint64_t)((uintptr_t)(*(void **)((uintptr_t)mb + offset)) \
-			+ (mb)->data_off))
-#else /* RTE_VIRTIO_USER */
-#define MBUF_DATA_DMA_ADDR(mb, offset) rte_mbuf_data_dma_addr(mb)
-#endif /* RTE_VIRTIO_USER */
+/**
+ * Return the physical address (or virtual address in case of
+ * virtio-user) of mbuf data buffer.
+ */
+#define VIRTIO_MBUF_ADDR(mb, vq) (*(uint64_t *)((uintptr_t)(mb) + (vq)->offset))
+#else
+#define VIRTIO_MBUF_ADDR(mb, vq) ((mb)->buf_physaddr)
+#endif
+
+/**
+ * Return the physical address (or virtual address in case of
+ * virtio-user) of mbuf data buffer, taking care of mbuf data offset
+ */
+#define VIRTIO_MBUF_DATA_DMA_ADDR(mb, vq) \
+	(VIRTIO_MBUF_ADDR(mb, vq) + (mb)->data_off)
 
 #define VTNET_SQ_RQ_QUEUE_IDX 0
 #define VTNET_SQ_TQ_QUEUE_IDX 1
@@ -182,8 +191,8 @@ struct virtqueue {
 	void *vq_ring_virt_mem;  /**< linear address of vring*/
 	unsigned int vq_ring_size;
 
-	phys_addr_t vq_ring_mem; /**< physical address of vring */
-				/**< use virtual address for virtio-user. */
+	phys_addr_t vq_ring_mem; /**< physical address of vring,
+				  * or virtual address for virtio-user. */
 
 	/**
 	 * Head of the free chain in the descriptor table. If
-- 
2.8.1

Re: [dpdk-dev] [PATCH] virtio: fix packet corruption

[Tan, Jianfeng]

Hi Oliver,

> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Tuesday, July 19, 2016 8:32 PM
> To: dev@dpdk.org; Tan, Jianfeng; Xie, Huawei; yuanhan.liu@linux.intel.com
> Subject: [PATCH] virtio: fix packet corruption
> 
> The support of virtio-user changed the way the mbuf dma address is
> retrieved, using a physical address in case of virtio-pci and a virtual
> address in case of virtio-user.
> 
> This change introduced some possible memory corruption in packets,
> replacing:
>   m->buf_physaddr + RTE_PKTMBUF_HEADROOM
> by:
>   m->buf_physaddr + m->data_off     (through a macro)
> 
> This patch fixes this issue, restoring the original behavior.

Could you be more specific on why we cannot use m->data_off here?

Thanks,
Jianfeng

> 
> By the way, it also rework the macros, adding a "VIRTIO_" prefix and
> API comments.
> 
> Fixes: f24f8f9fee8a ("net/virtio: allow virtual address to fill vring descriptors")
> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c      |  2 +-
>  drivers/net/virtio/virtio_rxtx.c        |  5 +++--
>  drivers/net/virtio/virtio_rxtx_simple.c | 13 +++++++------
>  drivers/net/virtio/virtqueue.h          | 25 +++++++++++++++++--------
>  4 files changed, 28 insertions(+), 17 deletions(-)
> 

Re: [dpdk-dev] [PATCH] virtio: fix packet corruption

[Olivier Matz]

Hi Jianfeng,

On 07/19/2016 03:03 PM, Tan, Jianfeng wrote:
> Hi Oliver,
> 
>> -----Original Message-----
>> From: Olivier Matz [mailto:olivier.matz@6wind.com]
>> Sent: Tuesday, July 19, 2016 8:32 PM
>> To: dev@dpdk.org; Tan, Jianfeng; Xie, Huawei; yuanhan.liu@linux.intel.com
>> Subject: [PATCH] virtio: fix packet corruption
>>
>> The support of virtio-user changed the way the mbuf dma address is
>> retrieved, using a physical address in case of virtio-pci and a virtual
>> address in case of virtio-user.
>>
>> This change introduced some possible memory corruption in packets,
>> replacing:
>>   m->buf_physaddr + RTE_PKTMBUF_HEADROOM
>> by:
>>   m->buf_physaddr + m->data_off     (through a macro)
>>
>> This patch fixes this issue, restoring the original behavior.
> 
> Could you be more specific on why we cannot use m->data_off here?

There is no guarantee that m->data_off == RTE_PKTMBUF_HEADROOM here as
virtqueue_enqueue_recv_refill() is called on a mbuf that is just
allocated with rte_mbuf_raw_alloc(). An alternative would be to set
data_off to RTE_PKTMBUF_HEADROOM, but as it's a fix and we are close to
the release, I prefered to restore the initial behavior.

I did not include the test plan because it relies on patch that are not
submitted yet (offload patches, they will be upstreamed very soon). It
is a quite simple test case with testpmd.

Regards,
Olivier

Re: [dpdk-dev] [PATCH] virtio: fix packet corruption

[Tan, Jianfeng]

> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Tuesday, July 19, 2016 9:11 PM
> To: Tan, Jianfeng; dev@dpdk.org; Xie, Huawei; yuanhan.liu@linux.intel.com
> Subject: Re: [PATCH] virtio: fix packet corruption
> 
> Hi Jianfeng,
> 
> On 07/19/2016 03:03 PM, Tan, Jianfeng wrote:
> > Hi Oliver,
> >
> >> -----Original Message-----
> >> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> >> Sent: Tuesday, July 19, 2016 8:32 PM
> >> To: dev@dpdk.org; Tan, Jianfeng; Xie, Huawei;
> yuanhan.liu@linux.intel.com
> >> Subject: [PATCH] virtio: fix packet corruption
> >>
> >> The support of virtio-user changed the way the mbuf dma address is
> >> retrieved, using a physical address in case of virtio-pci and a virtual
> >> address in case of virtio-user.
> >>
> >> This change introduced some possible memory corruption in packets,
> >> replacing:
> >>   m->buf_physaddr + RTE_PKTMBUF_HEADROOM
> >> by:
> >>   m->buf_physaddr + m->data_off     (through a macro)
> >>
> >> This patch fixes this issue, restoring the original behavior.
> >
> > Could you be more specific on why we cannot use m->data_off here?
> 
> There is no guarantee that m->data_off == RTE_PKTMBUF_HEADROOM here
> as
> virtqueue_enqueue_recv_refill() is called on a mbuf that is just
> allocated with rte_mbuf_raw_alloc(). An alternative would be to set
> data_off to RTE_PKTMBUF_HEADROOM, but as it's a fix and we are close to
> the release, I prefered to restore the initial behavior.

Oh yes, gotcha.

But if we do not set data_off properly, it's still buggy when others consume these mbufs, right?

Thanks,
Jianfeng

> 
> I did not include the test plan because it relies on patch that are not
> submitted yet (offload patches, they will be upstreamed very soon). It
> is a quite simple test case with testpmd.
> 
> Regards,
> Olivier

Re: [dpdk-dev] [PATCH] virtio: fix packet corruption

[Olivier Matz]

Hi,

On 07/19/2016 03:57 PM, Tan, Jianfeng wrote:
> 
> 
>> -----Original Message-----
>> From: Olivier Matz [mailto:olivier.matz@6wind.com]
>> Sent: Tuesday, July 19, 2016 9:11 PM
>> To: Tan, Jianfeng; dev@dpdk.org; Xie, Huawei; yuanhan.liu@linux.intel.com
>> Subject: Re: [PATCH] virtio: fix packet corruption
>>
>> Hi Jianfeng,
>>
>> On 07/19/2016 03:03 PM, Tan, Jianfeng wrote:
>>> Hi Oliver,
>>>
>>>> -----Original Message-----
>>>> From: Olivier Matz [mailto:olivier.matz@6wind.com]
>>>> Sent: Tuesday, July 19, 2016 8:32 PM
>>>> To: dev@dpdk.org; Tan, Jianfeng; Xie, Huawei;
>> yuanhan.liu@linux.intel.com
>>>> Subject: [PATCH] virtio: fix packet corruption
>>>>
>>>> The support of virtio-user changed the way the mbuf dma address is
>>>> retrieved, using a physical address in case of virtio-pci and a virtual
>>>> address in case of virtio-user.
>>>>
>>>> This change introduced some possible memory corruption in packets,
>>>> replacing:
>>>>   m->buf_physaddr + RTE_PKTMBUF_HEADROOM
>>>> by:
>>>>   m->buf_physaddr + m->data_off     (through a macro)
>>>>
>>>> This patch fixes this issue, restoring the original behavior.
>>>
>>> Could you be more specific on why we cannot use m->data_off here?
>>
>> There is no guarantee that m->data_off == RTE_PKTMBUF_HEADROOM here
>> as
>> virtqueue_enqueue_recv_refill() is called on a mbuf that is just
>> allocated with rte_mbuf_raw_alloc(). An alternative would be to set
>> data_off to RTE_PKTMBUF_HEADROOM, but as it's a fix and we are close to
>> the release, I prefered to restore the initial behavior.
> 
> Oh yes, gotcha.
> 
> But if we do not set data_off properly, it's still buggy when others consume these mbufs, right?
> 

This is done later in virtio_recv_pkts*() functions, one the host has
written the data in the mbuf.

Re: [dpdk-dev] [PATCH] virtio: fix packet corruption

[Tan, Jianfeng]

Hi Oliver,

> -----Original Message-----
> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> Sent: Tuesday, July 19, 2016 10:00 PM
> To: Tan, Jianfeng; dev@dpdk.org; Xie, Huawei; yuanhan.liu@linux.intel.com
> Subject: Re: [PATCH] virtio: fix packet corruption
> 
> Hi,
> 
> On 07/19/2016 03:57 PM, Tan, Jianfeng wrote:
> >
> >
> >> -----Original Message-----
> >> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> >> Sent: Tuesday, July 19, 2016 9:11 PM
> >> To: Tan, Jianfeng; dev@dpdk.org; Xie, Huawei;
> yuanhan.liu@linux.intel.com
> >> Subject: Re: [PATCH] virtio: fix packet corruption
> >>
> >> Hi Jianfeng,
> >>
> >> On 07/19/2016 03:03 PM, Tan, Jianfeng wrote:
> >>> Hi Oliver,
> >>>
> >>>> -----Original Message-----
> >>>> From: Olivier Matz [mailto:olivier.matz@6wind.com]
> >>>> Sent: Tuesday, July 19, 2016 8:32 PM
> >>>> To: dev@dpdk.org; Tan, Jianfeng; Xie, Huawei;
> >> yuanhan.liu@linux.intel.com
> >>>> Subject: [PATCH] virtio: fix packet corruption
> >>>>
> >>>> The support of virtio-user changed the way the mbuf dma address is
> >>>> retrieved, using a physical address in case of virtio-pci and a virtual
> >>>> address in case of virtio-user.
> >>>>
> >>>> This change introduced some possible memory corruption in packets,
> >>>> replacing:
> >>>>   m->buf_physaddr + RTE_PKTMBUF_HEADROOM
> >>>> by:
> >>>>   m->buf_physaddr + m->data_off     (through a macro)
> >>>>
> >>>> This patch fixes this issue, restoring the original behavior.
> >>>
> >>> Could you be more specific on why we cannot use m->data_off here?
> >>
> >> There is no guarantee that m->data_off == RTE_PKTMBUF_HEADROOM
> here
> >> as
> >> virtqueue_enqueue_recv_refill() is called on a mbuf that is just
> >> allocated with rte_mbuf_raw_alloc(). An alternative would be to set
> >> data_off to RTE_PKTMBUF_HEADROOM, but as it's a fix and we are close
> to
> >> the release, I prefered to restore the initial behavior.
> >
> > Oh yes, gotcha.
> >
> > But if we do not set data_off properly, it's still buggy when others consume
> these mbufs, right?
> >
> 
> This is done later in virtio_recv_pkts*() functions, one the host has
> written the data in the mbuf.

Thanks for clarification. Looks good to me.

Acked-by: Jianfeng Tan <jianfeng.tan@intel.com>

Re: [dpdk-dev] [PATCH] virtio: fix packet corruption

[Yuanhan Liu]

On Tue, Jul 19, 2016 at 02:31:59PM +0200, Olivier Matz wrote:
> The support of virtio-user changed the way the mbuf dma address is
> retrieved, using a physical address in case of virtio-pci and a virtual
> address in case of virtio-user.
> 
> This change introduced some possible memory corruption in packets,
> replacing:
>   m->buf_physaddr + RTE_PKTMBUF_HEADROOM
> by:
>   m->buf_physaddr + m->data_off     (through a macro)
> 
> This patch fixes this issue, restoring the original behavior.
> 
> By the way, it also rework the macros, adding a "VIRTIO_" prefix and
> API comments.
> 
> Fixes: f24f8f9fee8a ("net/virtio: allow virtual address to fill vring descriptors")
> 
> Signed-off-by: Olivier Matz <olivier.matz@6wind.com>

Thanks for the fix!

Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

	--yliu

Re: [dpdk-dev] [PATCH] virtio: fix packet corruption

[Thomas Monjalon]

2016-07-21 16:28, Yuanhan Liu:
> On Tue, Jul 19, 2016 at 02:31:59PM +0200, Olivier Matz wrote:
> > The support of virtio-user changed the way the mbuf dma address is
> > retrieved, using a physical address in case of virtio-pci and a virtual
> > address in case of virtio-user.
> > 
> > This change introduced some possible memory corruption in packets,
> > replacing:
> >   m->buf_physaddr + RTE_PKTMBUF_HEADROOM
> > by:
> >   m->buf_physaddr + m->data_off     (through a macro)
> > 
> > This patch fixes this issue, restoring the original behavior.
> > 
> > By the way, it also rework the macros, adding a "VIRTIO_" prefix and
> > API comments.
> > 
> > Fixes: f24f8f9fee8a ("net/virtio: allow virtual address to fill vring descriptors")
> > 
> > Signed-off-by: Olivier Matz <olivier.matz@6wind.com>
> 
> Thanks for the fix!
> 
> Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

Applied, thanks