[Intel-wired-lan] Reproducible TCP stream corruption with E810 & Linux 6.18.y

With heavy traffic, we have observed received TCP streams randomly
containing incorrect data in the middle. Generally the length of the
data does not change, a block of correct data just gets replaced with
incorrect data. In many instances the incorrect data has been
identified to belong to the same TCP stream but at much later offset.
It is unknown whether the data would have appeared also in the correct
position because the corruption was detected by the application and
the transfer terminated before it received that part of the stream.
The problem is not difficult to reproduce, takes usually a few minutes
of heavy traffic, and some real world traffic patterns seem to
reproduce the corruption with very high probability.


Hardware:

Intel Xeon x86-64 server
Dual port E810-XXV for SFP (rev 02)   8086:159b (Subsystem: 8086:0005)
firmware-version: 4.90 0x80020f00 1.3863.0
ICE OS Default Package version 1.3.41.0


Software:

Linux kernel 6.18.y with the in-tree 'ice' driver.
(testing described below done with patched 6.18.36)


Configuration:

Two ports of E810 in 802.3ad bond, a tagged VLAN on top
MTU 9000
IPv4


Potential root causes, from simplest to most likely:


1. ice_clean_rx_irq() may call ice_put_rx_mbuf() without calling
   ice_get_pgcnts()

This is very unlikely to happen in real use and probably won't cause
any bad effects even then, but feels unintended and maybe it still
should be something like this:

 			xdp_prepare_buff(xdp, hard_start, offset, size, !!offset);
 			xdp_buff_clear_frags_flag(xdp);
 		} else if (ice_add_xdp_frag(rx_ring, xdp, rx_buf, size)) {
+			ice_get_pgcnts(rx_ring, ntc);
 			ice_put_rx_mbuf(rx_ring, xdp, ntc, ICE_XDP_CONSUMED);
 			break;
 		}


2. ice_get_rx_buf() does not adjust the pagecnt_bias for size 0 buffers
   but ice_clean_rx_irq() may still enqueue them

Interestingly such buffers are skipped in ice_add_xdp_frag(), but not
if the descriptor is the first fragment of the frame. I haven't actually
traced whether this really causes the reference count to get corrupted,
but at least it feels suspicious to me. If zero length descriptors can
happen in the start of a frame, maybe something like this would be
appropriate:

 		if (!xdp->data) {
 			void *hard_start;
 
+			if (unlikely(!size)) {
+				if (!ice_is_non_eop(rx_ring, rx_desc)) {
+					ice_get_pgcnts(rx_ring, ntc);
+					ice_put_rx_mbuf(rx_ring, xdp, ntc, ICE_XDP_CONSUMED);
+				}
+				continue;
+			}
+
 			hard_start = page_address(rx_buf->page) + rx_buf->page_offset -
 				     offset;
 			xdp_prepare_buff(xdp, hard_start, offset, size, !!offset);

Since I don't even know if there can be zero length descriptors in here,
I'm not sure if the "unlikely" is really warranted or not. Also, I'm not
sure if doing the ice_put_rx_mbuf() here is appropriate or not, but I
just assumed that this might be the last round of the loop in which case
it might be nice to have the buffer properly dropped. This change also
means that such zero byte frames are not given to the XDP program either.
I have no idea if that is good or not (we don't currently have such
a thing).


3. zero size descriptors break the page flip logic

The condition in ice_can_reuse_rx_page() is this:

	/* if we are only owner of page we can reuse it */
	if (unlikely(rx_buf->pgcnt - pagecnt_bias > 1))
		return false;

My understanding of this is that normally the difference of 0 means
there is a reference counting bug, difference of 1 means the other half
of the page is free and any higher difference means the other half is
still in use, so looks correct.

However, since ice_get_rx_buf() does NOT adjust the bias for zero
size buffers, the differences now have alternate meaning. Now
a difference of 0 means that both halves are free and any difference 
of 1 or higher means that the CURRENT half is free (assuming it wasn't
enqueued anywhere) while the other half is still in use.

This means that flipping the page on zero size descriptors with the
current logic is very wrong and highly likely the root cause for the
observed corruption. Fixing this probably requires tracking somehow
if the bias was adjusted or not (or first decremented and later
incremented back) and NOT switching the half if it wasn't. One
possibility below, but obviously there are lots of ways of thinking
about this and I don't claim this is necessarily the best idea, just
to illustrate what I am talking about:

 	unsigned int page_offset;
 	unsigned int pgcnt;
 	unsigned int pagecnt_bias;
+#define ICE_RX_BUF_IN_USE	BIT(0)
+	u8 flags;
 };
 
 struct ice_q_stats {



 	struct ice_rx_buf *rx_buf;
 
 	rx_buf = &rx_ring->rx_buf[ntc];
+	rx_buf->flags = 0;
 	prefetchw(rx_buf->page);
 
 	if (!size)



 
 	/* We have pulled a buffer for use, so decrement pagecnt_bias */
 	rx_buf->pagecnt_bias--;
+	rx_buf->flags |= ICE_RX_BUF_IN_USE;
 
 	return rx_buf;
 }



 		 * as-is
 		 */
 		rx_buf->pagecnt_bias++;
+		rx_buf->flags &= ~ICE_RX_BUF_IN_USE;
 	}
 
 	if (unlikely(xdp_buff_has_frags(xdp))) {



 		 * To do this, only adjust pagecnt_bias for fragments up to
 		 * the total remaining after the XDP program has run.
 		 */
-		if (verdict != ICE_XDP_CONSUMED)
-			ice_rx_buf_adjust_pg_offset(buf, xdp->frame_sz);
-		else if (i++ <= xdp_frags)
-			buf->pagecnt_bias++;
+		if (buf->flags & ICE_RX_BUF_IN_USE) {
+			if (verdict != ICE_XDP_CONSUMED)
+				ice_rx_buf_adjust_pg_offset(buf, xdp->frame_sz);
+			else if (i++ <= xdp_frags)
+				buf->pagecnt_bias++;
+		}
 
 		ice_put_rx_buf(rx_ring, buf);
 	}


I guess this would also open a chance of small optimization in
ice_can_reuse_rx_page(), where the (unflipped) page should always
be reusable if the descriptor size was zero. That change was not
included in my testing because I didn't think of it until only
after I had already started the test.

One other question I got while looking at this is that in
ice_reuse_rx_page(), is it ever possible that old_buf and new_buf
are actually the same (probably not)? It was not trivial to see it,
but if it somehow is possible, then doing this afterwards feels
wrong:

	/* clear contents of buffer_info */
	rx_buf->page = NULL;


Anyways, I have now run almost two days of heavy TCP traffic (with
over 450 TB of data) with the changes shown above in the driver and
no corruption has been detected. Like mentioned, without the driver
changes it takes less then 10 minutes to break. While I'm not a Linux
Ethernet driver expert, far from it, this suggests that while I may
have missed some detail or implemented something incorrectly, the
theory itself about the root cause of the TCP corruption may have
some merit.

While there are some differences in the driver, at least some of this
looks like it may be applicable also to older Linux LTS branches.
At least a quick test with 6.12.y shows very similar corruption
appearing there too. In the latest 7.x mainline 'ice' driver the
whole bias and page flip logic seems to be gone, so none of this
directly applies there.