[dpdk-stable] [dpdk-dev] [PATCH v3 1/5] net/tap: fix mbuf double free when writev fails
Ferruh Yigit
ferruh.yigit at intel.com
Thu Apr 9 15:03:53 CEST 2020
On 4/9/2020 1:53 PM, wangyunjian wrote:
>
>
>> -----Original Message-----
>> From: Ferruh Yigit [mailto:ferruh.yigit at intel.com]
>> Sent: Thursday, April 9, 2020 5:52 PM
>> To: wangyunjian <wangyunjian at huawei.com>; dev at dpdk.org
>> Cc: keith.wiles at intel.com; Lilijun (Jerry) <jerry.lilijun at huawei.com>; xudingke
>> <xudingke at huawei.com>; stable at 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 at intel.com]
>>>> Sent: Tuesday, April 7, 2020 8:35 PM
>>>> To: wangyunjian <wangyunjian at huawei.com>; dev at dpdk.org
>>>> Cc: keith.wiles at intel.com; Lilijun (Jerry)
>>>> <jerry.lilijun at huawei.com>; xudingke <xudingke at huawei.com>;
>>>> stable at 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 at 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 at dpdk.org
>>>>>
>>>>> Signed-off-by: Yunjian Wang <wangyunjian at 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.
Yep, you are right.
>
>>
>> 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 *
>>>>>
>>>
>
More information about the stable
mailing list