Re: [PATCH net 1/6] igc: Fix RX HW timestamp reporting when NET_RX_BUSY_POLL is disabled

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 (3) · Medium: 0 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] When CONFIG_NET_RX_BUSY_POLL=n, the fix still returns wrong RX
  hardware timestamps to sockets bound to a PTP virtual clock…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the root cause
  the commit message names (get_timestamp() falling back to…
- [High] This problem wasn't introduced by this patch, but with
  CONFIG_NET_RX_BUSY_POLL=y (the common distro default)…
- [High] This problem wasn't introduced by this patch, but the
  CONFIG_NET_RX_BUSY_POLL=y branch of igc_construct_skb_timestamps()…

--

igc: Fix RX HW timestamp reporting when NET_RX_BUSY_POLL is disabled

When CONFIG_NET_RX_BUSY_POLL is disabled, get_timestamp() cannot find the
driver through the NAPI id. It then falls back to
skb_hwtstamps(skb)->hwtstamp, which igc never fills in. This patch makes
igc_construct_skb() and igc_construct_skb_zc() put the Timer0 timestamp
into hwtstamp directly in that configuration, and keeps the
SKBTX_HW_TSTAMP_NETDEV / netdev_data scheme when BUSY_POLL is enabled.

[Severity: High]
With CONFIG_NET_RX_BUSY_POLL=n, can sockets bound to a PTP virtual clock
with SOF_TIMESTAMPING_BIND_PHC still get wrong RX timestamps?

This branch always stores the Timer0 value and does not set
SKBTX_HW_TSTAMP_NETDEV. igc_get_tstamp() normally chooses the timer for
each socket:

drivers/net/ethernet/intel/igc/igc_main.c:igc_get_tstamp() {
    ...
	if (cycles)
		timestamp = igc_ptp_rx_pktstamp(adapter, tstamp->timer1);
	else
		timestamp = igc_ptp_rx_pktstamp(adapter, tstamp->timer0);
    ...
}

With the flag clear, __sock_recv_timestamp() uses hwtstamp as it is. For
BIND_PHC sockets it then converts the value:

net/socket.c:__sock_recv_timestamp() {
    ...
		if (tsflags & SOF_TIMESTAMPING_BIND_PHC)
			hwtstamp = ptp_convert_timestamp(&hwtstamp,
							 READ_ONCE(sk->sk_bind_phc));
    ...
}

ptp_convert_timestamp() passes the value to timecounter_cyc2time() as a
cycle count from the vclock's counter. For igc that counter is Timer1,
which igc_ptp_getcyclesx64() reads from IGC_SYSTIML_1/IGC_SYSTIMH_1.

igc provides getcyclesx64, so ptp_clock_freerun() returns false. That
means Timer0 can still be adjusted while vclocks exist, and Timer0 and
Timer1 can drift apart.

The Fixes: commit 069b142f5819 was added so that taprio/etf offload could
coexist with PTP vclocks. Does that use case stay broken when BUSY_POLL is
disabled?

[Severity: High]
This isn't a bug introduced by this patch, but the root cause in the
commit message seems to affect the other in-tree ndo_get_tstamp user,
tsnep, too. With CONFIG_NET_RX_BUSY_POLL=n, skb_napi_id() always returns 0:

include/linux/skbuff.h:skb_napi_id() {
#ifdef CONFIG_NET_RX_BUSY_POLL
	return skb->napi_id;
#else
	return 0;
#endif
}

So get_timestamp() in net/socket.c always falls back to
shhwtstamps->hwtstamp. tsnep_build_skb() in
drivers/net/ethernet/engleder/tsnep_main.c does:

		skb_shinfo(skb)->tx_flags |=
			SKBTX_HW_TSTAMP_NETDEV;
		memset(hwtstamps, 0, sizeof(*hwtstamps));
		hwtstamps->netdev_data = rx_inline;

Wouldn't tsnep then report the rx_inline kernel address as the RX
hardware timestamp, which is the same symptom described here? Once
hardware timestamping is enabled, an unprivileged socket using
SO_TIMESTAMPING with RX_HARDWARE|RAW_HARDWARE can receive this value.

The workaround here is an #ifndef inside igc only. Was a fix in
get_timestamp() in the core considered, so that tsnep is covered as well?

[Severity: High]
This is a pre-existing issue, but with CONFIG_NET_RX_BUSY_POLL=y this
branch stores only the kernel pointer ctx->rx_ts in the
netdev_data/hwtstamp union. hwtstamp never holds a real ktime. What
happens to code that reads ->hwtstamp without calling ndo_get_tstamp?

tpacket_get_timestamp() in net/packet/af_packet.c does not check
SKBTX_HW_TSTAMP_NETDEV:

	if (shhwtstamps &&
	    (flags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
	    ktime_to_timespec64_cond(shhwtstamps->hwtstamp, ts))
		return TP_STATUS_TS_RAW_HARDWARE;

So it looks like a TPACKET_V2/V3 ring with PACKET_TIMESTAMP set to
SOF_TIMESTAMPING_RAW_HARDWARE (for example tcpdump -j adapter_unsynced)
gets the RX buffer's kernel address in tp_sec/tp_nsec. Opening such a
socket needs CAP_NET_RAW.

get_timestamp() in net/socket.c also falls back to shhwtstamps->hwtstamp
with BUSY_POLL=y when dev_get_by_napi_id() fails. That can happen if
igc_reset_q_vector() and netif_napi_del() have removed the NAPI before
recvmsg runs.

[Severity: High]
This is also a pre-existing issue, but can netdev_data outlive the buffer
it points to? ctx->rx_ts points into the RX page, or into the UMEM frame
on the XSK path. The skb takes no reference on the bytes that hold the
timestamp header. igc_get_tstamp() dereferences the pointer later, at
recvmsg time:

	tstamp = hwtstamps->netdev_data;

	if (cycles)
		timestamp = igc_ptp_rx_pktstamp(adapter, tstamp->timer1);
	...

On the non-ZC path, a frame of up to IGC_RX_HDR_LEN bytes is copied
entirely into the linear area, so igc_construct_skb() takes this branch:

	} else {
		rx_buffer->pagecnt_bias++;
	}

No frag is attached in that case. igc_put_rx_buffer() then either gives
the same page half back to hardware through igc_reuse_rx_page(), or
frees it with __page_frag_cache_drain() when the page is not reusable.
igc_clean_rx_ring() also frees the pages on ifdown.

On the XSK path, igc_clean_rx_irq_zc() frees the frame right after the
skb is built:

		case IGC_XDP_PASS:
			igc_dispatch_skb_zc(q_vector, desc, ctx);
			fallthrough;
		case IGC_XDP_CONSUMED:
			xsk_buff_free(bi->xdp);

Later, xdp_umem_addr_unmap() vunmaps the UMEM.

In these cases, could igc_get_tstamp() read another packet's timestamp
or the contents of a freed page, or fault on an unmapped UMEM address?
The new !BUSY_POLL branch reads the timestamp synchronously and avoids
this, but the BUSY_POLL=y branch keeps the old behavior.

+#endif
+}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261001222443.3500206-1-anthony.l.nguyen%40intel.com