On 4/9/2020 9:03 AM, wangyunjian wrote: > > >> -----Original Message----- >> From: Ferruh Yigit [mailto:[email protected]] >> Sent: Tuesday, April 7, 2020 8:35 PM >> To: wangyunjian <[email protected]>; [email protected] >> Cc: [email protected]; Lilijun (Jerry) <[email protected]>; >> xudingke >> <[email protected]>; [email protected] >> Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH v3 1/5] net/tap: fix mbuf double >> free when writev fails >> >> On 4/7/2020 5:22 AM, wangyunjian wrote: >>> From: Yunjian Wang <[email protected]> >>> >>> When the tap_write_mbufs() function return with break, mbuf was freed >>> without incrementing num_packets. This may lead applications also free >>> the mbuf. And the pmd_tx_burst() function should returns the number of >>> original packets it actually sent excluding tso mbufs. >>> >>> Fixes: 9396ad334672 ("net/tap: fix reported number of Tx packets") >>> CC: [email protected] >>> >>> Signed-off-by: Yunjian Wang <[email protected]> >>> --- >>> drivers/net/tap/rte_eth_tap.c | 21 +++++++++++++++------ >>> 1 file changed, 15 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/net/tap/rte_eth_tap.c >>> b/drivers/net/tap/rte_eth_tap.c index 05470a211..4c4b6b0b2 100644 >>> --- a/drivers/net/tap/rte_eth_tap.c >>> +++ b/drivers/net/tap/rte_eth_tap.c >>> @@ -521,7 +521,7 @@ tap_tx_l3_cksum(char *packet, uint64_t ol_flags, >> unsigned int l2_len, >>> } >>> } >>> >>> -static inline void >>> +static inline int >>> tap_write_mbufs(struct tx_queue *txq, uint16_t num_mbufs, >>> struct rte_mbuf **pmbufs, >>> uint16_t *num_packets, unsigned long *num_tx_bytes) @@ >> -588,7 >>> +588,7 @@ tap_write_mbufs(struct tx_queue *txq, uint16_t num_mbufs, >>> seg_len = rte_pktmbuf_data_len(mbuf); >>> l234_hlen = mbuf->l2_len + mbuf->l3_len + mbuf->l4_len; >>> if (seg_len < l234_hlen) >>> - break; >>> + return -1; >>> >>> /* To change checksums, work on a * copy of l2, l3 >>> * headers + l4 pseudo header >>> @@ -634,10 +634,12 @@ tap_write_mbufs(struct tx_queue *txq, uint16_t >> num_mbufs, >>> /* copy the tx frame data */ >>> n = writev(process_private->txq_fds[txq->queue_id], iovecs, j); >>> if (n <= 0) >>> - break; >>> + return -1; >>> + >>> (*num_packets)++; >>> (*num_tx_bytes) += rte_pktmbuf_pkt_len(mbuf); >>> } >>> + return 0; >>> } >>> >>> /* Callback to handle sending packets from the tap interface @@ >>> -708,8 +710,15 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs, >> uint16_t nb_pkts) >>> num_mbufs = 1; >>> } >>> >>> - tap_write_mbufs(txq, num_mbufs, mbuf, >>> - &num_packets, &num_tx_bytes); >>> + ret = tap_write_mbufs(txq, num_mbufs, mbuf, >>> + &num_packets, &num_tx_bytes); >> >> reusing 'ret' here breaks the logic at the end of the loop that free tso >> mbufs, >> which expects 'ret' is number of mbufs in tso case. >> >>> + if (ret != 0) { >>> + txq->stats.errs++; >>> + /* free tso mbufs */ >>> + for (j = 0; j < ret; j++) >> >> 'ret' only can be '0' or '-1', and we take the branch only when it is '-1', >> so this >> block is not used at all and it doesn't free any mbuf. > > I'm sorry for my mistakes. I will fix it in next version. > what about following: > > error = tap_write_mbufs(txq, num_mbufs, mbuf, > &num_packets, &num_tx_bytes); > if (error == -1) { > txq->stats.errs++; > /* free tso mbufs */ > for (j = 0; j < ret; j++) > rte_pktmbuf_free(mbuf[j]); > break; > }
+1, but still needs to free the 'mbuf_in' before break. Or maybe it is better to create a new variable like 'num_tso_mbufs' and use it instead of 'ret', which is more readable, and this enables to reuse the 'ret'. > > Thanks > Yunjian >>> + rte_pktmbuf_free(mbuf[j]); >> >> >> In the no tso case, if the 'tap_write_mbufs()' fails, this doesn't free the >> 'mbuf_in'. >> >>> + break; >>> + } >>> num_tx++; >>> /* free original mbuf */ >>> rte_pktmbuf_free(mbuf_in); >>> @@ -722,7 +731,7 @@ pmd_tx_burst(void *queue, struct rte_mbuf **bufs, >> uint16_t nb_pkts) >>> txq->stats.errs += nb_pkts - num_tx; >>> txq->stats.obytes += num_tx_bytes; >>> >>> - return num_packets; >>> + return num_tx; >> >> +1 to return number of original packets. >> >>> } >>> >>> static const char * >>> >
