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/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.

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