From 0134a5c7b48c60abb7c96fa4d3362879732bc52a Mon Sep 17 00:00:00 2001 From: Chengchang Tang Date: Wed, 9 Sep 2020 17:23:39 +0800 Subject: [PATCH] net/hns3: fix crash when Tx multiple buffer packets Currently, there is a possibility that segment faults occur when sending packets whose payloads are stored in multiple buffers based on hns3 network engine. The related core dump information as follows: Program terminated with signal 11, Segmentation fault. 0 hns3_reassemble_tx_pkts 2512 temp = temp->next; Missing separate debuginfos, use: (gdb) bt 0 hns3_reassemble_tx_pkts 1 0x0000000000969c60 in hns3_check_non_tso_pkt 2 0x000000000096adbc in hns3_xmit_pkts 3 0x000000000050d4d0 in rte_eth_tx_burst 4 0x000000000050fca4 in pkt_burst_transmit 5 0x00000000004ca6b8 in run_pkt_fwd_on_lcore 6 0x00000000004ca7fc in start_pkt_forward_on_core 7 0x00000000006975a4 in eal_thread_loop 8 0x0000ffffa6f7fc48 in start_thread 9 0x0000ffffa6ed1600 in thread_start The root cause is that hns3 PMD driver invokes the rte_pktmbuf_free_seg API function to release the same rte_mbuf multiple times. The rte_mbuf pointer is not set to NULL in the internal function hns3_rx_queue_release_mbufs which is invoked during queue setup, stop and close. As a result the rte_mbuf in Rx queues will be repeatedly released when the user application setup queues or stop/start the dev for multiple times. Probably for performance reasons, DPDK mempool lib does not check for the repeated rte_mbuf releases. The Address of released rte_mbuf are directly stored into the per lcore cache of the mempool. This makes the rte_mbufs obtained from mempool by calling rte_mempool_get_bulk API function repetitively. ultimately, it causes to access to a NULL pointer in PMD driver. This patch fixes this problem by setting released mbuf pointer to NULL in the internal function named hns3_rx_queue_release_mbuf. And the other internal function named hns3_reassemble_tx_pkts is optimized to avoid a similar problem. Fixes: bba636698316 ("net/hns3: support Rx/Tx and related operations") Cc: stable@dpdk.org Signed-off-by: Chengchang Tang Signed-off-by: Wei Hu (Xavier) Signed-off-by: Chengwen Feng --- drivers/net/hns3/hns3_rxtx.c | 61 ++++++++++++++---------------------- 1 file changed, 23 insertions(+), 38 deletions(-) diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c index ada02de0c2..68d7a6a8ef 100644 --- a/drivers/net/hns3/hns3_rxtx.c +++ b/drivers/net/hns3/hns3_rxtx.c @@ -43,15 +43,19 @@ hns3_rx_queue_release_mbufs(struct hns3_rx_queue *rxq) if (rxq->rx_rearm_nb == 0) { for (i = 0; i < rxq->nb_rx_desc; i++) { - if (rxq->sw_ring[i].mbuf != NULL) + if (rxq->sw_ring[i].mbuf != NULL) { rte_pktmbuf_free_seg(rxq->sw_ring[i].mbuf); + rxq->sw_ring[i].mbuf = NULL; + } } } else { for (i = rxq->next_to_use; i != rxq->rx_rearm_start; i = (i + 1) % rxq->nb_rx_desc) { - if (rxq->sw_ring[i].mbuf != NULL) + if (rxq->sw_ring[i].mbuf != NULL) { rte_pktmbuf_free_seg(rxq->sw_ring[i].mbuf); + rxq->sw_ring[i].mbuf = NULL; + } } } @@ -2368,37 +2372,24 @@ hns3_fill_first_desc(struct hns3_tx_queue *txq, struct hns3_desc *desc, } } -static int -hns3_tx_alloc_mbufs(struct hns3_tx_queue *txq, struct rte_mempool *mb_pool, - uint16_t nb_new_buf, struct rte_mbuf **alloc_mbuf) +static inline int +hns3_tx_alloc_mbufs(struct rte_mempool *mb_pool, uint16_t nb_new_buf, + struct rte_mbuf **alloc_mbuf) { - struct rte_mbuf *new_mbuf = NULL; - struct rte_eth_dev *dev; - struct rte_mbuf *temp; - struct hns3_hw *hw; +#define MAX_NON_TSO_BD_PER_PKT 18 + struct rte_mbuf *pkt_segs[MAX_NON_TSO_BD_PER_PKT]; uint16_t i; /* Allocate enough mbufs */ - for (i = 0; i < nb_new_buf; i++) { - temp = rte_pktmbuf_alloc(mb_pool); - if (unlikely(temp == NULL)) { - dev = &rte_eth_devices[txq->port_id]; - hw = HNS3_DEV_PRIVATE_TO_HW(dev->data->dev_private); - hns3_err(hw, "Failed to alloc TX mbuf port_id=%d," - "queue_id=%d in reassemble tx pkts.", - txq->port_id, txq->queue_id); - rte_pktmbuf_free(new_mbuf); - return -ENOMEM; - } - temp->next = new_mbuf; - new_mbuf = temp; - } - - if (new_mbuf == NULL) + if (rte_mempool_get_bulk(mb_pool, (void **)pkt_segs, nb_new_buf)) return -ENOMEM; - new_mbuf->nb_segs = nb_new_buf; - *alloc_mbuf = new_mbuf; + for (i = 0; i < nb_new_buf - 1; i++) + pkt_segs[i]->next = pkt_segs[i + 1]; + + pkt_segs[nb_new_buf - 1]->next = NULL; + pkt_segs[0]->nb_segs = nb_new_buf; + *alloc_mbuf = pkt_segs[0]; return 0; } @@ -2418,10 +2409,8 @@ hns3_pktmbuf_copy_hdr(struct rte_mbuf *new_pkt, struct rte_mbuf *old_pkt) } static int -hns3_reassemble_tx_pkts(void *tx_queue, struct rte_mbuf *tx_pkt, - struct rte_mbuf **new_pkt) +hns3_reassemble_tx_pkts(struct rte_mbuf *tx_pkt, struct rte_mbuf **new_pkt) { - struct hns3_tx_queue *txq = tx_queue; struct rte_mempool *mb_pool; struct rte_mbuf *new_mbuf; struct rte_mbuf *temp_new; @@ -2433,7 +2422,6 @@ hns3_reassemble_tx_pkts(void *tx_queue, struct rte_mbuf *tx_pkt, uint16_t len_s; uint16_t len_d; uint16_t len; - uint16_t i; int ret; char *s; char *d; @@ -2449,7 +2437,7 @@ hns3_reassemble_tx_pkts(void *tx_queue, struct rte_mbuf *tx_pkt, last_buf_len = buf_size; /* Allocate enough mbufs */ - ret = hns3_tx_alloc_mbufs(txq, mb_pool, nb_new_buf, &new_mbuf); + ret = hns3_tx_alloc_mbufs(mb_pool, nb_new_buf, &new_mbuf); if (ret) return ret; @@ -2458,12 +2446,9 @@ hns3_reassemble_tx_pkts(void *tx_queue, struct rte_mbuf *tx_pkt, s = rte_pktmbuf_mtod(temp, char *); len_s = rte_pktmbuf_data_len(temp); temp_new = new_mbuf; - for (i = 0; i < nb_new_buf; i++) { + while (temp != NULL && temp_new != NULL) { d = rte_pktmbuf_mtod(temp_new, char *); - if (i < nb_new_buf - 1) - buf_len = buf_size; - else - buf_len = last_buf_len; + buf_len = temp_new->next == NULL ? last_buf_len : buf_size; len_d = buf_len; while (len_d) { @@ -2924,7 +2909,7 @@ hns3_check_non_tso_pkt(uint16_t nb_buf, struct rte_mbuf **m_seg, if (unlikely(nb_buf > HNS3_MAX_NON_TSO_BD_PER_PKT)) { txq->exceed_limit_bd_pkt_cnt++; - ret = hns3_reassemble_tx_pkts(txq, tx_pkt, &new_pkt); + ret = hns3_reassemble_tx_pkts(tx_pkt, &new_pkt); if (ret) { txq->exceed_limit_bd_reassem_fail++; return ret; -- 2.20.1