> -----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;
}
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 *
> >
