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

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 13 Jan 2025 10:04:50 +0000
Bruce Richardson <bruce.richardson@intel.com> wrote:

> 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@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@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

Ok, that makes sense.