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

[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 at 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?

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

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