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.
> + 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 *
>
