Re: [PATCH v2 06/13] net/txgbe: check length of Tx packets

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

On 11/1/2024 1:52 AM, Jiawen Wu wrote:
> On Fri, Nov 1, 2024 9:23 AM, Ferruh Yigit wrote:
>> On 10/28/2024 2:31 AM, Jiawen Wu wrote:
>>> Add checking of the Tx packet length to avoid TDM fatal error as far as
>>> possible. Set the pkt_len=1518 for invalid packet in simple Tx code path,
>>> and drop it directly in featured Tx code path.
>>>
>>> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
>>> ---
>>>  drivers/net/txgbe/txgbe_rxtx.c          | 33 +++++++++++++++++++++++++
>>>  drivers/net/txgbe/txgbe_rxtx_vec_neon.c | 10 +++++---
>>>  drivers/net/txgbe/txgbe_rxtx_vec_sse.c  | 11 ++++++---
>>>  3 files changed, 48 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/net/txgbe/txgbe_rxtx.c b/drivers/net/txgbe/txgbe_rxtx.c
>>> index 2d2b437643..06acbd0881 100644
>>> --- a/drivers/net/txgbe/txgbe_rxtx.c
>>> +++ b/drivers/net/txgbe/txgbe_rxtx.c
>>> @@ -160,6 +160,8 @@ tx4(volatile struct txgbe_tx_desc *txdp, struct rte_mbuf **pkts)
>>>  	for (i = 0; i < 4; ++i, ++txdp, ++pkts) {
>>>  		buf_dma_addr = rte_mbuf_data_iova(*pkts);
>>>  		pkt_len = (*pkts)->data_len;
>>> +		if (pkt_len < RTE_ETHER_HDR_LEN)
>>> +			pkt_len = TXGBE_FRAME_SIZE_DFT;
>>>
>>>  		/* write data to descriptor */
>>>  		txdp->qw0 = rte_cpu_to_le_64(buf_dma_addr);
>>> @@ -180,6 +182,8 @@ tx1(volatile struct txgbe_tx_desc *txdp, struct rte_mbuf **pkts)
>>>
>>>  	buf_dma_addr = rte_mbuf_data_iova(*pkts);
>>>  	pkt_len = (*pkts)->data_len;
>>> +	if (pkt_len < RTE_ETHER_HDR_LEN)
>>> +		pkt_len = TXGBE_FRAME_SIZE_DFT;
>>>
>>>  	/* write data to descriptor */
>>>  	txdp->qw0 = cpu_to_le64(buf_dma_addr);
>>> @@ -813,6 +817,30 @@ txgbe_parse_tun_ptid(struct rte_mbuf *tx_pkt, uint8_t tun_len)
>>>  	return ptid;
>>>  }
>>>
>>> +static inline bool
>>> +txgbe_check_pkt_err(struct rte_mbuf *tx_pkt)
>>> +{
>>> +	uint32_t total_len = 0, nb_seg = 0;
>>> +	struct rte_mbuf *mseg;
>>> +
>>> +	mseg = tx_pkt;
>>> +	do {
>>> +		if (mseg->data_len == 0)
>>> +			return true;
>>> +		total_len += mseg->data_len;
>>> +		nb_seg++;
>>> +		mseg = mseg->next;
>>> +	} while (mseg != NULL);
>>> +
>>> +	if (tx_pkt->pkt_len != total_len || tx_pkt->pkt_len == 0)
>>> +		return true;
>>> +
>>> +	if (tx_pkt->nb_segs != nb_seg || tx_pkt->nb_segs > 64)
>>> +		return true;
>>> +
>>> +	return false;
>>> +}
>>> +
>>>
>>
>> Hi Jiawen,
>>
>> Above are generic checks, we may add this function to ethdev driver
>> header (ethdev_driver.h) so that any PMD can use it, what do you think?
> 
> In fact, the limitation of tx_pkt->nb_segs is already implemented in
> .tx_pkt_prepare. But users developing their own apps don't necessarily call
> this function. So I'd like to add these in txgbe_xmit_pkts(). What are you
> going to do about it?
> 
> 

I had the same thought, that is why I am not suggesting to add these
checks to .tx_pkt_prepare() function.

But still these generic checks can go into a new static inline function
for drivers to use, not for end users, and any other driver can call
this function. Your two drivers already using the same function.

This is not hard requirement, but if there is a common code for multiple
drivers, that can go into a helper function in ethdev_driver.h