Re: [dpdk-dev] [dpdk-stable] [PATCH v3 1/5] net/tap: fix mbuf double free when writev fails

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 4/7/2020 5:22 AM, wangyunjian wrote:
> From: Yunjian Wang <wangyunjian@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: stable@dpdk.org
> 
> Signed-off-by: Yunjian Wang <wangyunjian@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.

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