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

[Bruce Richardson]

On Sat, Jan 11, 2025 at 09:07:14AM -0800, Stephen Hemminger wrote:
> On Mon, 6 Jan 2025 14:25:05 +0000
> Bruce Richardson <bruce.richardson at intel.com> wrote:
> 
> > 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 at 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.
> 
> But if the application is using the driver in "keep CRC" mode,
> then it probably intends to look at the CRC. In that case crc_len is 4,
> and that trailing buffer may need to be retained.
> 
> It all goes back to the design mistake of the semantics of "keep CRC"
> mode. In that mode, the mbuf chain has hidden data (past pkt_len and data_len).
> 

If CRC is not to be stripped, then crc_len should be zero here also, in
which case nothing happens for freeing data. [If crc_len is not zero in
that case, that's a bug in the config path, not datapath here]

/Bruce