Re: [PATCH v4 01/24] net/_common_intel: add pkt reassembly fn for intel drivers

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

On Fri, Dec 20, 2024 at 08:15:40AM -0800, Stephen Hemminger wrote:
> On Fri, 20 Dec 2024 14:38:58 +0000
> Bruce Richardson <bruce.richardson@intel.com> wrote:
> 
> > +
> > +			if (!split_flags[buf_idx]) {
> > +				/* it's the last packet of the set */
> > +				start->hash = end->hash;
> > +				start->vlan_tci = end->vlan_tci;
> > +				start->ol_flags = end->ol_flags;
> > +				/* we need to strip crc for the whole packet */
> > +				start->pkt_len -= crc_len;
> > +				if (end->data_len > crc_len) {
> > +					end->data_len -= crc_len;
> > +				} else {
> > +					/* free up last mbuf */
> > +					struct rte_mbuf *secondlast = start;
> > +
> > +					start->nb_segs--;
> > +					while (secondlast->next != end)
> > +						secondlast = secondlast->next;
> > +					secondlast->data_len -= (crc_len - end->data_len);
> > +					secondlast->next = NULL;
> > +					rte_pktmbuf_free_seg(end);
> > +				}
> 
> The problem with freeing the last buffer is that the CRC will be garbage.
> What if the CRC is sitting past the last mbuf?
> 
> +-----------------------+    +-----+
> | Data                  +--->+ CRC |
> +-----------------------+    +-----+
> 
> This part (from original code) will free the second mbuf which contains
> the CRC. The whole "don't strip CRC and leave it past the mbuf data" model
> of mbuf's is a danger trap.

Can you explain more clearly what you see as the issue with the current
code above?

The "crc_len" variable contains the length of the CRC included in the
packet, which should be removed from that before returning the mbuf from RX.
It contains "0" if the CRC is HW stripped, and "4" if the CRC needs to be
removed by software - something that is done just by subtracting the CRC
length from the packet and buffer lengths. The freeing of the last segment
occurs only in the case that the last segment contains the CRC only - or
part of the CRC only, as otherwise we would have an extra empty buffer at
the end of the chain.

/Bruce