On Mon, 6 Jan 2025 14:25:05 +0000
Bruce Richardson <bruce.richard...@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.richard...@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).
