Re: [PATCH] igc: fix invalid length and corrupted multi-segment mbufs

[Martin Weiser <martin.weiser@allegro-packets.com>]

Hi Bruce,

thank you very much for your feedback.
Please see my answers inline below.

I will send a v2 of the patch.

Best regards,
Martin


Am 29.10.24 um 18:42 schrieb Bruce Richardson:
> On Mon, Oct 28, 2024 at 03:17:07PM +0100, Martin Weiser wrote:
>>
>> The issue only appeared with hardware-timestamping enabled
>> (RTE_ETH_RX_OFFLOAD_TIMESTAMP).
>>
>> The length of the prepended hardware timestamp was not subtracted from
>> the data length so that received packets were 16 bytes longer than
>> expected.
>>
>> In scatter-gather mode only the first mbuf has a timestamp but the
>> data offset of the follow-up mbufs was not adjusted accordingly.
>> This caused 16 bytes of packet data to be missing between
>> the segments.
>>
>> Signed-off-by: Martin Weiser <martin.weiser@allegro-packets.com>
>> ---
> 
> Hi,
> 
> thanks for the patch. Some comments inline below.
> 
> /Bruce
> 
>>  drivers/net/igc/igc_txrx.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/net/igc/igc_txrx.c b/drivers/net/igc/igc_txrx.c
>> index d0cee1b016..2fafa91bd5 100644
>> --- a/drivers/net/igc/igc_txrx.c
>> +++ b/drivers/net/igc/igc_txrx.c
>> @@ -347,6 +347,8 @@ igc_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts)
>>  
>>  		rxm->data_off = RTE_PKTMBUF_HEADROOM;
>>  		data_len = rte_le_to_cpu_16(rxd.wb.upper.length) - rxq->crc_len;
>> +		if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP)
>> +			data_len -= IGC_TS_HDR_LEN;
>>  		rxm->data_len = data_len;
>>  		rxm->pkt_len = data_len;
>>  		rxm->nb_segs = 1;
>> @@ -509,6 +511,12 @@ igc_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
>>  		 */
>>  		rxm->data_off = RTE_PKTMBUF_HEADROOM;
>>  		data_len = rte_le_to_cpu_16(rxd.wb.upper.length);
>> +		if (rxq->offloads & RTE_ETH_RX_OFFLOAD_TIMESTAMP) {
>> +			if (first_seg == NULL)
>> +				data_len -= IGC_TS_HDR_LEN;
>> +			else
>> +				rxm->data_off -= IGC_TS_HDR_LEN;
> 
> This initially confused me, because I was assuming that data_off was a typo
> for data_len. However, then I realised on closer examination that, when
> timestamp offload is enabled, the actual buffer addresses sent down to the
> hardware are offset by IGC_TS_HDR_LEN (meaning the first buffer of a pkt
> has the start of data at "normal" data_offset in mbuf, but subsequent
> buffers need adjustment). Is my understanding of the issue correct?

This is exactly right. Took me a bit to understand what was happening here and
I do not know if there might be a better way do this than to change the start
offset of all descriptors. But there is probably a reason why it was done this way.
Initially the issue was detected as forwarding of packets that maxed out the MTU
failed since they now, with the increased length, exceeded the MTU. Only then it
became apparent that the scatter-gather handling was also broken.

> 
> In either case, I have two small bits of feedback on this:
> * Firstly, I think this needs a comment explaining the logic here, to avoid
> others being confused as I was.

This will be part of patch v2.

> * Secondly, a very minor point, but is the code clearer or shorter, if you
> merge this extra code down into the next block which is already checking
> for the first_segment of a packet or not?

I would actually prefer to keep it that way as not to mix up the the data buffer
address and length handling with the mbuf chain construction.

> 
>> +		}
>>  		rxm->data_len = data_len;
>>  
>>  		/*
>> @@ -557,6 +565,7 @@ igc_recv_scattered_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
>>  				last_seg->data_len = last_seg->data_len -
>>  					 (RTE_ETHER_CRC_LEN - data_len);
>>  				last_seg->next = NULL;
>> +				rxm = last_seg;
>>  			} else {
>>  				rxm->data_len = (uint16_t)
>>  					(data_len - RTE_ETHER_CRC_LEN);
>> -- 
>> 2.47.0