On 4/9/2020 1:53 PM, wangyunjian wrote: > > >> -----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.
Yep, you are right. > >> >> 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 * >>>>> >>> >
