From: Tjerk Kusters <[email protected]>
When Rx hardware timestamping is enabled (e.g. ptp4l, which configures
HWTSTAMP_FILTER_ALL), the NIC prepends a 16-byte timestamp header to the
first Rx buffer of every received frame. igb_clean_rx_irq() strips this
header inside its per-buffer loop:
if (igb_test_staterr(rx_desc, E1000_RXDADV_STAT_TSIP)) {
ts_hdr_len = igb_ptp_rx_pktstamp(rx_ring->q_vector,
pktbuf, ×tamp);
pkt_offset += ts_hdr_len;
size -= ts_hdr_len;
}
For a frame that spans more than one Rx buffer (e.g. a jumbo frame), this
block runs once per buffer. The timestamp header only exists at the start
of the first buffer, but igb_ptp_rx_pktstamp() is called for every buffer.
On a continuation buffer the data is packet payload, not a timestamp
header. igb_ptp_rx_pktstamp() already has two guards against acting on a
non-header buffer: it returns 0 if PTP is disabled, and returns 0 if the
reserved dwords (the first 8 bytes) are non-zero. Neither is sufficient
here: PTP is enabled, and a continuation buffer whose payload happens to
begin with 8 zero bytes passes the reserved-dword check. In that case the
payload is mistaken for a valid timestamp header and igb_ptp_rx_pktstamp()
returns IGB_TS_HDR_LEN, and the caller reduces that buffer's size by 16.
igb_add_rx_frag() adds a continuation buffer from its original offset, so
the buffer loses its last 16 bytes and the data of the next buffer
follows directly. A frame spanning N buffers loses 16 bytes per affected
continuation buffer, in the middle of the payload when N > 2. If the last
buffer of a frame holds 8 to 15 bytes, all zero, the unsigned size wraps
and a fragment of about 4 GiB is added to the skb. The sender chooses the
frame length and content, so this can be triggered remotely.
This is easily triggered by a GigE Vision camera streaming dark frames
(mostly 0x00 pixel data) over jumbo UDP with PTP active on the receiver:
the all-zero frames are dropped as truncated while frames with non-zero
content are fine. The driver reports no error; only the IP layer counts
the frames in IpExtInTruncatedPkts.
No content-based check can reliably tell a continuation buffer that begins
with zero bytes from a real timestamp header, because both are all zero.
Fix it structurally instead: only attempt the strip on the first buffer of
a frame, which is the only buffer that can contain a timestamp header. In
igb_clean_rx_irq() skb is NULL until the first buffer has been processed,
so guarding the strip with !skb restricts it to the first buffer
regardless of payload content.
This relies on the previous patch. Without it, an XDP program that does
not return XDP_PASS for the first buffer of a multi-buffer frame leaves
skb NULL for the rest of that frame. With the receive length limited
while XDP is attached, such frames no longer reach the XDP path.
igc has the same bug; Paul Moses' igc series, linked below, fixes it the
same way.
Link: https://lore.kernel.org/all/[email protected]/
Fixes: 5379260852b0 ("igb: Fix XDP with PTP enabled")
Cc: [email protected]
Reviewed-by: Piotr Kwapulinski <[email protected]>
Reviewed-by: Aleksandr Loktionov <[email protected]>
Reviewed-by: Kurt Kanzenbach <[email protected]>
Tested-by: Alexander Nowlin <[email protected]>
Signed-off-by: Tjerk Kusters <[email protected]>
---
drivers/net/ethernet/intel/igb/igb_main.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c
b/drivers/net/ethernet/intel/igb/igb_main.c
index 8abdd4e58550..cf9911137bd3 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -9122,8 +9122,11 @@ static int igb_clean_rx_irq(struct igb_q_vector
*q_vector, const int budget)
rx_buffer = igb_get_rx_buffer(rx_ring, size, &rx_buf_pgcnt);
pktbuf = page_address(rx_buffer->page) + rx_buffer->page_offset;
- /* pull rx packet timestamp if available and valid */
- if (igb_test_staterr(rx_desc, E1000_RXDADV_STAT_TSIP)) {
+ /* pull rx packet timestamp if available and valid; it is only
+ * present on the first buffer of a frame
+ */
+ if (!skb &&
+ igb_test_staterr(rx_desc, E1000_RXDADV_STAT_TSIP)) {
int ts_hdr_len;
ts_hdr_len = igb_ptp_rx_pktstamp(rx_ring->q_vector,
--
2.27.0
