> -----Original Message----- > From: Ferruh Yigit [mailto:ferruh.yi...@intel.com] > Sent: Thursday, April 9, 2020 5:52 PM > To: wangyunjian <wangyunj...@huawei.com>; dev@dpdk.org > Cc: keith.wi...@intel.com; Lilijun (Jerry) <jerry.lili...@huawei.com>; > xudingke > <xudin...@huawei.com>; sta...@dpdk.org > Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH v3 1/5] net/tap: fix mbuf double > free when writev fails > > On 4/9/2020 9:03 AM, wangyunjian wrote: > > > > > >> -----Original Message----- > >> From: Ferruh Yigit [mailto:ferruh.yi...@intel.com] > >> Sent: Tuesday, April 7, 2020 8:35 PM > >> To: wangyunjian <wangyunj...@huawei.com>; dev@dpdk.org > >> Cc: keith.wi...@intel.com; Lilijun (Jerry) > >> <jerry.lili...@huawei.com>; xudingke <xudin...@huawei.com>; > >> sta...@dpdk.org > >> 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 <wangyunj...@huawei.com> > >>> > >>> 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: sta...@dpdk.org > >>> > >>> Signed-off-by: Yunjian Wang <wangyunj...@huawei.com> > >>> --- > >>> 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.
I don't think it needs to free the 'mbuf_in' before break. The 'num_tx' does not increase, the caller will free unsent packets. > > 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 for your suggestion, will include it in next version. Yunjian > > > > > 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 * > >>> > >
