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

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

[wangyunjian]

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);
+		if (ret != 0) {
+			txq->stats.errs++;
+			/* free tso mbufs */
+			for (j = 0; j < ret; j++)
+				rte_pktmbuf_free(mbuf[j]);
+			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;
 }
 
 static const char *
-- 
2.19.1

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

[Ferruh Yigit]

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

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

[wangyunjian]

> -----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;
}

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

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

[Ferruh Yigit]

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

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

[wangyunjian]

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

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

[Ferruh Yigit]

On 4/9/2020 1:53 PM, wangyunjian wrote:
> 
> 
>> -----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.

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

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

[Stephen Hemminger]

On Thu, 9 Apr 2020 08:03:23 +0000
wangyunjian <wangyunjian@huawei.com> wrote:

> 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;
> }

There is a free bulk, and normally each buf counts against errors.

        if (error == -1) {
		txq->stats.errs += num_packets;
		rte_pktmbuf_free_bulk(mbuf, num_packets);
		break;
	}

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

[wangyunjian]

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Thursday, April 9, 2020 10:51 PM
> To: wangyunjian <wangyunjian@huawei.com>
> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org;
> keith.wiles@intel.com; Lilijun (Jerry) <jerry.lilijun@huawei.com>; xudingke
> <xudingke@huawei.com>; stable@dpdk.org
> Subject: Re: [dpdk-dev] [dpdk-stable] [PATCH v3 1/5] net/tap: fix mbuf double
> free when writev fails
> 
> On Thu, 9 Apr 2020 08:03:23 +0000
> wangyunjian <wangyunjian@huawei.com> wrote:
> 
> > 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;
> > }
> 
> There is a free bulk, and normally each buf counts against errors.
> 
>         if (error == -1) {
> 		txq->stats.errs += num_packets;

I think to consider only the original packet number, not sent packets.

> 		rte_pktmbuf_free_bulk(mbuf, num_packets);

Thanks for your suggestion, will include it in next version.

Yunjian
> 		break;
> 	}

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

[Ferruh Yigit]

On 4/10/2020 2:41 AM, wangyunjian wrote:
>> -----Original Message-----
>> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
>> Sent: Thursday, April 9, 2020 10:51 PM
>> To: wangyunjian <wangyunjian@huawei.com>
>> Cc: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org;
>> keith.wiles@intel.com; Lilijun (Jerry) <jerry.lilijun@huawei.com>; xudingke
>> <xudingke@huawei.com>; stable@dpdk.org
>> Subject: Re: [dpdk-dev] [dpdk-stable] [PATCH v3 1/5] net/tap: fix mbuf double
>> free when writev fails
>>
>> On Thu, 9 Apr 2020 08:03:23 +0000
>> wangyunjian <wangyunjian@huawei.com> wrote:
>>
>>> 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;
>>> }
>>
>> There is a free bulk, and normally each buf counts against errors.
>>
>>         if (error == -1) {
>> 		txq->stats.errs += num_packets;
> 
> I think to consider only the original packet number, not sent packets.

+1. 'mubf' here is the output of the 'rte_gso_segment()', the number of mbuf
application sent is 1.

> 
>> 		rte_pktmbuf_free_bulk(mbuf, num_packets);
> 
> Thanks for your suggestion, will include it in next version.
> 
> Yunjian
>> 		break;
>> 	}
>