[dpdk-dev] [PATCH] ixgbe: fix crash caused by bulk allocation failure in vector pmd

[dpdk-dev] [PATCH] ixgbe: fix crash caused by bulk allocation failure in vector pmd

[Balazs Nemeth]

Since the introduction of vector PMD, a bug in ixgbe_rxq_rearm could
cause a crash. As long as the memory pool allocated to the RX queue
has mbufs available, there is no problem. After allocation of _all_
mbufs from the memory pool, previously returned mbufs by
rte_eth_rx_burst could be accessed by subsequent calls to the PMD and
could be returned by subsequent calls to rte_eth_rx_burst. From the
perspective of the application, the means that fields within the mbuf
could change and that previously allocated mbufs could appear multiple
times.

After failure of mbuf allocation, the dd bits should indicate that the
packets are not ready. For this, this patch adds code to reset the dd
bits in the first RTE_IXGBE_DESCS_PER_LOOP packets of the next
RTE_IXGBE_RXQ_REARM_THRESH packets only if the next
RTE_IXGBE_RXQ_REARM_THRESH packets that will be accessed contain
previously allocated packets.

Setting the bits is not enough. The bits are checked _after_ setting
the mbuf fields, thus a mechanism is needed to prevent the previously
used mbuf pointers from being accessed during the speculative load of
the mbuf fields. For this reason, not only the dd bits are reset, but
also the mbufs associated to those descriptors are set to point to a
"fake" mbuf.

Signed-off-by: Balazs Nemeth <balazs.nemeth@intel.com>
---
 lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c
index 203ddf7..457f267 100644
--- a/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c
+++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c
@@ -54,17 +54,28 @@ ixgbe_rxq_rearm(struct igb_rx_queue *rxq)
 	struct rte_mbuf *mb0, *mb1;
 	__m128i hdr_room = _mm_set_epi64x(RTE_PKTMBUF_HEADROOM,
 			RTE_PKTMBUF_HEADROOM);
