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

[wangyunjian <wangyunjian@huawei.com>]

> -----Original Message-----
> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]
> Sent: Thursday, April 9, 2020 5:52 PM
> To: wangyunjian <wangyunjian@huawei.com>; dev@dpdk.org
> Cc: keith.wiles@intel.com; Lilijun (Jerry) <jerry.lilijun@huawei.com>; xudingke
> <xudingke@huawei.com>; stable@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.yigit@intel.com]
> >> Sent: Tuesday, April 7, 2020 8:35 PM
> >> To: wangyunjian <wangyunjian@huawei.com>; dev@dpdk.org
> >> Cc: keith.wiles@intel.com; Lilijun (Jerry)
> >> <jerry.lilijun@huawei.com>; xudingke <xudingke@huawei.com>;
> >> stable@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 <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.
> >
> > 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 *
> >>>
> >