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

[Bruce Richardson <bruce.richardson@intel.com>]

On Fri, Nov 01, 2024 at 02:42:20PM +0100, Martin Weiser wrote:
> 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.
> 

Ok, that is fine if you think it better. I'll just expect V2 with some
added comments to clarify things.

Thanks,
/Bruce