+	__m128i dma_addr0, dma_addr1;
+
+	rxdp = rxq->rx_ring + rxq->rxrearm_start;
 
 	/* Pull 'n' more MBUFs into the software ring */
 	if (rte_mempool_get_bulk(rxq->mb_pool,
-				 (void *)rxep, RTE_IXGBE_RXQ_REARM_THRESH) < 0)
+				 (void *)rxep,
+				 RTE_IXGBE_RXQ_REARM_THRESH) < 0) {
+		if (rxq->rxrearm_nb + RTE_IXGBE_RXQ_REARM_THRESH >=
+		    rxq->nb_rx_desc) {
+			dma_addr0 = _mm_xor_si128(dma_addr0, dma_addr0);
+			for (i = 0; i < RTE_IXGBE_DESCS_PER_LOOP; i++) {
+				rxep[i].mbuf = &rxq->fake_mbuf;
+				_mm_store_si128((__m128i *)&rxdp[i].read,
+						dma_addr0);
+			}
+		}
 		return;
-
-	rxdp = rxq->rx_ring + rxq->rxrearm_start;
+	}
 
 	/* Initialize the mbufs in vector, process 2 mbufs in one loop */
 	for (i = 0; i < RTE_IXGBE_RXQ_REARM_THRESH; i += 2, rxep += 2) {
-		__m128i dma_addr0, dma_addr1;
 		__m128i vaddr0, vaddr1;
 
 		mb0 = rxep[0].mbuf;
-- 
2.1.0

Re: [dpdk-dev] [PATCH] ixgbe: fix crash caused by bulk allocation failure in vector pmd

[Ananyev, Konstantin]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Balazs Nemeth
> Sent: Friday, September 26, 2014 10:57 AM
> To: dev@dpdk.org
> Cc: Nemeth, Balazs
> Subject: [dpdk-dev] [PATCH] ixgbe: fix crash caused by bulk allocation failure in vector pmd
> 
> Since the introduction of vector PMD, a bug in ixgbe_rxq_rearm could
> cause a crash. As long as the memory pool allocated to the RX queue
> has mbufs available, there is no problem. After allocation of _all_
> mbufs from the memory pool, previously returned mbufs by
> rte_eth_rx_burst could be accessed by subsequent calls to the PMD and
> could be returned by subsequent calls to rte_eth_rx_burst. From the
> perspective of the application, the means that fields within the mbuf
> could change and that previously allocated mbufs could appear multiple
> times.
> 
> After failure of mbuf allocation, the dd bits should indicate that the
> packets are not ready. For this, this patch adds code to reset the dd
> bits in the first RTE_IXGBE_DESCS_PER_LOOP packets of the next
> RTE_IXGBE_RXQ_REARM_THRESH packets only if the next
> RTE_IXGBE_RXQ_REARM_THRESH packets that will be accessed contain
> previously allocated packets.
> 
> Setting the bits is not enough. The bits are checked _after_ setting
> the mbuf fields, thus a mechanism is needed to prevent the previously
> used mbuf pointers from being accessed during the speculative load of
> the mbuf fields. For this reason, not only the dd bits are reset, but
> also the mbufs associated to those descriptors are set to point to a
> "fake" mbuf.
> 
> Signed-off-by: Balazs Nemeth <balazs.nemeth@intel.com>
> ---
>  lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c b/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c
> index 203ddf7..457f267 100644
> --- a/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c
> +++ b/lib/librte_pmd_ixgbe/ixgbe_rxtx_vec.c
> @@ -54,17 +54,28 @@ ixgbe_rxq_rearm(struct igb_rx_queue *rxq)
>  	struct rte_mbuf *mb0, *mb1;
>  	__m128i hdr_room = _mm_set_epi64x(RTE_PKTMBUF_HEADROOM,
>  			RTE_PKTMBUF_HEADROOM);
> +	__m128i dma_addr0, dma_addr1;
> +
> +	rxdp = rxq->rx_ring + rxq->rxrearm_start;
> 
>  	/* Pull 'n' more MBUFs into the software ring */
>  	if (rte_mempool_get_bulk(rxq->mb_pool,
> -				 (void *)rxep, RTE_IXGBE_RXQ_REARM_THRESH) < 0)
> +				 (void *)rxep,
> +				 RTE_IXGBE_RXQ_REARM_THRESH) < 0) {
> +		if (rxq->rxrearm_nb + RTE_IXGBE_RXQ_REARM_THRESH >=
> +		    rxq->nb_rx_desc) {
> +			dma_addr0 = _mm_xor_si128(dma_addr0, dma_addr0);
> +			for (i = 0; i < RTE_IXGBE_DESCS_PER_LOOP; i++) {
> +				rxep[i].mbuf = &rxq->fake_mbuf;
> +				_mm_store_si128((__m128i *)&rxdp[i].read,
> +						dma_addr0);
> +			}
> +		}
>  		return;
> -
> -	rxdp = rxq->rx_ring + rxq->rxrearm_start;
> +	}
> 
>  	/* Initialize the mbufs in vector, process 2 mbufs in one loop */
>  	for (i = 0; i < RTE_IXGBE_RXQ_REARM_THRESH; i += 2, rxep += 2) {
> -		__m128i dma_addr0, dma_addr1;
>  		__m128i vaddr0, vaddr1;
> 
>  		mb0 = rxep[0].mbuf;
> --
> 2.1.0

Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

Re: [dpdk-dev] [PATCH] ixgbe: fix crash caused by bulk allocation failure in vector pmd

[Thomas Monjalon]

> > Since the introduction of vector PMD, a bug in ixgbe_rxq_rearm could
> > cause a crash. As long as the memory pool allocated to the RX queue
> > has mbufs available, there is no problem. After allocation of _all_
> > mbufs from the memory pool, previously returned mbufs by
> > rte_eth_rx_burst could be accessed by subsequent calls to the PMD and
> > could be returned by subsequent calls to rte_eth_rx_burst. From the
> > perspective of the application, the means that fields within the mbuf
> > could change and that previously allocated mbufs could appear multiple
> > times.
> > 
> > After failure of mbuf allocation, the dd bits should indicate that the
> > packets are not ready. For this, this patch adds code to reset the dd
> > bits in the first RTE_IXGBE_DESCS_PER_LOOP packets of the next
> > RTE_IXGBE_RXQ_REARM_THRESH packets only if the next
> > RTE_IXGBE_RXQ_REARM_THRESH packets that will be accessed contain
> > previously allocated packets.
> > 
> > Setting the bits is not enough. The bits are checked _after_ setting
> > the mbuf fields, thus a mechanism is needed to prevent the previously
> > used mbuf pointers from being accessed during the speculative load of
> > the mbuf fields. For this reason, not only the dd bits are reset, but
> > also the mbufs associated to those descriptors are set to point to a
> > "fake" mbuf.
> > 
> > Signed-off-by: Balazs Nemeth <balazs.nemeth@intel.com>
> 
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

Applied

Thanks
-- 
Thomas